I think it would be nice to do this for more GUI nodes. But it'd also be good to have a convention for it. The convention for hidden editor-used metatags is two underscores __, so I would use that for these internal nodes too.

do we want to have some kind of naming convention for internal nodes like starting with an underscore?

• See also Add naming convention for engine internal nodes and metadata godot-proposals#4550.

To me, this looks good. But I suggest getting some more opinions on this.
Another question we need to decide on is whether we guarantee compatibility regarding the name of internal nodes and to which extend we want to document these.

IMO internal children should never be accessed by the user. If there is something that is commonly needed to be accessed, we should add a function that returns this node (e.g. get_minimap()).
We can't guarantee that the node's internal structure won't ever change. Names are nice for some corner cases to make it convenient for the user, but it might give some false impression that they can be relied upon.

@KoBeWi For some like the scrollbars I guess not, but we expect users to be able to access the children of the ZoomHBox node, so it makes sense to give those readable unique names. For example GraphEdit does not have an API for accessing the zoom out, zoom in, etc buttons. I don't think it's worth adding many methods to access these when a unique name will work just fine.

For my use case I want to hide the snap to grid button. It's fine if we can't fully guarantee compatibility with these names, but at least hiding get_node(^"_SnapButton") and get_node(^"_SnapAmount") is vastly more readable and more stable than hiding get_child(4) and get_child(5). Even if the names change, the intent is still very readable. Otherwise I would have to leave a comment, which is worse than self-documenting code.

I'm fine with giving names, but it should not give any expectations. These nodes are internal, hidden from the user and can be renamed or removed at any point.

we expect users to be able to access the children of the ZoomHBox node

Not really, they are just accessible due to how node API works.

it makes sense to give those readable unique names. For example GraphEdit does not have an API for accessing the zoom out, zoom in, etc buttons. I don't think it's worth adding many methods to access these when a unique name will work just fine.

It can be exposed with one method and an enum, e.g. get_internal_button(ZOOM_IN), which guarantees that the button exist, regardless of its name and position in tree. Names of internal nodes are a nice convenience for doing stuff that isn't intended and you are using them at your own risk.

I'm not super psyched about this.

Giving them readable names to make it easier for developers to parse the tree and what it's doing sounds great, but doing it so that users can access what is supposed to be internal implementation details sounds bad to me.

If we find that there are important features missing from GraphEdit's public API, adding them via new methods should be the preferred route.

Sorry for digressing a bit from the actual question, but to be honest, in terms of code structure/API quality the menu bar (zoom hbox) shouldn't be part of GraphEdit at all, but rather implemented by the user IMO. It's basically just a composition of a HBoxContainer and flat Buttons and a SpinBox, which uses GraphEdit's public API. What do you think? (I'm asking this because it's compat breaking and should be included in the refactor PR if desired)

IMO internal children should never be accessed by the user. If there is something that is commonly needed to be accessed, we should add a function that returns this node (e.g. get_minimap()).

I agree with this position and I don't think we should name the internal nodes. As no matter how much we try to avoid possible conflicts, we can't be certain. And we don't want the engine to get in the way of the developer, cause conflicts when using find_child methods, or lead to weird situations where users would try to add a child of the same name to the node, but it would fail with no visible reason.

I think this change should be rejected.

@and-rad @YuriSizov Not including human-readable unique names leads to 2 things:

• If the user wants to touch the nodes, they have to do so by index, which is more brittle and less readable.
• If the user is never supposed to touch the nodes, they are forced to keep the GraphEdit's default controls. I don't think this is reasonable to do, I think the user should be able to customize GraphEdit as they wish.

At most I can see @Geometror's suggestion of removing these nodes from GraphEdit, but that seems like a separate issue. I don't like it when incremental progress is blocked in favor of waiting (potentially years) for a refactor.

The users who want full control over a complex node such as GraphEdit will ultimately need to touch the internal nodes, and they probably know what the risks are (in addition, the docs should warn users of touching internal nodes directly). I agree that this "hacky" way of interacting with nodes should be discouraged, but for those who have to, it's much nicer to work with proper names. Of course we should minimize the need to do so (e.g. by removing the menu of GraphEdit), but we can't provide helper methods (which basically return just a pointer) for every internal node.

If the user is never supposed to touch the nodes, they are forced to keep the GraphEdit's default controls. I don't think this is reasonable to do, I think the user should be able to customize GraphEdit as they wish.

Is this a common use case though? I imagine GraphEdit is mostly used for editor tools and tool plugins. When would it be desirable to remove these controls and in favor of what instead? Wouldn't it be sufficient to have methods that hide the parts of GraphEdit that you don't think you need, like the controls panel and the mini map?

This all sounds like stuff that should have been discussed prior to creating the PR.

• If the user wants to touch the nodes, they have to do so by index, which is more brittle and less readable.
• If the user is never supposed to touch the nodes, they are forced to keep the GraphEdit's default controls. I don't think this is reasonable to do, I think the user should be able to customize GraphEdit as they wish.

Yes, there are these limitations, but it's okay that we have them. The main purpose of these compound controls is to provide something that is ready to use. Customizability is nice, but if we want to have sane API, we have to limit it to a controlled subset of features. Which is why providing methods to access reasonably useful nodes (like the top hbox) is a fine compromise, and assuming someone wants to refine every bit of the control is not so reasonable.

I've already listed several issues with giving hidden nodes names, issues which affect users who are not going to modify the control, which is the majority of them. KoBeWi also made a point that even if we were to name the nodes, this comes with no guarantees. That is because the internal structure is not a part of the "official" API, it's an implementation detail. Making false signals that the control is stable enough to have node names that you can potentially rely upon is a bad idea. And that's exactly what the names would suggest, otherwise why would we even name them?

And and-rad makes an excellent point that there doesn't seem to be any demand for this change. There are some upvotes on the PR itself, but no prior discussion and no consensus regarding going this way to address any limitation.

As a power user I agree that sometimes you may want to go beyond the official and stable API, and Godot is extremely hackable due to its architecture. But being hackable is exactly that, you rely on something that has no guarantees to work, and using indices and node types is fine for that. As long as this doesn't affect those who don't want to do any of that, it's fine. Your suggestion, unfortunately, does affect regular users who have no desire to hack and deeply customize GraphEdit.

If there was a strong demand for a similar change, then I agree with KoBeWi's suggestion of exposing some generic API for accessing internal nodes using an enum of keyed values. But so far I don't think that even that is warranted. And if you need that level of customization, you can probably make your own GraphEdit, it's not that hard given there is already an implementation to reference.

As long as this doesn't affect those who don't want to do any of that, it's fine. Your suggestion, unfortunately, does affect regular users who have no desire to hack and deeply customize GraphEdit.

I fully agree with you on that statement, but this PR (or naming internal nodes in general) does not affect regular users at all, or am I missing something? It doesn't break anything, has no performance penalty (aside from constructing another String, but that's negligible), it just makes things a little bit nicer under the hood.

Besides that, having human-readable names for internal (or even editor) nodes might help us with debugging. (Personally, I had a few situations during the GraphEdit refactor where descriptive names would have been very useful.) This might be true even for some users, because it really helps understanding the internal structure.

I haven't benchmarked it but it's possible that this is actually faster because otherwise Godot will generate the @@ name at runtime as a placeholder for the otherwise-unnamed nodes, but these names are known at compile time.

but this PR (or naming internal nodes in general) does not affect regular users at all, or am I missing something?

@Geometror

It affects looking up nodes by name (find_child/find_children) and it affects adding nodes as children of GraphEdit (or any other such node). In both cases name conflicts are possible, but the user won't be able to see a clear reason why they get wrong results or why they can't add a node called _TopLayer to GraphEdit.

It's not that far-fetched in my opinion, and without using some absurd naming scheme we will always risk creating such implicit conflicts.

And my other point was that naming nodes signals some form of stability to the user, which we cannot guarantee and should not be limited by. But seeing names, users may start to rely on them more, which will inevitably lead to problems. It's the same as exposing an internal method to the public. You become bound by it, and can't really do anything without breaking compatibility.

Also, what about this node? If we were to remove the names, there's also this internal node which already has a name in master. Same with the scroll bar nodes. If we don't desire these nodes to have names, I guess we should remove the names of these nodes too for consistency.

@aaronfranke

Yes, I believe we should remove those names, because we do not use them consistently, we don't have a naming scheme for them, and they only slightly improve debugging (but not really, structure is already very simple for these nodes).

It affects looking up nodes by name (find_child/find_children)

find_child() has owned argument, which is true by default. It makes such nodes ignored.

it affects adding nodes as children of GraphEdit

Maybe we could set names that include @? There is _set_name_nocheck() that allows it; it can be made public. It's also much faster than normal set name. (it's unsafe tho)

or why they can't add a node called _TopLayer to GraphEdit.

To be honest this seems particularly far-fetched to me, I don't think this is a reasonable worry to have.

find_child() has owned argument, which is true by default. It makes such nodes ignored.

Yes, but this is not really a solution, as owned is not the same as internal, and you often have a legitimate need to fetch nodes without an owner that you add at runtime.

To be honest this seems particularly far-fetched to me, I don't think this is a reasonable worry to have.

From what I've seen, starting names with the underscore is not the most ridiculous case out there 🙃 And since we want to use sensible, human-readable names, this increases changes for a conflict. Besides, your PR argues for a principle that should apply to every compound node, so even if you don't see this example as being reasonable, I'm sure you can find some other node with a more clear potential naming conflict.

@YuriSizov If there is a concern with the _ prefix convention, we could change it to a double underscore, or _Internal, or _GodotGraphEditInternal, whatever it takes to make it unique and not conflict.

The documentation suggests how to interact with "any of its children".

So, if this is not an endorsed use case, this needs to be removed from the documentation. Or, as I am proposing with this PR, we make the node names unique so users can do this without accessing by index.

I recently noticed that ScrollContainer already sets names for its nodes: