Allow node visibility to work with custom user-provided node types #86268
Conversation
There was a problem hiding this comment.
Not particularly against this change, but duck typing introduces some issues.
First of all, we can't be sure if the entire interface is implemented, so we may end up in a weird state where some parts of the API exist and do their thing, and others don't and the feature is not fully functional.
Second, these names are pretty generic. Right now the feature is only limited to a handful of classes which implement the methods and thus their descendants also implement them. This PR, for the most part, removes class checks, which means it may now affect custom classes which are not intended to be used with this feature but have methods with same names.
editor/gui/scene_tree_editor.cpp
Outdated
| if (p_node->has_signal("visibility_changed")) { | ||
| if (!p_node->is_connected("visibility_changed", callable_mp(this, &SceneTreeEditor::_node_visibility_changed))) { |
There was a problem hiding this comment.
If I combine those into one if statement, the line gets really long and overall harder to read. However, I have an even better way to improve this code that avoids constructing multiple Callables.
editor/gui/scene_tree_editor.cpp
Outdated
| p_node->connect("visibility_changed", callable_mp(this, &SceneTreeEditor::_node_visibility_changed).bind(p_node)); | ||
| } | ||
| } else if (p_node->is_class("Node3D")) { | ||
| if (p_node->is_class("CanvasItem")) { |
There was a problem hiding this comment.
Seems like it can be grouped with p_node->is_class("Node3D") to reduce the lines further. In theory, we can also just check meta itself, which would be on brand for this PR 🙃
There was a problem hiding this comment.
Yeah I agree. I was ignoring this before because it was outside of my use cases, but really, there is no strong reason to not just enable this for all node types.
To be clear: Whether a node can be locked or not is decided by the respective editor plugins (CanvasItemEditorPlugin or Node3DEditorPlugin). The code here in the scene tree editor only affects unlocking. It makes sense that if a user editor plugin locks a node using the same exact API as we already use for CanvasItem and Node3D, the scene tree editor should show it (and if the user does not want this, they should use a different metadata name than the one we already reserve in the editor for this purpose).
What parts of the interface am I missing that we should check for?
Indeed, visible is a fairly generic word that users could potentially use for other things. If users are using visible in their own projects for some other meaning they should probably pick a different name. However, really, showing an eyeball in the scene hierarchy is... just not a problem, it doesn't break their projects etc. Users will likely be happy to see it there, and if they don't want it there, they can just rename their method to something other than |
aaronfranke
force-pushed
the
scene-tree-visible
branch
from
af323ba to
32d425a
Compare
You aren't missing anything, but users might. They may implement some of the methods but not the signal or other methods. You don't check for everything all the time, which would be very verbose and unacceptable, but that also means that users may implement only parts of the logic and thus the entire thing won't function properly. And this is where my second point loops. They may have these methods or signals implemented for entirely different reasons with entirely unexpected side effects. Not to mention that discoverability of this feature would be very poor for people to even be aware of it and its repercussions. |
|
@YuriSizov The repercussions are that "an icon shows up in the editor" which "calls the method the user provides". I don't think this is likely to be a bad thing. As for discoverability, it seems pretty obvious to me, but we can write some documentation to explain this. Do you have an alternative suggestion for how to provide this feature to user code? |
Yes, an icon would show up that calls some method defined by the user, whether the developer wanted it or not. Which may by accident make something that should be treated with care to become trivially accessible. Without any indication to the developer that there is some feature their code would hook into.
It is absolutely not self-explanatory that adding some methods would enable an editor hook for a feature. There is no documentation, no UI hints, and it doesn't even follow the familiar pattern of "virtual" methods that need to be implemented consciously to enable a feature. Without reading the code, or finding this PR, you wouldn't even know that there is a feature that you can use, let alone how to use it. So documenting this would be an absolute must, especially if we go with the duck typing approach.
Well, that's a question for a proposal. There were a few which discussed the idea of providing way for users to add functionality/buttons to the scene dock. We should probably establish a pattern with how this functionality can be added, and decide how extensible it makes sense to make it. I'm not ready to offer a thought-out solution for something like this because we don't really have a good system for a feature that we want to be implementable by any class extending, say, Object, without bloating Object itself. What we did previously is exactly what you're doing. And the outcome of it was people didn't know that there is something that they can use as it was never documented, it was not a part of the class reference or any other reference material, and it was very easy to break because there was no API contract. For example, #43078. Again, I'm not completely against this change. But I do see some big problems with the idea. |
aaronfranke
force-pushed
the
scene-tree-visible
branch
from
32d425a to
0a1fb94
Compare
aaronfranke
force-pushed
the
scene-tree-visible
branch
from
0a1fb94 to
2c8efe5
Compare
We have StatusIndicator now, but it's missing
It results in errors when used in non-tool script. Harmless, but still. |
There was a problem hiding this comment.
The code itself looks fine, though not sure why that Callable change is here.
There are proper checks for all methods when adding the buttons, so the system can't break due to being implemented partially etc. (with the exception of non-tool scripts, but that's not really worth handling). I mentioned some classes that could be affected unintentionally, but it doesn't happen, because they don't implement full set of methods.
One potential improvement would be introducing some constants for these methods/signals, so they are not hard-coded like that.
aaronfranke
force-pushed
the
scene-tree-visible
branch
from
2c8efe5 to
0700b80
Compare
I split that into its own PR here #87778 but I hadn't rebased this PR since that was merged. I just rebased so now it's gone.
That actually might be a helpful warning to inform users that their custom node type should be a tool script in order to handle visibility. If they make the visibility code on a non-tool script, this way there is a relevant error message instead of the visibility system just not working in the editor. |
|
Thanks! |
|
This proposal can be closed: godotengine/godot-proposals#8812 |
In the current master, the scene tree editor's visibility system is hard-coded for a few types, namely these 4: CanvasItem, CanvasLayer, Node3D, and Window. However, this is a weird restriction that hard-codes a lot of types and requires a lot of calls to
p_node->is_class()instead of just checking if the node has what we need.This PR changes the code to be more generic, so now users can provide their own types that have visibility:
As a bonus, this code is a lot shorter now, since we don't have to repeat code so many times for each type.
In the current master, the only node types with
is_visiblemethods are CanvasItem, CanvasLayer, Node3D, and Window, so this PR does not affect the behavior of any engine-provided node types.