Add an `is_scene_root` method for Nodes

markdibarry commented 2 years ago

Describe the project you are working on

Basic sidescroller

Describe the problem or limitation you are having in your project

It's common to have scenes that aren't intended to run on their own without a parent, like a custom text box. In this example, when the scene is instanced with a parent, you wouldn't want it to have default text displayed; it should initialize blank, since the parent should determine it's behavior. However, debugging an individual scene can be difficult without having default text, so it'd be helpful if in the _ready method you had the ability to check to see if the node was the root node for the scene, and, if so, use some default text, since it's just for editing and debugging.

The built-in helper methods and properties return a different root or root-adjacent node based on the context. This took quite awhile and a lot of digging to discover the following layout:

While in the editor, the root node for the scene is get_tree().edited_scene_root.
While running in game, the root node for the scene is the the first child of get_tree().root.

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

is_scene_root() should return whether or not the node is the scene's root regardless of the context.

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

So, currently, the only way to do this is as follows (loosely translated from C#):

if Engine.is_editor_hint():
    return self == get_tree().edited_scene_root
else:
    return get_parent() == get_tree().root;

so perhaps it could just do this behind the scenes.

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

It can, but those few lines of script took an hour or so to figure out with plenty of trial and error and probably deserves a line of comment explaining why it's necessary due to the inconsistent results of the built-in methods. This would remove the confusion involved.

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

It's just an extension method.

h0lley commented 2 years ago

Here's what I do to detect a F6 launch: get_tree().current_scene.scene_file_path != ProjectSettings.get_setting("application/run/main_scene") in the _ready() of an autoload script. but what you propose certainly would make life easier.

dalexeev commented 2 years ago

However, debugging an individual scene can be difficult without having default text, so it'd be helpful if in the _ready method you had the ability to check to see if the node was the root node for the scene, and, if so, use some default text, since it's just for editing and debugging.

Here's what I do to detect a F6 launch

This is a little easier:

print(self == get_tree().current_scene)

And besides, default values can be set in the Inspector, in variable declarations, or in _init.

SilencedPerson commented 2 years ago

This doesn't really solve anything, you still have to code around it. I would prefer having an ability to specify a "test scene".

h0lley commented 2 years ago

print(self == get_tree().current_scene)

ah, this is pretty much the is_scene_root() then. what I posted is still what one would need to do when they want to know in an autoload script though.

so perhaps this can be unified by injecting a flag into F6 launches (and custom scene launches) that then can be tested for like OS.has_feature("scene_test")?

edit: actually this already kind of happens as F6 and Ctrl+Shift+F5 use a command line argument to pass the scene file to launch with. so with something like not OS.get_cmdline_args().empty(), one can check for a test scene launch anywhere. that check should probably always be paired with OS.is_debug_build().

markdibarry commented 2 years ago

@h0lley No. They were replying to your separate issue. print(self == get_tree().current_scene) doesn't work in the editor. Unless we've missed something, there is no property or method that returns a consistent scene root.

Rodeo-McCabe commented 2 years ago

@h0lley No. They were replying to your separate issue. print(self == get_tree().current_scene) doesn't work in the editor. Unless we've missed something, there is no property or method that returns a consistent scene root.

That's because there is no consistent scene root. When a scene is added to the scene tree, what used to be the root node of the scene is now a child node of something else. It's not a root node anymore. Even if you could figure out which node used to be the root, it's no longer a root so the method is_scene_root logically should return false anyway.

I think this can be worked around with a few lines of code, and the proposed change doesn't really add anything useful.

markdibarry commented 2 years ago

That's because there is no consistent scene root.

This proposal isn't about getting one consistent scene root, it's about what is considered the current scene root is inconsistent based on the context. The same scene, using the same methods, returns a different node depending on the context.

Even if you could figure out which node used to be the root, it's no longer a root

I'm confused. Are you saying that get_tree().edited_scene_root and get_tree().root is never updated after the creation? I feel that's all the more reason to have a method that can determine the current scene root.

I think this can be worked around with a few lines of code

As mentioned in the proposal, it can, but those few lines of code took a long time to be written due to the inconsistent results of the built in properties and methods. Having to first determine the context, and then call different specific methods based on that context is not very intuitive, and was considered unexpected behavior by the many Godot users who helped me figure this out over the course of two hours.

The other option is to change get_tree().root, get_tree().current_scene, or get_tree().edited_scene_root to return a consistent root placement across contexts, but I believe they work the way they do for a reason and this would be better suited to a separate method. Maybe rather than returning a boolen, a get_tree().current_scene_root() may be more appropriate?

Rodeo-McCabe commented 2 years ago

This proposal isn't about getting one consistent scene root,

But you just said those methods should "return a consistent root placement across contexts". How do you plan to get a consistent root without getting a consistent root?

I think those methods do exactly what one would expect. The way you described how they work makes sense to me.

A couple things I want to point out:

It sounds like you have one script that you want to run both in the editor as a tool script and at runtime during the game. IME that always leads to weird behavior, and it's better to separate it into two scripts, even if there is some code repetition.
Having a scene that relies on a parent is almost always a bad idea, for this exact reason (you can't test it on it's own, and there is the potential at runtime to attempt to use it without its parent).
it should initialize blank, since the parent should determine it's behavior. However, debugging an individual scene can be difficult without having default text

You can save the scene with the default text you want. When you run it in game, the _ready of the parent can simply set the text and it will overwrite the default. Consider the fact that it's not initialized blank doesn't matter: All ready calls are done in a single process frame so you probably won't even see the default text, and even if it does appear for a frame, that's like 1/30th of a second or less, nobody will notice and it doesn't even matter if they did anyway.

markdibarry commented 2 years ago

I'm sorry I'm doing a bad job with my Godot terminology. In most of the documentation, Godot refers to the node at this following position as the "root" node of the scene: Root

So if one wanted to get the current node at the "root" position of their scene, they would need to do this and know to do this:

var root_node
if Engine.is_editor_hint():
    root_node = get_tree().edited_scene_root
else:
    root_node = get_tree().root.get_child(0);

I think those methods do exactly what one would expect.

While it sounds like you're much more seasoned and familiar with Godot's engine, I've been a Godot user for almost three years now, and neither I nor any of the other users I reached out to knew that the "root" (the one I referred to above) in the editor is only exposed as get_tree().edited_scene_root and only in game as the first child of get_tree().root. Of course, this is just a small sized survey group.

If accessing the "root" node is not recommended, or being able to easier access the "root" node of a scene is an unwanted feature, I can close this, but I think it should be mentioned in the documentation that this is the process a user needs to recreate to do so.

h0lley commented 2 years ago

confusion about "root": there's the root Viewport, which you get via get_tree().root, and then there is the root node of your current scene - the one you marked in that screenshot - which is a child of the root Viewport and you get it via get_tree().current_scene.

in the editor, things are a bit different - but I think that's to be expected. one is a scene file being edited, the other is the scene running in the game. most people probably aren't even aware that the editor is a Godot application that uses the same SceneTree system the games use. and even with that knowledge, I wouldn't expect get_tree().current_scene to be the same thing in the editor as in the game. the editor has its own root viewport where its entire interface sits in.

originally I figured this was about an easy way to detect F6 launches, but I feel like your use-case involving the editor side of things is indeed quite niche. so you want some properties to be set differently for a scene within the editor depending on whether it's opened in its own tab or instantiated into another scene, right? is that really helpful? why can you not just set default values in one of the ways @dalexeev suggested earlier?

markdibarry commented 2 years ago

My use-case is less important since I achieved it via other means later this morning after I decided I wanted to avoid writing and using an extension method across my project if I could.

At this point, I just find that accessing the first node in the scene tree is not currently intuitive and after finding others were also unaware of how to do it, I want to either provide a better way of doing so or make the process easily accessible to the community.

markdibarry commented 2 years ago

I'm not trying to be difficult, but I feel like I gotta plead my case, because on the Discord everyone seems to think this is confusing, but in this thread I seem to be the only one who thinks this isn't intuitive. If after this comment, everyone still thinks it's a natural, direct process, I'll surrender and close!

Here's the process my brain went through:

The docs refer to it as either the root node or the top node, so one would naturally check the SceneTree methods referencing that terminology.
There's no top_node property or any methods, but there is a root property. Is that it?
No, that's the root viewport, and looking it up in the documentation the SceneTree article confirms that, but the API refers to it as a Window. In the article it shows that the SceneTree is directly below it, so I suppose the first child of that would be the first node in the scene tree, so let's check, and it is!
Wait, nope. Not when you're in the editor. Maybe I should be using a different method/property? Let's look some more.
Ah, there's current_scene. It doesn't say anything about a "root" or a "top node" but maybe it's just not named consistently with the docs? The API just says "current scene", so not much help there. Let's try it out, and success!
Wait, nope. Again, not when you're in the editor. It returns null. But I guess "Current Scene" is the terminology used in the API to access the top node.
Let's look for something else, maybe named editor_current_scene or something along those lines?
Here's edited_scene_root, but I made that mistake before. The API just says it's "the root". Maybe it's similar to root but it's the viewport root for the editor? Then I should access it's first child and that should get me the first node in the scene tree.
Wait, no, this time it actually means it's the scene's root node, and I guess this works, but only in the editor?

So now I know: root = the viewport root current_scene = the scene's root node in game but not the editor editor_scene_root = the scene's root node in the editor but not the game root

That feels like there could either be an easier way, more consistent naming, better documentation, or all three.

h0lley commented 2 years ago

I agree things could be named more clearly, especially the inconsistency between current_scene and edited_scene_root - it's easy to see how it can be confused with root.

and the docs are improvable for sure, perhaps even with that graphic you made there - that showcases it really nicely I think. the @1234 nodes could additionally be labeled something like "Editor interface Control nodes" to make people understand what those are and that the editor itself is actually a SceneTree, too.

by the way, are you reading stable or latest docs? in Godot 4, the root Viewport will turn into Window (extending Viewport).

YuriSizov commented 2 years ago

I don't believe there is an inconsistency. In the context of running code in the editor the name edited_scene_root makes perfect sense and reflects its purpose clearly. And current_scene is a part of change_scene(), change_scene_to(), reload_current_scene() API, and makes sense in that context as well.

I think that the case is curious, but the outcome is the product of misunderstanding of what a scene and what a SceneTree is, nothing more. We may want to have an API for the problem described in the OP, but as it stands the proposal ignores one more case. This proposal is written from a standpoint that a running project equals a running scene. But that's not the case. Were we to introduce a method such as is_scene_root(), users will expected it to tell them whether an arbitrary node is a root of an instanced arbitrary scene inside of your running project. This cannot be checked by using current_scene or edited_scene_root.

So I think that checking either current_scene or edited_scene_root to specifically solve the problem in the OP is the best solution. We could probably rename edited_scene_root to edited_scene in 4.0, if it helps in any way. That would require resolving the conflict with the existing get/set_current_scene methods in some places.

markdibarry commented 2 years ago

Thanks for all the feedback. I believe that there's a misunderstanding about what is desired, but to be clear, it's just a way of knowing what the top node is in the scene tree, regardless of context. Basically (as I now know), current_scene in the project, and edited_scene_root in the editor. It's clear that is_scene_root is not a wanted feature, but I think we've got to the root of the issue that the two existing properties meant to solve this problem are not named consistently enough and API descriptions aren't descriptive enough to know they're related. I'm going to close this and open a more focused issue.

YuriSizov commented 2 years ago

I believe that there's a misunderstanding about what is desired, but to be clear, it's just a way of knowing what the top node is in the scene tree, regardless of context.

That was clear to me, but I also pointed out that there is a different thing that users want and would expect to get with a method called is_scene_root(). You are focused on your case, but Godot users often ask how to detect if a node was spawned with a specific scene. And in that context is_scene_root(), as you suggest it, would not work for them, but the name suggests to them that it might. And it so happens that your use case and their use case cannot be solved at the same time, because you don't want to know if a node is the scene root. You want to know if the scene itself is instanced standalone.

markdibarry commented 2 years ago

Ah, my bad! I see what you mean now. You're saying (especially if this were a method called on a node), that users may expect that it would tell them if it's the root of a scene that was loaded and not the scene tree itself. When something is confusing, it's so easy to forget that my solution might cause the same amount of confusion for others but in a different place.

I think you're right that reconciling the naming and fleshing out the API description would be enough to solve this.

godotengine / godot-proposals