Use human-readable unique names for GraphEdit's internal nodes

[aaronfranke]

Implements this proposal for GraphEdit godotengine/godot-proposals#7061

GraphEdit is a node in which user code is expected to be able to interact with its internal nodes (well, at least the Zoom HBox nodes). This PR gives the nodes readable names. The specific names are up for discussion, for example do we want to have some kind of naming convention for internal nodes like starting with an underscore?

EDIT: This screenshot is outdated.

[MewPurPur]

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.

[Geometror]

I find this sensible, more so with MewPurPur's suggestion of a naming convention for internal nodes. (what convention exactly is up for discussion, but personally I'd use one leading underscore since these internal nodes are unrelated to editor stuff)

[dalexeev]

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.

[Geometror]

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.

[KoBeWi]

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.

[aaronfranke]

@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.

[KoBeWi]

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.

[and-rad]

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.

[Geometror]

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)

[YuriSizov]

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.

[aaronfranke]

@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.

[Geometror]

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.

[and-rad]

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.

[YuriSizov]

• 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.

[Geometror]

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.

[aaronfranke]

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.

[aaronfranke]

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. Anyway, it should be changed regardless, _ConnectionsLayer is more readable than CLAYER.

[YuriSizov]

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).

[KoBeWi]

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)

[aaronfranke]

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.

[YuriSizov]

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.

[YuriSizov]

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.

[aaronfranke]

@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.

[dalexeev]

Note that there is already a precedent:

I also support the idea of a prefix, like _GD_ or _GodotInternal_.

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

[YuriSizov]

I suppose prefixing all of those names with _Internal_ should act as a natural deterrer, and it does lower the chance for conflicts. So in theory it would be workable.

But there is still no demonstrable demand for this from users, no linked issue or proposal, no defined use case. Your PR only covers it with a very questionable statement:

GraphEdit is a node in which user code is expected to be able to interact with its internal nodes

No, it's not. If it was, we'd have API exposed for that. You are not expected to interact with internal nodes anywhere in GUI code, unless there is clear and well-defined API for that. In GraphEdit expected interactions are limited to accessing and adding to the menu HBox, and adding GraphNodes.

Can you demonstrate why you need to access every node in the tree, and why existing or possible new API wouldn't be enough to cover your needs?

[aaronfranke]

no demonstrable demand for this from users

Well, I am interested in it, and I've heard many discussions in the past of this being desired. Before opening this PR it did not even occur to me that there would be people against it, naming nodes seemed like an obvious improvement that everyone wanted but it's hard to comprehensively do at once.

no linked issue or proposal

This proposal has received exclusively positive feedback: godotengine/godot-proposals#4550

Related: godotengine/godot-proposals#1018

Related: #29291

no defined use case

I want to hide the snap settings because in my situation we don't desire the user to configure snap. I would also like to hide the auto-layout button because it does nothing when clicked right now. I could also imagine other use cases such as wanting to hide the zoom buttons in a situation where the user is not supposed to zoom. There's also the fact that it would make the remote debugger scene tree more readable.

I really don't think any of these use cases is unreasonable and I also don't think it's worth bloating the API with a dozen method calls to access each of these internal nodes.

Can you demonstrate why you need to access every node in the tree

That's not a good argument. That's like asking, why add variables to GDScript, can you demonstrate why you would need access to every variable name? Just because the nodes would be available to access does not mean you have to access every single one of them. Even for the nodes you don't access, it's still useful to see them with readable names in the remote debugger next to the other named nodes.

[YuriSizov]

I want to hide the snap settings because in my situation we don't desire the user to configure snap. I would also like to hide the auto-layout button because it's does nothing when clicked right now. I could also imagine other use cases such as wanting to hide the zoom buttons in a situation where the user is not supposed to zoom.

These sound like good candidates for configuration properties. Especially snap and zoom, which are not just buttons in the GUI, but features that you would probably want to disable completely. I would agree with a PR adding flags to GraphEdit to disable those features. Internal node access is not required for that at all.

That's not a good argument. That's like asking, why add variables to GDScript, can you demonstrate why you would need access to every variable name?

To me, naming every node is inviting users to use them more often. And thus it's expanding the API surface. And you propose to expand it without consideration — to everything. That's like proposing to expose every property and method of a class to the API "just in case". We should act with more purpose, and we should have clear use cases for our decisions, especially such decisions that increase our API surface.

You've provided some examples. I like them, I support providing means for those situations. But I don't think we should do it the way you propose with this PR.

---

Before opening this PR it did not even occur to me that there would be people against it

If you look back at all the discussions we had about naming editor nodes, for example, you'll notice that one of the reasons against that was that we don't want to have a robust tree, which users will inevitably depend upon. We would just be making things harder on ourselves somewhere where a better designed API or indeed unstable hacks would be better (former provides more guarantees than a node name ever will, latter is explicitly unreliable and you use it on our own risk).

And as a result we still don't have many editor nodes named.

[saki7]

If you look back at all the discussions we had about naming editor nodes, for example, you'll notice that one of the reasons against that was that we don't want to have a robust tree, which users will inevitably depend upon.

Then why are nodes named automatically like "@@1"? Those bogus names and human-readable names conceptually look same to me, because they are both named. In practice, human-readable names look much better to me because they are readable.

Let's consider a realistic use-case for abusing bogus names: one might enumerate all child nodes and store them in Map<String, Control*>, while using get_name() for the key. The user might not even read the name, since it's solely used as the key for the map element. When the name silently changes, it might well introduce a bug. This kind of situation can happen regardless of whether the name is human-readable or not.

If we are to make the scene tree truly sane, I think someone needs to propose extra functionality like Node::set_user_defined_name(const String& name) and Node::has_user_defined_name(). Even so, I think this PR makes your life better, because the (potentially bug-prone) names become human-readable by default.

[KoBeWi]

Then why are nodes named automatically like "@@1"?

Because every node needs to be named, even if the name is not used. @ is used, because users can't use it for their node names, so there is no chance of conflict. Also these names are assigned automatically, based on initialization order. So you can't rely on them at all, because they can change at any time.

[saki7]

How about adopting a naming scheme like _ClassName@@1, _ClassName@@2, _ClassName@@3,... for internal nodes?
Or it could be _ArbitraryInternalName@@1, _ArbitraryInternalName@@2, _ArbitraryInternalName@@3,...

[AThousandShips]

_ArbitraryInternalName@@1 or even _ClassName@@1 isn't any more human readable than @@1, to be properly human readable it has to carry some information about what it does, and it shouldn't be any longer than it needs to be

[saki7]

Then how about @_ArbitraryInternalName? I think we can safely pick that name without clashing other names because we know what we're doing in internals, right?
I still can't see any technical reason why internal nodes can't named in a human-readable way. There should exist viable solution for this.

[AThousandShips]

It has already been expanded on above, and I agree, that giving nodes human readable names risk encouraging relying on them despite them not being exposed parts of the API and thus not considered for compatibility, so they can change at any time

I really don't see or agree that they need human readable names

But let's stop having this debate here, as it's only tangential to this PR, we are talking in very esoteric terms and not about this case really, if you like you can continue in some proposal or discussion relevant to it, like godotengine/godot-proposals#4550

[saki7]

I really don't see or agree that they need human readable names

I need human readable names because it's extremely hard to debugging a scene by looking at those paths like @@1/@@2/@@3/@@4.

Can't we just let users use it? It is their own responsibility for using internal names if they shoot themselves in the foot, no?

But let's stop having this debate here, if you like you can continue in some proposal or discussion relevant to it, [snip]

This PR should be the right place to talk because it's illustrating a solution for the real problem.

[aaronfranke]

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.