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

Use human-readable unique names for internal nodes for improved readability #7061

Open
aaronfranke opened this issue Jun 10, 2023 · 11 comments

Comments

@aaronfranke
Copy link
Member

aaronfranke commented Jun 10, 2023

Note: Related to #1018 but not the same since that is about the editor's nodes.

Describe the project you are working on

A game with a need to customize Godot's Control nodes, specifically GraphEdit in my case.

Describe the problem or limitation you are having in your project

Currently, if you want to customize one of Godot's complex GUI nodes, such as GraphEdit, your ability to do so is artificially hindered by the lack of names. For example take a look at GraphEdit:

Screenshot 2023-06-10 at 4 10 04 PM

Let's say that in your game you do not want to show the snap settings, because they are not needed in your use case. Or, another use case would be hiding the auto-layout button in case your implementation does not support that feature - it's better to hide the button than to show a button that does nothing.

How can you do this? Currently there is no clean way. From looking at the list of nodes, there is no way to tell which nodes do what. I would have to guess and check to find the indices, and use get_child(4) and get_child(5) to access the snap settings, but this will silently break if Godot ever changes the order of the nodes.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I propose to give these nodes unique names for improved readability:

Screenshot 2023-04-28 at 10 26 48 PM

This way, you can use get_node(^"_SnapButton") and get_node(^"_SnapAmount"), which is vastly more readable and more stable than using the index of the child node. This might even be slightly faster because Godot does not have to generate one of those @ names, instead the names are known at compile time.

One objection to this would be that the Godot developers don't want users to access the internal nodes. That is why I have the node names prefixed with _, to indicate that they are "private" and accessing them is not recommended. However, some users (like me) will access the nodes anyway if they need to access the nodes.

Another objection would be that if Godot ever changes the names, the code will break. I don't think this is an issue, because at least it will break in a way that's noisy and the intent of the code will still be clear from reading the name. Having unstable names is vastly better than accessing via child index.

Yet another objection is that we could add helper methods to provide a "cleaner" API for accessing the nodes. However, I think this is silly - it's not worth bloating the engine with dozens of unnecessary methods that will be rarely used.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

godotengine/godot#76563 Use human-readable unique names for GraphEdit's internal nodes.

We may also want to do this for other GUI nodes with internal nodes - such as TextEdit, Tree, PopupMenu, ColorPicker, etc. However I am not an expert on those. I want to set a precedent that naming internal nodes is a good thing, but I personally only need GraphEdit's nodes named, so that is my focus.

If this enhancement will not be used often, can it be worked around with a few lines of script?

Accessing by index, like get_child(4) and get_child(5), is a work-around, but that's terrible.

Is there a reason why this should be core and not an add-on in the asset library?

This is about nodes that are already a part of the core engine.

@YuriSizov
Copy link
Contributor

Let's say that in your game you do not want to show the snap settings, because they are not needed in your use case. Or, another use case would be hiding the auto-layout button in case your implementation does not support that feature - it's better to hide the button than to show a button that does nothing.

As mentioned in the discussion for the PR the proper solution for this is to expose configuration options for snapping, layout, etc. You propose a very general solution (with several drawbacks) to a very specific problem. Configuration options would be the appropriate way to address the limitations you present and would be user friendly and easy to work with.

@dalexeev
Copy link
Member

There are concerns:

  1. possible name conflicts;
  2. by giving names to these nodes, we encourage access to a non-public API and give some guarantees.

To avoid this, I think the following is sufficient:

  1. Use some obvious prefix like _GD_ or _GodotInternal_ (see Add naming convention for engine internal nodes and metadata #4550).
  2. Document that these nodes are not part of the public API and are subject to change or removal without any notice in any release. Use at your own risk. Officially not recommended.

@YuriSizov
Copy link
Contributor

There are concerns

These are the drawbacks of the proposed implementation, yes. But the main concern for me right now is that this is not the correct solution to the presented problem. The limitation that is described is solved with an established approach of making it configurable in a traditional Godot way, with exposed properties. There is no reason to implement something that requires a way more involved approach for Godot users who seek to customize their GraphEdit nodes.

In fact, hiding the nodes is not even a full solution, as you probably want to disable the feature completely, at least in the examples presented. So you definitely need a configuration option for that, to disable shortcuts and whatnot on top of hiding the visual elements.

@CarpenterBlue
Copy link

I think both Yuri and Aaron are correct, there should be more configuration options but sometimes, you simply need to brute force your customizations and for that, having proper names would be better.

@AThousandShips
Copy link
Member

Adding properties and configuration directly to the class and continuing to treat the nodes as internal is in my opinion the most BC and generally safe method

For this I hold to the attitude that internal properties and components and nodes should be accessible, but hidden, I often hate how interfaces lock you out completely, and I feel a balance is the best:
Not making it too easy for people who don't know what they're doing messing things up, not locking ourselves to an exposed API, while still allowing people to do things at their own risk.

I'd say that if we want to allow direct access to things like scroll bars etc. we should offer a method for it, allowing us to not make any promises about the internal makeup, but providing a stable way of access, or preferably, adding a direct interface for just manipulating scroll indirectly, just as an example.

@SysError99
Copy link

I could see this to be extremely useful but at a same time this gives more risks for users that use weird name combinations. However, this might work if it's an option that users need to perform manually rather than an automated process.

@aaronfranke
Copy link
Member Author

@SysError99 What do you mean manually vs automated? get_node() won't call itself.

@SysError99
Copy link

SysError99 commented Jun 17, 2023

@aaronfranke What I meant is that naming internal node names should be an option for users to enable the readable names or just leave the task to Godot if not needed. Sometimes If users aren't aware that readable internal names may conflict with customised node names it may cause unexpected behaviours. E.g., I use underscore for all Controls that get displayed on top of others.

On the footnote, I personally think that we just make them available on the first place would be the best option (make them visible on the editor and make them customisable). This way we don't need to worry about internal conflicts and give users more control on internal nodes. The only downside is that it may not look nice on the node tree especially when users not wanting to mess around with them.

@akien-mga
Copy link
Member

While it doesn't implement this proposal, godotengine/godot#81582 claims to solve the problems identified here. Can you confirm if that's a satisfactory solution?

@aaronfranke
Copy link
Member Author

Actually, a few weeks ago I decided to just hide the entire menu bar Godot provides in favor of making a custom one, so I don't personally need this anymore.

I still think readable node names are good, but if you don't see the value in it, feel free to close this proposal.

@wyattbiker
Copy link

So many discussions around internal node accessibility. I am not sure where to post. But here it goes:

I have an example I would like fixes for. FileDialog is lacking some crucial customization abilities. For example the internal ConfirmDialogs are not modal and if the user clicks away (see create folder prompt), it disappears behind the window and cannot be recovered without stopping application. Also some field sizes for Paths are too small and hard to see. This occurs only when accessing File System as opposed to Resources.

image

I have resorted to a trickery I don't like it because it is not general enough. But works for this case. The function locates the top level dialog nodes and changes them to modal. So the user cannot click away now.

func set_dialogs_to_modal(this: FileDialog):
	var node_list=this.get_children(true)	
	for node in node_list:
		if node.is_class("ConfirmationDialog") or node.is_class("AcceptDialog"):
			node.always_on_top=true

Only reason this guaranteed to work is because I know the class names. But if you have a node like the Path name in screen shot not so easy. I know it's FileDialog/@VBoxContainer@10/@HBoxContainer@27/@LineEdit@22 and want to change the size lets say. No way to do it because the node @numbers keep changing on new instances.

I am not sure what the fix is to allow internal nodes to be accessed other than creating guaranteed unique names.

P.S. I did file a bug report for the modal issue. godotengine/godot#92848

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

No branches or pull requests

8 participants