Godot: [C#] Underscores in front of "lifecycle" methods

[levrik]

Not sure if this was brought up before already but I'm confused by the naming of the "lifecycle" methods (e.g. _Ready, _Process) in C#. They also have the underscore in front like GDScript.
So far as I understand why this was needed in GCScript I'm not sure if it's needed in C#. It feels kinda weird to me since we already have the override keyword in front.

[akien-mga]

CC @neikeq @Hinsbart

[neikeq]

I guess we could remove them. No idea if it may cause conflicts though, it's a matter of trying. It's also a simple task, changing it here should be enough: https://github.com/godotengine/godot/blob/75d0aeb0e9250db729ddba56570386f7c10084af/modules/mono/editor/bindings_generator.cpp#L1644

[NathanWarden]

Not that I mind the underscore, but it probably would be less weird to not have it :) If nobody has already taken this task I'll go ahead and tackle it if nobody else wants to do it.

[NathanWarden]

@neikeq There are a few naming conflicts for some of the method names. For instance, there's a "Get" method on Object, but then there's a virtual "_Get" method. So we end up getting a conflict when the underscore gets removed.

There are a number of solutions that could be used, but the first one that comes to mind is to replace the underscore with "On". So, "OnReady", "OnProcess", etc... I think this would be more consistent/expected compared to other API's.

I tried this and everything is working conflict free.

Ideas? Thoughts?

[neikeq]

We would need to hear the opinion of users about that. Personally, I don't have anything against it. Is there any name that doesn't sound right with the "On" prefix?

[RayKoopa]

OnXxx seems fine to me. Even though, at some .NET 1.0 time, it was typically used / meant to be used only for tiny helper methods which raise an event with given parameters (Windows Forms most prominently), many people overrode that in their sub classes to indeed simply execute their own code when the event was about to be raised (sometimes not even calling the base implementation so the event was never raised again in sub classes).

Slightly dumb example given you couldn't simply override Text here:

public class Button
{
    private string _text;

    public string Text
    {
        get => _text;
        set
        {
            string oldText = _text;
            _text = value;
            OnTextChanged(oldText, _text);
        }
    }

    public event TextChangedEventHandler TextChanged;

    protected virtual void OnTextChanged(string oldText, string newText)
    {
        TextChanged?.Invoke(this, new TextChangedEventArgs(oldText, newText);
    }
}
// And people typically did
public class FancyAssButton : Button
{
    private string _textToDraw;
    protected override void OnTextChanged(string oldText, string newText)
    {
        // Oops, didn't call base, but who cares, right? Right.
        _textToDraw = ConvertTo1337Speak(newText);
    }
}

So it has been abused over time to become exactly what we want it to be, a place to put custom code for handling events.

It's definitely better than the underscore which outright violates C# naming rules. Let's not be as bad as Unity in that matter (with it's ugly lowercase public instance members) :)

Plus I can't think of a better prefix otherwise, except HandleXxx, but it's lengthy and sounds dumb.

[NathanWarden]

They seem to work, but here's a full list if anyone wants to chime in:

AStar.OnComputeCost
AStar.OnEstimateCost
BaseButton.OnPressed
BaseButton.OnToggled
CanvasItem.OnDraw
CollisionObject.OnInputEvent
CollisionObject2D.OnInputEvent
Control.OnGetMinimumSize
Control.OnGuiInput
EditorExportPlugin.OnExportBegin
EditorExportPlugin.OnExportFile
EditorResourceConversionPlugin.OnConvert
EditorResourceConversionPlugin.OnConvertsTo
EditorSceneImporter.OnGetExtensions
EditorSceneImporter.OnGetImportFlags
EditorSceneImporter.OnImportAnimation
EditorSceneImporter.OnImportScene
EditorScript.OnRun
MainLoop.OnDropFiles
MainLoop.OnFinalize
MainLoop.OnIdle
MainLoop.OnInitialize
MainLoop.OnInputEvent
MainLoop.OnInputText
MainLoop.OnIteration
Node.OnEnterTree
Node.OnExitTree
Node.OnInput
Node.OnPhysicsProcess
Node.OnProcess
Node.OnReady
Node.OnUnhandledInput
Node.OnUnhandledKeyInput
Object.OnGet
Object.OnGetPropertyList
Object.OnInit
Object.OnNotification
Object.OnSet
RigidBody.OnIntegrateForces
RigidBody2D.OnIntegrateForces
TileSet.OnForwardSubtileSelection
TileSet.OnIsTileBound
VisualScriptCustomNode.OnGetCaption
VisualScriptCustomNode.OnGetCategory
VisualScriptCustomNode.OnGetInputValuePortCount
VisualScriptCustomNode.OnGetInputValuePortName
VisualScriptCustomNode.OnGetInputValuePortType
VisualScriptCustomNode.OnGetOutputSequencePortCount
VisualScriptCustomNode.OnGetOutputSequencePortText
VisualScriptCustomNode.OnGetOutputValuePortCount
VisualScriptCustomNode.OnGetOutputValuePortName
VisualScriptCustomNode.OnGetOutputValuePortType
VisualScriptCustomNode.OnGetText
VisualScriptCustomNode.OnGetWorkingMemorySize
VisualScriptCustomNode.OnHasInputSequencePort
VisualScriptCustomNode.OnStep
VisualScriptSubCall.OnSubcall

[levrik]

@NathanWarden @RayKoopa The only methods where the On prefix sounds a bit odd are Process and PhysicsProcess. They are no event-like methods.

Edit: I think same applies to MainLoop.OnIdle but I'm not sure what this method actually does.

[RayKoopa]

Sounds totally fine to me. OnIsTileBound is what sounds horrible to me.

[levrik]

@RayKoopa Uh. Didn't saw that one.

[NathanWarden]

@RayKoopa Yeah, haha, that one is a bit odd :) I'm almost certain we could make a blacklist of exceptions for this.

@levrik I agree those sounded slightly odd to me too until I thought about what was really going on. IE. they are really about the event of the frame or physics being finished and here's how long they took (delta). I'm fine with the names, but for sake of clarity of what I'm saying, is that they could have been more accurately called OnFrameProcessed or OnPhysicsProcessed.

@levrik @RayKoopa @neikeq Since we'll most likely need a blacklist of some methods, what do you guys think about Process/PhysicsProcess having "On". I'm casting my vote for having "On" for them since they are technically events, but am also not dogmatic about that either.

Finally, @neikeq if a blacklist sounds good, do you have a preference where this should be and data type? The most obvious would just be sticking it right in the function as an array/hashtable.

[akien-mga]

I'm fine with the names, but for sake of clarity of what I'm saying, is that they could have been more accurately called OnFrameProcessed or OnPhysicsProcessed.

No blacklist please. You can do what you want to make the C# bindings more idiomatic, but it should all be automated conversion that anyone can perform in their heads to go from GDScript to C# and back when familiar with both languages. Renaming _process to OnFrameProcessed means effectively to change the Godot API and break compatibility with other languages. If you think that the Godot callback should be _frame_processed, that's another discussion separate from Mono bindings.

The aim is not to be like "ah, this Godot API is awkward, let's hack the Mono bindings so that it's less awkward". If an API is awkward, it should be fixed for everyone or noone.

[levrik]

@akien-mga I don't think @NathanWarden proposed to change that in the C# bindings only.

[RayKoopa]

I agree on not having a blacklist, that makes everything needlessly complicated. If the prefix is bad, we need another one. If you feel crazy, even something like GDReady, GDIsCrankyPants :/.
Best thing would still be to have no prefix at all. I haven't seen all the conflicting names yet. With the example given above of Get and _Get, having two methods Get and OnGet doesn't sound much wiser too. Maybe (if possible?) one can be renamed completely to be more specific anyway, like GetProperty, but then again I haven't even used the C# bindings yet so I'm not sure if that's wise aswell.

[NathanWarden]

@akien-mga What @levrik said is what I meant. It wasn't a name change proposal. I was only using the name OnFrameProcessed as an example to show that the Process function is actually an event and that it could have just as easily been named OnFrameProcessed, therefore calling it OnProcess still makes sense because it is actually an event of the frame having been processed despite OnProcess not sounding like an event.

As far as IsTileBound... is this even a proper name for this function? It looks like it should actually be called TileIsBound or simply TileBound because this is the method signature. Notice there are no booleans despite it's name implying them. @akien-mga Now this time, haha, I'm possibly proposing a name change :D

void IsTileBound(int drawnId, int neighborId)

I think it's misnamed? Can somebody who's familiar with TileSet chime in? There's no documentation for it and I don't know what this method does :)

[akien-mga]

void IsTileBound(int drawnId, int neighborId)

That would be a bug in the method binding, it's a virtual method and it is supposed to return a boolean (well actually a Variant):

• https://github.com/godotengine/godot/blob/master/scene/resources/tile_set.cpp#L845-L858
• https://github.com/godotengine/godot/blob/master/scene/resources/tile_set.cpp#L935 <-- Bug is here, @MarianoGnu forgot to specify the return type

[akien-mga]

The wrong is_tile_bound return value should be fixed in #14505, currently building to generate the mono glue and confirm.

[NathanWarden]

Ah, okay, thanks for clearing that up :)

So, just to be clear, the blacklist I was talking about would be to not put "On" in front of certain methods like IsTileBound. The rest would be OnXxx, IE. OnProcess, OnPhysicsProcess, OnReady, but then you'd have IsTileBound instead of OnIsTileBound.

Are you fine with that, or do you think that will be problematic? Out of about 40 or 50 methods we literally have 1 that On doesn't make sense.

I'm not sure there's really any other prefix to put that would make sense on all methods. Putting GD would work, but I think that's more strange than just having an underscore. Like I said in my initial post, I'm not against keeping the underscore, but just that I agree that it's a little strange to have.

[RayKoopa]

IMHO no blacklist, not even for a single method. A developer would wonder why all other methods handling Godot stuff are prefixed with On, but that one is not. Even if the name is weird like OnIsCreepyMethod, that name is still easier to understand than this arbitrary "hacky" exception.

[akien-mga]

Are you fine with that, or do you think that will be problematic? Out of about 40 or 50 methods we literally have 1 that On doesn't make sense.

No, there are a lot more than that. Any virtual method bound to scripts that returns non-void is basically not an event. See e.g.:

AStar.OnComputeCost
AStar.OnEstimateCost

Those are not events, they're methods you can override to return a float which is used in AStar's algorithm. You can define your own cost estimation and computation logic using those methods (in GDScript, _compute_cost and _estimate_cost.

So that's to see with @neikeq who knows this stuff better, but it might be worth separating the bulk of BIND_METHODV stuff between the real events/callbacks, and the Node-specific methods you can override to customize behaviour like _is_tile_bound, _compute_cost or drag_data.

As @RayKoopa says, there should not be any blacklist/no exceptions. If the virtual methods have different natures, they should be better separated at the engine level, not in the bindings for each language.

[NathanWarden]

Yep, I agree. Then, I think it would be best to just leave it as underscore? :/ (edit: It does keep it consistent with GDScript, and therefore the documentation as at least one positive thing)

HandleXxx would probably be fine, but as @RayKoopa mentioned it makes it more lengthy and doesn't sound very good.

But, I'll be out for about half the day. So if you guys can think of something better, when I get back I can just drop it in and I can generate a new list that I can post so we can see if it works :)

[vnen]

I don't think _is_tile_bound should be prefixed anyway. Nor the ones in VisualScriptCustomNode. For instance, the virtual methods on EditorPlugin are not prefixed.

I think this is a good opportunity to review them and break compatibility if need be.

[akien-mga]

I think this is a good opportunity to review them and break compatibility if need be.

I agree, but it should be discussed in its own issue :)

[Zylann]

OnGet/OnSet are weird to me. Or in general, I always saw On* methods as event/signal handlers, and as such, methods that don't return a value and that are called by a notification/event system...

[Zylann]

Also, underscore in front of methods often means that the method is not public, which is specified already in C# (and not in GDScript), so maybe removing the undersore and making the method protected would be enough.

[neikeq]

I think we should just keep the underscore, it doesn't look bad to me and makes it more familiar for existing Godot users. That being said, we should ask users about this to have a better idea of what they think (probably in the facebook group where most people is).
I agree with @akien-mga. I'm against a black-list. The less bindings we have to maintain the better, so it should be automatic. I prefer OnIsTileBound (which sounds bad), to a black list.
@Zylann That's the first thing that @NathanWarden tried, but there are conflicts with other methods when removing the underscore.

[NathanWarden]

@Zylann I definitely would have liked just the method names too, but that won't work without changing several Godot method names, which would also affect GDScript, which means we'll need to get more people involved than just those of us interested in C#.

I agree with @neikeq. I think we should just keep the underscore. It isn't ideal, but it really works just fine. I personally never even noticed a problem with it until it was brought up, haha. But, maybe that was just from using GDScript with Godot up to that point. :)

Plus, we could always revisit this in the future. IE. If enough people want it changed we could change it in something like Godot 4 (or a Godot 3 point release if it makes sense). I think it'll be far more obvious how we should proceed after Godot 3 is released whether this is an issue for the broader community (Facebook/Twitter/etc) or not.

[mhilbrunner]

I agree with neikeq. Maybe not the most idiomatic to have the underscores, but less mental overhead. I don't particularly enjoy having to keep a mental model of "oh, its OnProcess() in C#, _process() in GDScrip and Process() in Python".

In some cases it makes sense, but especially when you start to introduce talk about blacklisting / special casing etc... nah. I'd prefer to use the GDScript names and keep it as _process.

[abgenullt]

Please, please remove the underscore.
Look here http://www.mono-project.com/community/contributing/coding-guidelines/ and here https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/naming-guidelines

In both frameworks underscores are not recommended and common.
Please, make the C# support like a C# programmer would use it, not a python programmer.

The "Process" method is overridden, so the name should be just "override void Process". Only for events, the name should starts with "On..."

[Zylann]

@abgenullt

I definitely would have liked just the method names too, but that won't work without changing several Godot method names, which would also affect GDScript, which means we'll need to get more people involved than just those of us interested in C#.

I don't think C# should end up modifying GDScript in any way, but:

@neikeq There are a few naming conflicts for some of the method names. For instance, there's a "Get" method on Object, but then there's a virtual "_Get" method. So we end up getting a conflict when the underscore gets removed.

The reason why this conflicts is that _ means two different things in this situation, which is tied to engine implementation not using virtuality for that, luckily not a problem in Python. Changing this would need to add a special case for this method (like InternalGet or CustomGet) but that renaming would then affect other languages because there is no straightforward way of doing it automatically when C# bindings are generated, hence the "blacklist" that some dislike. Maybe a check for two script-accessible versions can detect it, but could miss the real semantic if there are other methods that do this for different reasons (but I dunno, I haven't checked every binding).

[MarianoGnu]

if you replace all the _ for "On" that would automatically result on "Get" and "OnGet", confusing but still valid and no need for an exeption, anyway i'm not a C# user, my opinion has no weight

[Zylann]

@MarianoGnu it conflicts because _ means multiple things in Godot. It almost always mean the method is either protected or private, and sometimes it's also for handling an event, but not always. Hence why prefixing On everywhere could be overkill imo... it doesn't look that bad though, given @RayKoopa 's list, seems coherent most of the time... only a few ones seem to be off, like OnGet, OnIsTileBound or OnGetMinimumSize (well, getters :p)

[NathanWarden]

Maybe if I get some time later today I'll get the list of methods that conflict. It seemed to be pretty small. Maybe we can work through those so that we can remove the underscore.

[abgenullt]

The bindings for C# and for any other language should be independent in the coding form. If not, you will never get a clean language-specific binding. So the discussion is how to handle this problem generally.

[RiverMesa]

I myself would be in favor of using the On prefix too - it does look a little funny on some of the methods, but I think that's preferable to outright violating a language naming convention or using something as elaborate/complex as a blacklist.

[levrik]

I'm not really against something like a blacklist. I think we shouldn't violate the language naming conventions and also doesn't create weird new namings within the naming conventions (e.g. OnGet, OnIsTileBound, ...). I think it's kinda clear which ones are events and would get a On prefix. Besides that we have auto complete and documentation that will help discover these. I also don't think we should make the difference between GDScript and C# bindings as minimal as possible. I don't think many people will use C# and GDScript in the same project.

[MarianoGnu]

if i'm correct, methods binded with an _ will be binded but not exposed to GdScript (they are used only for connecting signals internally) it's a use case, not a convenion. Virtual methods use an _ in the front, these are the ones who should/could be worked on.
As i said i'm not a C# user, when times comes i'll use whatever is implemented, and in the poll about what to do with Function Names, i voted "keep it the same as gdscript", that's all i can share as opinion

[Zylann]

@MarianoGnu all this still applies if we ignore all "internal" bindings (@NathanWarden though it may reduce the list you made? Was it taken into account?)

[neikeq]

The On prefix is generally a convention for methods that fire an event. It also requires the user to mentally map underscored methods to this style, and it looks out of place in some cases. I wouldn't use it.

I would go with one of the following:

• Leave the underscore as is. Seriously, we don't need to follow other conventions strictly, specially for something as trivial as this. We already had many conflicts when choosing PascalCase over the engine's snake_case. If you compare the two styles linked by @abgenullt, you will notice they are completely different.
• Remove the prefix (_Process --> Process). This would be the ideal naming, but causes conflicts with methods like _get and get. One possibility would be to just keep the underscore in the cases where removing it causes conflicts (this would be automatic, no black-list).

[MarianoGnu]

One possibility would be to just keep the underscore in the cases where removing it causes conflicts (this would be automatic, no black-list).

This might not possible, because you could remove an _ and this could cause a conflict with a class that inherits from this one, if oyu track the entire tree you could find that a class with a function _do_something() has a child class with both functions _do_something and do_something. Really guys? what's the problem with keeping the underscore? :/

[reduz]

If it's more consistent with the language, I'm fine with precluding those
functions with On. At this point, C# users clearly prefer coherent syntax
to something easy to translate to GDScript or other APIs.

On Fri, Dec 22, 2017 at 3:27 PM, Mariano Javier Suligoy <
[email protected]> wrote:

One possibility would be to just keep the underscore in the cases where
removing it causes conflicts (this would be automatic, no black-list).

This might not possible, because you could remove an _ and this could
cause a conflict with a class that inherits from this one, if oyu track the
entire tree you could find that a class with a function _do_something() has
a child class with both functions _do_something and do_something. Really
guys? what's the problem with keeping the underscore? :/

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/14466#issuecomment-353651802,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z26z6tQUt2R9W3o-eJK2bFkAIZFAkks5tC_SlgaJpZM4Q8NqO
.

[NathanWarden]

As promised, albeit a bit late, here are the errors I get when building without an underscore and without any prefix in front of the methods. I believe it's because the internal methods and the virtual methods have the same names. Sorry for it being an image...

[neikeq]

What do you think would be the best solution for these conflicts? Do we keep the underscore in those cases?

[NathanWarden]

I'd be fine with that.

[MarianoGnu]

Keeping the _ only on these cases would cause inconsistences on the API, forcing the C# users to remember when to use and when not to use the _
I'd recomend to replace it by something else, if not by the "On" then another prefix like "Gd", "Internal", "Virtual" or whatever

[MarianoGnu]

in fact is already difficult to me to understand when to use .get and when to use ._get U_U

[Zylann]

@MarianoGnu you never use ._get(), that's the point. You only ever write it to override the function, but in a non-virtual way because that's how it was implemented in Godot (although the use case is similar).

[NathanWarden]

Is it possible when there's a name conflict to just make the base method virtual and ignore the actual virtual methods?

For instance, we have Resource.SetupLocalToScene and Resource._SetupLocalToScene, which look like:

``` C#
[GodotMethod("_setup_local_to_scene")]
public virtual void _SetupLocalToScene()
{
return;
}

``` C#
        [GodotMethod("setup_local_to_scene")]
        public void SetupLocalToScene()
        {
            NativeCalls.godot_icall_0_11(method_bind_9, Object.GetPtr(this));
        }

Could we just have the original be a virtual method?

C# [GodotMethod("setup_local_to_scene")] public virtual void SetupLocalToScene() { NativeCalls.godot_icall_0_11(method_bind_9, Object.GetPtr(this)); }

The only one that seems to be an oddball is Get since the base Get returns an object and the virtual one doesn't return anything... which, as a side note, seems completely useless to me that it doesn't make it possible to manipulate the Get output. Edit: Meaning, making the base Get method virtual and ignoring the actual virtual one would actually make the function useful.

[MarianoGnu]

@Zylann i meant when to call it, not when to override it, and i mean both from gdscript and c++

[Zylann]

@MarianoGnu in GDScript, you never call it. In C++, I believe the use case is similar, like, _get is kind of an override that is unable to remove/replace internal properties... you can get both using get (properties of the object, AND those you may return in _get, makes no difference for the caller) maybe @reduz can clarify this

[aaronfranke]

I personally would prefer simpler names such as void Process() etc, if not that, then I think void _Process() is better than void OnProcess() since it's almost as simple as void Process() and matches the GDscript implementation.

[mhilbrunner]

As there doesn't seem to be much agreement, I suggest leaving it as is. C# is nearing being final at this point anyway, not sure if we should make changes to this at this point.

[jonbonazza]

Frankly, not adhering to language conventions is unprofessional and is not a good look for the engine. I'd argue that following language conventions trumps everything else in this thread.

Personally, I am not seeing the issue with just removing the understore and renaming the conflicts where approriate. It seems like the primary concern with this approach is that it would make transitioning from gdscript to c# difficult, but I honestly think that this is a non-issue. I doubt that many people are going to up and port their codebase. The target audience for c# should be people who actually program c# (that is, people who have prior experience in c#) and it seems that these people collectivly agree that the current approach is simply unacceptable.

Standard language conventions exist for a reason.

EDIT: also, it looks like all the conflucts are actually internal methods?

[mysticfall]

I largely agree with what @jonbonazza said above, though I can live with it if there's a sufficient reason why we should leave it as it is.

Maybe the important thing is, this could be our final chance to change this if it's not too late already. Currently, the C# support is considered at an alpha stage but once 3.1 is out, we wouldn't be able to do implement such changes that would break users' projects like this.

So, I'd like to see this issue to be sufficiently discussed before we decide to go either of the ways.

[vnen]

It seems like the primary concern with this approach is that it would make transitioning from gdscript to c# difficult,

No, the issue is not the transition between languages, but the need to repeat the Class API documentation with other naming convention. Though there's already a change in case convention, if there's the removal of underscores there must be a pattern to it. Also, the conversion should be automatic: if something is added to the ClassDB, it should reflect into the C# glue without specific changes.

The problem with simply removing is that are naming conflicts, in particular with virtual methods. But this should not be solved in case-by-case basis, some automated solution must be applied.

[jonbonazza]

@vnen I understand your concerns, but I really don't think that this is as large of a deal as you seem to think it is. In fact, this is actually fairly common for different languages to have different APIs in the same library. Also, as I mentioned before, unless I am misreading, it seems that the conflicts are all virtual methods and are never even used by client code. Is this true? If so, then sweating over such a detail seems a bit premature. Renaming these conflicts entirely isn't even something that would need documented, as they aren't even intended to be used, right? Hell, as a compromise, I think it's fair to say that leaving underscores in this case would be "alright."

[aaronfranke]

Functions such as _Process() are overridden in user scripts. Changing these names will require users to also change their scripts, though it is a very minor fix and an acceptable change.

[Zylann]

To me the issue we have is that we used to store properties such as "virtuality", "privateness" or "internalness" inside naming conventions such as _, which is ambiguous from the point of view of an automatic converter. In C# those "conventions" make little sense because the language has the tools to specify all those particularities in an explicit way. So to me, there is a pattern. But it isn't going from the existing API to C#, it is the other way around, information is missing to tell things apart.

[vnen]

it seems that the conflicts are all virtual methods and are never even used by client code. Is this true?

It is quite the opposite, since the concept of virtual methods is that they can be overridden in the game code. _ready and _process are used all the time. Of course, those don't conflict with anything, but others do, and where are you okay with losing the consistency (consider that you are reading the base API doc and need to the convert the name in your head).

I agree that the convention is not clear and it's not even applied consistently. TBH we could even remove the underscores even from the base API, but that would mean breaking compatibility, so for now it's not an option. Until, then there should be some middle ground.

In fact, this is actually fairly common for different languages to have different APIs in the same library.

I wonder if those different API are maintained by the same people. The problem is that it is more work to do.

The major concern is that any change in the bindings should be reflected, so any kind of process should be automated and consistent (i.e. it cannot change the name of an existing function because another function was added).

---

Another point is that pretty much every tutorial is written in GDScript, including code samples in documentation and demo projects. The idea of using the same API is that GDScript and C# users can talk to each other without much getting lost in translation. I know this is a "minor change", but we should not need to maintain two API sets to do the same thing.

[mysticfall]

I don't think GDScript compatibility could be a good reason to avoid this change. We already use a different name convention from GDScript, and if people can easily guess what would be an equivalent for C# when they look at some GDScript code by changing snake_case names to PascalCase ones, they can surely do so by dropping any underscore (and that's already included when you switch to PascalCase).

Tutorials and sample projects written in C# will certainly catch up, and pretty quickly at that if we'll have more C# users to come once its support stabilizes. But the naming convention for core methods is something that can't be changed so easily after that.

So, I don't think we should mitigate such a temporary problem of lacking C# documentation or sample code by permanently breaking naming convention for our core API.

[jonbonazza]

@vnen sorry, I seem to not have communicated as clearly as would have hoped.

So if we are to adhere by conventions of C#, then it's inevitable taht some changes will need to be made to the C# APIs. What I am suggestion is that we remove the '_' from _every_ public API. There were concerns about conflicts--that is, virtual methods, such as _Get which if you removed the underscore, would conflict with another method of the same name (in this example, Object.Get). From my understanding all of these cases where removing the underscore would conflict with other methods are internal methods and we could probably get away with leaving the underscore for those methods without issue (or ideally, naming them something completely different in C#), as no one would be using them anyway.

[mysticfall]

@jonbonazza By the way, virtual methods are meant to be overriden by users as explained by @vnen so it's a bit different from internal methods.

Probably, the ideal solution would be simply removing one of those conflicting methods in C#. For example, the source code for Object.Get and _Get looks like this (let's ignore the wrong return type of _Get for the moment) :
```c#
[GodotMethod("get")]
public object Get(string property)
{
return NativeCalls.godot_icall_1_297(method_bind_3, Object.GetPtr(this), property);
}

[GodotMethod("_get")]
public virtual void _Get(string property)
{
return;
}
`` API wise, there's absolutely no reason to have the both versions, since it'd be enough to declare the first one asvirtualand let users optionally override it. It's how C# API is supposed to be written so it'll also eliminate the need for people to learn that Godot specific convention which exists soley because GDScript doesn't have avirtual` keyword.

Technically though, I can see how it can be quite problematic to fix it. I haven't looked into how Godot handles such methods, but it's possible that it works by invoking _Get internally to see if it was overriden when its Get method is called. And it can even try to optimize the process by checking whether or not _Get is actually overriden. So, in that case we can't simply remove either of them.

In that case, it might make more sense if we can just rename all such 'virtual' methods as protected virtual T DoSomething(). For example, the above _Get method would be protected void DoGet() (again, let's forget about the incorrect return type).

[vnen]

Just to clarify: I'm not against removing the underscore to follow the language standard. My points is that it should be trivial for a user to convert them in their head when looking the class API, without requiring to remember some arbitrary changes.

Again, the problem is not internal methods. If they were internal, this wouldn't be a problem at all. All methods available for C# are public. The private internal methods aren't exposed to this (or at least they shouldn't).

I'm really not fond of having to blacklist things. But if there are only a few isolated cases and we can guarantee no new conflicts will happen (we probably can, but still), then it might be the solution. If that can be done automatically, great, but we still have to document this well enough to avoid confusion.

[jonbonazza]

Sorry, what I meant was the virtual methods that are in question of conflicting are also internal ones.

[neikeq]

I don't understand why this need to scrupulously and meticulously follow a naming convention. These methods are not from a framework wrote in C# from scratch, these are bindings to an existing code base that follows different conventions. Of course, we can try to make the bindings as natural as we can for the target language, but there will always be limits.

Now, the problem about underscores in virtual methods is that removing them causes conflicts with other methods. No, they are not internal methods, otherwise we wouldn't generate them. As said before, one option could be to keep the underscore in cases where there is a conflict and remove it everywhere else, but that would go against the rule of following naming conventions that motivated the change in the first place. It also adds more variables to keep in mind when mentally translating names. These names are not from GDScript. These are the names used by the Godot scripting API; sooner or later you have to deal with them, so difficulting the translation is not a good idea.

I have nothing against removing the underscore for virtual methods (except perhaps that they allow for quicker filtering of virtual methods during autocompletion), but we have to find a solution for the conflicts first.

[NathanWarden]

I'm all for removing the underscores, as many people on this thread already know I tried, but the cure for this seems to be more troublesome than the disease.

[aaronfranke]

Based on everything I know about this issue, that removing would be a pain and possibly have inconsistencies, for a minor style benefit, I vote to keep the underscores.

By the way, I personally prefer the underscores to On prefixes.

[jonbonazza]

What frustrates me is that people call it a "mild style benefit." The fact that very few people seem to understand the importance of conforming to standards and conventions (and standard conventions) is this most disappointing part of this whole thing.

[mysticfall]

If there's significant practical hurdles to fix this issue, I agree that we should leave it as it is. I'd like to see it fixed though but based on what I read in this thread, it seems it won't happen unless someone will take the job him/herself and makes a PR.

I believe the majority of us agree that it's better to fix it so our codebase could be more conforming to the standard coding convention by now. As such, if we need to discuss this matter further I guess it better be on the technical side, so we can hopefully find a way to overcome whatever obstacles for fixing this before 3.1, if it's feasible at all.

[aaronfranke]

@jonbonazza The standard which we are conforming to, in this case, is the Godot API.

[mysticfall]

@aaronfranke But in this case, it's a standard that is mostly pointless in the context of C# which also conflicts with the widely used conding conventions of the language, so I agree with what @jonbonazza said in principle.

And if we are to regard whatever coding convention that Godot API suggests, probably we shouldn't have tried to convert all snake_case names to PascalCase ones in the first place.

But as I said, I can see how fixing the issue involves some real work while there are other issues which better be fixed before 3.1. So I just hope someone who knows internals of how glue generation works to take some time to contribute.

[NathanWarden]

If anyone wants to give it a try they can reference my code here, although it's a bit outdated: https://github.com/NathanWarden/godot/tree/cs_virtual_method_rename

[neikeq]

Guys, the problem is not implementing it. I would gladly implement it myself if there was anything to implement. I explained what the problem was. If you want to see this done, you should try proposing a solution for the conflicts.

[mysticfall]

@neikeq I think I understand the problem, since what I meant by 'implementing' certainly includes finding a suitable way to resolve the Get vs _Get situation that I mentioned above.

Maybe we could get a more clear idea of the problem if you could explain the difficulties involved in that specific case.

So, I'd like to ask what would happen if we simply remove _Get and make Get as a virtual method instead? Would it break any existing functionalities? And, can we assume every other conflicts would be similar to this particular case, so the solution we may find for Get / _Get can be applied universally?

[neikeq]

What frustrates me is that people call it a "mild style benefit." The fact that very few people seem to understand the importance of conforming to standards and conventions (and standard conventions) is this most disappointing part of this whole thing.

Like I said previously, I really don't understand the need to not deviate one bit from a standard convention but, as explained above, that's not the reason this is not changed. Many changes were already made to please C# users, even though I may not agree with many of them.

[neikeq]

@NathanWarden shared an screenshot of the errors he gets when removing underscores in thi comment. Reading that comment, I see why there was the confusion above about these methods being internal.

The list of conflicts seems to be:

• Control
  
  
  
  • _GetMinimumSize with GetMinimumSize
  
  
• Object
  
  
  
  • _Get with Get
  
  
  • _GetPropertyList with GetPropertyList
  
  
  • _Set with Set
  
  
• BaseButton
  
  
  
  • _Pressed with Pressed
  
  
• Resource
  
  
  
  • _SetupLocalToScene with SetupLocalToScene
  
  
• MainLoop
  
  
  
  • _Idle with Idle
  
  
  • _InputEvent with InputEvent
  
  
  • _InputText with InputText
  
  
  • _Iteration with Iteration
  
  

At first glance, one would say we should have a single method. There is a problem in C# with sharing the same method for both get and _get though, see:

class Object:
    virtual func get(property): # wrapper function which calls the Engine's Object.get
        internal_engine_call_Object__get(property)

class MyScript extends Object:
    override func get(property):
        # custom code here
        return base.get(property)

Engine's Object.get is called, it doesn't find the property so it falls back to calling MyScript._get, which in this case would be MyScript.get. MyScript.get does its thing and calls the Object.get wrapper method, which then again calls Engine's Object.get which falls back to MyScript._get and so on... It's an infinite loop.

[mysticfall]

Thanks for the explanation. Then, it seems that the heart of the matter is that the native method is invoking its virtual counter part (as I also suspected in my comment above).

In that case, the only remedy I can think of is to use On- or Do- prefix for the virtual method.

However, even though using such a prefix is quite common, it's not a part of any conding standard that I know of. On the other hand, names of non-private methods are covered by the official coding guideline from MS, which clearly states they should follow PascalCase naming.

So, using something other than _ might have some benefit of making it more conforming to the standard coding convention.

But simply using either of those proposed alternatives for all cases probably would result in unnatural names. For example, while DoGet might be quite a common choice in such a situation, names like DoPressed won't make much sense.

Maybe it'd make more sense if we can extend the ClassDB format so that we can specify different names for a specific binding by hand (and possibly with other informations, like what additional interfaces it'd extend, and etc - i.e. #17791).

But if must rely on blind replacing of auto generated method names, I'm not sure if it'd be worth the hassle at this point. So I have to agree it'd be better to leave it as it is, even though I'd like to see Godot's API to be as much standard conforming as possible.

[nathanfranke]

I agree with @aaronfranke; C# methods should be consistent with GDScript methods. If it is stupid to have an underscore before each lifecycle method, what about renaming the GDScript methods also?

Unpopular opinion: How about we implement function overrides in GDScript and just use func ready() and void Ready()?

[mysticfall]

In my opinion, it would be better C# methods to be consistent with the official convention of the language rather with another language which follows different rules.

I agree that we shouldn't make it confusing for people who may switch between the two languages to use Godot. However, I don't think it'd be too difficult for them to see which GDScript method to correspond with a C# one, as long as there's a consistent rule (e.g. snake_case to PascalCase).

As to lifecycle methods, it seems that the only reason why GDScript adopted such a convention is that it does not have override keyword as C# does. So I don't see any reason why C# should follow such a foreign convention when it already diverges from GDScript in many other aspects when they seem to fit its standard better.

[Shadowblitz16]

@mysticfall
gdscript already isn't consistent.

for example signals are named ready while their implementations are named _ready.

I think we should follow the C# style guidelines and use On prefex for hooks and signal implementation.

an example would be void OnReady() {}.

Do doesn't really make sense and doesn't follow the conventional C# style guidelines.

their editor counter parts would remain without _ or On.

[neikeq]

Is the On prefix used in the C# style guidelines for hooks though? As far as I understand, it's actually used for the method that raises an event, e.g.:

``` C#
event Action TextChanged;

void OnTextChanged(string text) {
TextChanged?.Invoke(text);
}
```

[Shadowblitz16]

its used for the functions that implement the logic

[Flavelius]

In my opinion, On[X] makes sense because _ready etc are like engine events in a broader sense (which makes it a nice hint towards their intention too). And Regular methods are more likely to be imperatively named which means 'Ready()' Set() Go(), are free to be used by programmers. Plus regular event subscriptions are a little less likely to be overrides at the same time.

[neikeq]

its used for the functions that implement the logic

Any reference? From what I've seen, this is not the case, and the convention is to use it on methods that raise the event as in my example.

In my opinion, On[X] makes sense because _ready etc are like engine events in a broader sense

The motivation for getting rid of the underscore prefix is that it looks out of place when considering standard conventions, so it doesn't make sense to change it to something that goes against standard conventions as well.

[neikeq]

I'll give a try at removing the underscore soon and see what the resulting conflicts are. If the conflicts are very few and for unimportant methods then I think we can move forward with the change, only keeping the underscore when there are conflicts.

[JimArtificer]

Today the naming is internally consistent. Don't sacrifice that for a little syntactic sugar.