Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Renames to follow .NET naming conventions #69547

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Dec 4, 2022

Renamed Godot structs
Original name New name
AABB Aabb
Rect2i Rect2I
RID Rid
Vector2i Vector2I
Vector3i Vector3I
Vector4i Vector4I
Renamed Godot classes
Original name New name
AESContext AesContext
AudioStreamWAV AudioStreamWav
CPUParticles2D CpuParticles2D
CPUParticles3D CpuParticles3D
CSGBox3D CsgBox3D
CSGCombiner3D CsgCombiner3D
CSGCylinder3D CsgCylinder3D
CSGMesh3D CsgMesh3D
CSGPolygon3D CsgPolygon3D
CSGPrimitive3D CsgPrimitive3D
CSGShape3D CsgShape3D
CSGSphere3D CsgSphere3D
CSGTorus3D CsgTorus3D
CurveXYZTexture CurveXyzTexture
DTLSServer DtlsServer
EditorSceneFormatImporterFBX EditorSceneFormatImporterFbx
EditorSceneFormatImporterGLTF EditorSceneFormatImporterGltf
EditorVCSInterface EditorVcsInterface
EncodedObjectAsID EncodedObjectAsId
Generic6DOFJoint3D Generic6DofJoint3D
GLTFAccessor GltfAccessor
GLTFAnimation GltfAnimation
GLTFBufferView GltfBufferView
GLTFCamera GltfCamera
GLTFDocument GltfDocument
GLTFDocumentExtension GltfDocumentExtension
GLTFDocumentExtensionConvertImporterMesh GltfDocumentExtensionConvertImporterMesh
GLTFLight GltfLight
GLTFMesh GltfMesh
GLTFNode GltfNode
GLTFSkeleton GltfSkeleton
GLTFSkin GltfSkin
GLTFSpecGloss GltfSpecGloss
GLTFState GltfState
GLTFTexture GltfTexture
GLTFTextureSampler GltfTextureSampler
GPUParticles2D GpuParticles2D
GPUParticles3D GpuParticles3D
GPUParticlesAttractor3D GpuParticlesAttractor3D
GPUParticlesAttractorBox3D GpuParticlesAttractorBox3D
GPUParticlesAttractorSphere3D GpuParticlesAttractorSphere3D
GPUParticlesAttractorVectorField3D GpuParticlesAttractorVectorField3D
GPUParticlesCollision3D GpuParticlesCollision3D
GPUParticlesCollisionBox3D GpuParticlesCollisionBox3D
GPUParticlesCollisionHeightField3D GpuParticlesCollisionHeightField3D
GPUParticlesCollisionSDF3D GpuParticlesCollisionSdf3D
GPUParticlesCollisionSphere3D GpuParticlesCollisionSphere3D
HMACContext HmacContext
HTTPClient HttpClient
HTTPRequest HttpRequest
InputEventMIDI InputEventMidi
JNISingleton JniSingleton
JSON Json
JSONRPC JsonRpc
MultiplayerAPI MultiplayerApi
MultiplayerAPIExtension MultiplayerApiExtension
Object GodotObject
ORMMaterial3D OrmMaterial3D
PacketPeerDTLS PacketPeerDtls
PacketPeerUDP PacketPeerUdp
PCKPacker PckPacker
RDShaderSPIRV RDShaderSpirV
ResourceUID ResourceUid
SkeletonModification2DCCDIK SkeletonModification2DCcdik
SkeletonModification2DFABRIK SkeletonModification2DFabrik
SkeletonModification3DCCDIK SkeletonModification3DCcdik
SkeletonModification3DFABRIK SkeletonModification3DFabrik
StreamPeerGZIP StreamPeerGZip
StreamPeerTCP StreamPeerTcp
StreamPeerTLS StreamPeerTls
TCPServer TcpServer
Thread GodotThread
UDPServer UdpServer
UPNP Upnp
UPNPDevice UpnpDevice
VisualShaderNodeCurveXYZTexture VisualShaderNodeCurveXyzTexture
VisualShaderNodeScreenUVToSDF VisualShaderNodeScreenUVToSdf
VisualShaderNodeSDFRaymarch VisualShaderNodeSdfRaymarch
VisualShaderNodeSDFToScreenUV VisualShaderNodeSdfToScreenUV
VisualShaderNodeTextureSDF VisualShaderNodeTextureSdf
VisualShaderNodeTextureSDFNormal VisualShaderNodeTextureSdfNormal
WebRTCDataChannel WebRtcDataChannel
WebRTCDataChannelExtension WebRtcDataChannelExtension
WebRTCMultiplayerPeer WebRtcMultiplayerPeer
WebRTCPeerConnection WebRtcPeerConnection
WebRTCPeerConnectionExtension WebRtcPeerConnectionExtension
XMLParser XmlParser
ZIPPacker ZipPacker
ZIPReader ZipReader

@raulsntos raulsntos force-pushed the dotnet/pascal-case branch 9 times, most recently from 57da7d4 to 40b0ecf Compare December 6, 2022 01:26
@Chaosus Chaosus added this to the 4.0 milestone Dec 6, 2022
@Chaosus Chaosus requested a review from neikeq December 6, 2022 09:26
@raulsntos raulsntos force-pushed the dotnet/pascal-case branch 6 times, most recently from c347d5c to 3766159 Compare December 7, 2022 05:07
@raulsntos raulsntos marked this pull request as ready for review December 7, 2022 05:34
@raulsntos raulsntos requested review from a team as code owners December 7, 2022 05:35
@raulsntos raulsntos force-pushed the dotnet/pascal-case branch 2 times, most recently from ccfd30c to 4b01ff1 Compare December 7, 2022 18:56
@raulsntos raulsntos force-pushed the dotnet/pascal-case branch 3 times, most recently from e7839eb to d8cf926 Compare December 16, 2022 19:40
@raulsntos raulsntos force-pushed the dotnet/pascal-case branch 2 times, most recently from 400d13f to b8b25c2 Compare January 6, 2023 15:14
Renamed C# types and members to use PascalCase and follow .NET naming conventions.
@akien-mga akien-mga merged commit 01b5644 into godotengine:master Jan 27, 2023
@akien-mga
Copy link
Member

Thanks!

@Zhersi
Copy link

Zhersi commented Jan 29, 2023

  • Renamed Godot.Object to GodotObject to avoid ambiguity with System.Object.
  • Renamed Godot.Thread to GodotThread to avoid ambiguity with System.Threading.Thread.

@raulsntos

It seems a little redundant to me to prefix anything in the Godot namespace with Godot just for the sake of avoiding ambiguity.

Perhaps a "using alias directive" would be better suited for avoiding ambiguities like this?

While I would likely prefer to use Godot.Object and System.Object in full personally, a person wishing to rename/alias Godot.Object to avoid ambiguity with System.Object could simply:

using Godot;
using System;
using GodotObject = Godot.Object;

Alternatively, I believe this would also work:

using Godot;
using System;
using Object = Godot.Object;

@neikeq
Copy link
Contributor

neikeq commented Jan 29, 2023

@Zhersi Adding those aliases for each file is too annoying. And if you happen to forget it, you could be using the wrong class by accident. Additionally, it's about readability as well (it's hard to know which Object class is being used).

Because of all that, often in our own code I tend to explicitly write Godot.Object, for which I need to add a comment to disable ReSharper warnings about redundancy:

// ReSharper disable once RedundantNameQualifier
where T : Godot.Object

@Zhersi
Copy link

Zhersi commented Jan 29, 2023

@neikeq
Thanks for sharing your thoughts!

I agree that having to use aliases for each file can be annoying, which is why I also prefer to explicitly write Godot.Object and System.Object, however I wanted to share the possibility of using aliases for people who want different names for these classes.

My main issue is that I find it needlessly verbose (and redundant) to prefix for example Object with Godot when it already belongs to the Godot namespace (Godot.GodotObject).

I would much rather the Godot prefix be removed, in favor of letting users figure out how to resolve any potential ambiguities on their own.

@Kyniverse
Copy link

I would like to throw my voice into this as well by agreeing with @Zhersi on the matter of redundancy-

One potential solution to having to define an alias for each file would be to make use of Global Using Directives. This allows one to define the alias in a single file that will then be used throughout the entire project without having to worry about the redundancy or aliasing:

image

Another potential solution would be to rename Object to something like GObject in order to still have a slight deviation from Object itself without having the need to add redundant naming schemes resulting in something like Godot.GodotObject

@Zhersi
Copy link

Zhersi commented Jan 29, 2023

@Kyniverse

Thanks for introducing me to "global using directives"!
That would indeed prevent someone from having to create an alias in every file.
(Solving that issue for anyone wanting to use aliases instead of explicitly writing ex. System.Object)

In regards to renaming GodotObject to GObject, while it is less verbose, I still find it slightly redundant for the same reason as pointed out in my previous comment.

@rambda
Copy link

rambda commented Jan 30, 2023

GObject makes perfect sense to me, since people who're reading to code definitely knows G stands for Godot. Just like Unreal's UObject.

@neikeq
Copy link
Contributor

neikeq commented Jan 31, 2023

I didn't go with GObject to avoid confusion because that name is commonly associated with the GLib project. It could be GDObect but I don't like the 3 capital letters.

I have at least the following issues with global using directives:

  • The semantics of a source file can change due to factors unrelated to the file itself or the namespaces it uses. This was already the case before, but it wasn't as problematic in practice. With global usings, a source file (which could have been copied from somewhere else) which was meant to reference the System.Object class, would now be referencing Godot.Object.
  • A global using alias cannot be overridden (in the same namespace) by a specific file (no using Object = System.Object;), as it counts as a duplicate.
  • It can easily cause confusion, as Object is commonly associated with System.Object, and the only disambiguation to be found is in another file.

Honestly, messing with global aliases in someone else's project just sounds wrong to me.

At the end of the day, the redundant naming in Godot.GodotObject is but a slight annoyance that's worth it to solve a real problem. I'm open to consider other naming options, but it's unlikely that we return to Object.

@Zhersi
Copy link

Zhersi commented Jan 31, 2023

Thanks for taking the time to respond again @neikeq!
I really hope I am not making too much trouble over naming conventions/policies.

I cannot help but ask if something should be renamed just to avoid it having an identical name to something in another namespace. Is not some of the point of namespaces to allow for exactly this to be the case in the first place?

Renaming Object to GodotObject does solve the ambiguity between System.Object and Godot.Object.
However, should this really be the policy for resolving ambiguities of this kind?

What about other ambiguities such as:

System Godot
System.Array Godot.Collections.Array
System.Drawing.Color Godot.Color
System.Numerics.Vector2 Godot.Vector2
System.Numerics.Vector3 Godot.Vector3

Should these also be renamed (potentially prefixed "Godot")?

My personal opinion is that it would be better to revert the names of GodotObject and GodotThread to Object and Thread respectively, and let users handle ambiguities like this (be it by aliases or by explicitly stating the namespace, ex. Godot.Object),

@raulsntos
Copy link
Member Author

The .NET naming guidelines recommend avoiding type name conflicts (e.g.: 2 types with the same name in different namespaces that are often used together).

See the Namespaces and Type Name Conflicts section of the guidelines:

❌ DO NOT give the same name to types in namespaces within a single application model.

For example, do not add a type named Page to the System.Web.UI.Adapters namespace, because the System.Web.UI namespace already contains a type named Page.

❌ DO NOT give types names that would conflict with any type in the Core namespaces.

For example, never use Stream as a type name. It would conflict with System.IO.Stream, a very commonly used type.

❌ DO NOT assign type names that would conflict with other types within a single technology.

❌ DO NOT introduce type name conflicts between types in technology namespaces and an application model namespace (unless the technology is not intended to be used with the application model).

Since the System namespace is part of the Core namespaces, we should avoid names that conflict with names of types defined in that namespaces (e.g.: System.Object, System.Array, etc.)

We should also avoid names of types in namespace that are often used together with the Godot namespace. I don't think the System.Drawing namespace is often used together with the Godot namespace so we should be safe there.

For the System.Numerics namespace, I think it's a bit more complicated. I can see use cases for using the namespace in a Godot game to take advantage of some of their types, in order to use hardware acceleration for some math operations, but I would say these operations would be in some utilities file that would convert the types and most of the game code would not mix vector types. So I think we're still safe in this regard.

The only types that I agree should probably also be renamed are Godot.Collections.Array and Godot.Collections.Dictionary since they conflict with names of types in Core namespaces and they are very likely to be used together. In fact, I would say these conflicts are more common than GodotObject because I don't think most users would reference the GodotObject type directly.


About the Godot prefix, I don't think we should prefix every class with Godot but for GodotObject it seems to me like the best choice. GObject and GDObject don't really follow the guidelines.

See the .NET General Naming guidelines:

❌ DO NOT use abbreviations or contractions as part of identifier names.

For example, use GetWindow rather than GetWin.

❌ DO NOT use any acronyms that are not widely accepted, and even if they are, only when necessary.

Therefore, I think since the type represents a Godot Object and not any other kind of Object, it makes sense to me to call it GodotObject. If we were to use acronyms it would likely have to be GDObject since there's already precedent in the GD class, but again the guidelines recommend doing this only when necessary and I personally agree with @neikeq that 3 capital letters look ugly.

@Zhersi
Copy link

Zhersi commented Jan 31, 2023

Thanks for clarifying @raulsntos!

@jasonswearingen
Copy link

my bad... sorry I was wrong :( (deleted old post) System Numerics uses Pascal fields too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to build Godot with C# support and Godex PascalCase naming inconsistencies with C#
8 participants