Godot: Multiple C# classes hiding inherited members in GodotSharp

[redxdev]

Godot version:
3.0 14b230f35f6b2498ef1fe93d8947bcc6f54b9261

OS/device including version:
Windows 10 1709

Issue description:
When the GodotSharp solution is built for the first time, a number of warnings appear as some generated method names either hide C# object members or other Godot-specific members.

Full list of members:

• ARVRPositionalTracker.GetType() hides object.GetType()
• LightOccluder2D.LightMask hides CanvasItem.LightMask
• LineEdit.FocusMode hides Control.FocusMode
• Slider.FocusMode hides Control.FocusMode
• PathFollow2D.Rotate hides Node2D.Rotate(float)
• ArrayMesh.BlendShapeMode hides Mesh.BlendShapeMode
• ArrayMesh.ArrayFormat hides Mesh.ArrayFormat
• ArrayMesh.ArrayType hides Mesh.ArrayType

Method hiding may result in unexpected behavior in C# scripts.

It actually seems that some of the members listed above actually reimplement the members they are hiding _without any changes_ and as such work as expected, but they still emit a warning.

[neikeq]

Should use new to get rid of the warnings: https://github.com/godotengine/godot/blob/14b230f35f6b2498ef1fe93d8947bcc6f54b9261/modules/mono/editor/bindings_generator.cpp#L652-L653

These are fine, as long as they are actual members hiding inherited members in the Godot API, and not the result of renaming them to PascalCase that is causing it.

BTW, why do the messages in the screenshot have an error icon instead of warning?

[redxdev]

BTW, why do the messages in the screenshot have an error icon instead of warning?

Good question. I found this issue a while back and it was showing the warning icon but recently it changed to an error icon. I'll do some digging and figure out which commit it was that broke it and submit a separate issue.

These are fine, as long as they are actual members hiding inherited members in the Godot API, and not the result of renaming them to PascalCase that is causing it.

The ARVRPositionalTracker.GetType() method definitely needs to be changed, though, as it hides object.GetType() (that's System.Object, not Godot.Object) which is used for C# reflection. Using new would be a bad idea for that method.

[redxdev]

Made a separate issue for warnings appearing as errors #15768

[akien-mga]

Is this still relevant in the current master branch?

[aaronfranke]

Still relevant in master. Updated list of warnings:

• Object.ToString() hides object.ToString()
• CPUParticles.Flags hides GeometryInstance.Flags
• ARVRPositionalTracker.GetType() hides object.GetType()
• ArrayMesh.BlendShapeMode hides Mesh.BlendShapeMode
• ArrayMesh.ArrayFormat hides Mesh.ArrayFormat
• ArrayMesh.ArrayType hides Mesh.ArrayType
• PathFollow2D.Rotate hides Node2D.Rotate
• LightOccluder2D.LightMask hides CanvasItem.LightMask
• LineEdit.FocusMode hides Control.FocusMode
• Slider.FocusMode hides Control.FocusMode

[aaronfranke]

Most of these things can be renamed in core and would improve the consistency and/or readability of the API. See also: https://github.com/godotengine/godot/issues/16863. Here's a few proposals:

• LightOccluder2D has light_mask but it also has get_occluder_light_mask and set_occluder_light_mask, so this property would make more sense if it was called occluder_light_mask.

• CPUParticles has a Flags enum, but it also has get_particle_flag and set_particle_flag, so this enum would make more sense if it was called ParticleFlags.

• ARVRPositionalTracker has get_type, but it also has get_tracker_id, and get_type is super ambiguous, so it would make more sense if this was called get_tracker_type. I also noticed it has a method called get_name which also seems ambiguous, so how about get_tracker_name?

• PathFollow2D has rotate, which throws a warning due to Node2D's rotate(), and I don't think it's ideal to give a property and method the same name. Not 100% sure what to rename it to, so I mentioned it here: https://github.com/godotengine/godot/issues/33026#issuecomment-568894723

For the Mesh/ArrayMesh ones, the current code is really confusing:

• Mesh.BlendShapeMode is only used in ArrayMesh AFAICT, so why not move it there?

• ArrayMesh.ArrayType is literally the exact same as Mesh.ArrayType, why does it exist?

• ArrayMesh.ArrayFormat is a subset of Mesh.ArrayFormat, and it's a bitwise enum, so we could just use Mesh.ArrayFormat and ignore the higher order bits, but I guess it makes sense as-is.

I have a branch here which solves all of these problems. I don't think the first three require much discussion, they seem like very good improvements. For PathFollow2D, I renamed the bool to rotates for now, but I think this one requires some discussion. For ArrayMesh, I removed ArrayType and moved Mesh.BlendShapeMode to it, discussion would be nice. I'll submit a pull request after 3.2 is released.

For the remaining warnings, I'm not getting a warning about LineEdit.FocusMode and Slider.FocusMode anymore, and I'm getting a few warnings about editor code:

  ObjectType/ResourceFormatLoader.cs(12,52): warning CS1574: XML comment has cref attribute 'EditorImportPlugin' that could not be resolved [/home/franke/workspace/godot/modules/mono/glue/Managed/Generated/GodotSharp/GodotSharp.csproj]
  ObjectType/Spatial.cs(168,102): warning CS1574: XML comment has cref attribute 'EditorSpatialGizmo' that could not be resolved [/home/franke/workspace/godot/modules/mono/glue/Managed/Generated/GodotSharp/GodotSharp.csproj]
  ObjectType/EditorSpatialGizmoPlugin.cs(73,31): warning CS0108: 'EditorSpatialGizmoPlugin.GetName()' hides inherited member 'Resource.GetName()'. Use the new keyword if hiding was intended. [/home/franke/workspace/godot/modules/mono/glue/Managed/Generated/GodotSharpEditor/GodotSharpEditor.csproj]

[vnen]

• LightOccluder2D has light_mask but it also has get_occluder_light_mask and set_occluder_light_mask, so this property would make more sense if it was called occluder_light_mask

It's already the occluder, no need to have it on the name of class in the property. But honestly, if CanvasItem already has a light mask property, why does LightOccluder2D have a different one? I believe we can remove the specific property and just use the CanvasItem one.