godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.92k stars 20.17k forks source link

Errors when changing scene using change_scene_to_file() #85852

Open golddotasksquestions opened 9 months ago

golddotasksquestions commented 9 months ago

Tested versions

Reproducible in: 4.2 stable

System information

Win 10, Godot 4.2 stable, Forward +, dedicated Nvidea RTX 1080

Issue description


Edit: currently recommend workaround/fix seems to be to call change_scene_to_file() deferred.

So instead of

get_tree().change_scene_to_file("res://some_scene.tscn")

write:

get_tree().change_scene_to_file.bind("res://some_scene.tscn").call_deferred()


Original issue description:

Using get_tree().change_scene_to_file("res://some_scene.tscn") works , but it produces a number of red errors. These seem to be different depending on the circumstances.

To test this, I made two scenes, Scene A and Scene B using Area3Ds and CharacterBody3D (with default template script):

image


In Scene A I change scene by walking into an Area3D with a CharacterBody3D (template script), triggering a body_entered signal:

extends Node3D

func _on_area_3d_body_entered(body):
    if "Player" in body.name:
        get_tree().change_scene_to_file("res://change_3d_scene_b.tscn")

This will produce these two errors:

E 0:00:05:0401 change_3d_scene_A.gd:5 @ _on_area_3d_body_entered(): This function can't be used during the in/out signal. <C++ Error> Condition "locked" is true. <C++ Source> scene/3d/area_3d.cpp:289 @ _clear_monitoring()

change_3d_scene_A.gd:5 @ _on_area_3d_body_entered() E 0:00:05:0401 change_3d_scene_A.gd:5 @ _on_area_3d_body_entered(): Removing a CollisionObject node during a physics callback is not allowed and will cause undesired behavior. Remove with call_deferred() instead. scene/3d/collision_object_3d.cpp:113 @ _notification() change_3d_scene_A.gd:5 @ _on_area_3d_body_entered()

In Scene B I change scene by walking into an Area3D with a CharacterBody3D, triggering a body_entered signal which will set a can_interact boolean variable in the character script. If the player then presses an Input action, the character fires a global custom signal, which the level connects to and changes the scene:

Global Autoload:

signal change_level

Character:

var can_interact = false

func _physics_process(delta):
    if can_interact:
        if Input.is_action_just_pressed("ui_accept"):
            Global.change_level.emit()

Level:

extends Node3D

func _ready():
    Global.change_level.connect(change_to_other_level)

func _on_area_3d_body_entered(body):
    if "Player" in body.name:
        body.can_interact = true

func _on_area_3d_body_exited(body):
    if "Player" in body.name:
        body.can_interact = false

func change_to_other_level():
    get_tree().change_scene_to_file("res://change_3d_scene_a.tscn")

This will produce these four errors:

E 0:00:07:0043 change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process(): Condition "!is_inside_tree()" is true. Returning: Transform3D() <C++ Source> scene/3d/node_3d.cpp:343 @ get_global_transform()

change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process() E 0:00:07:0043 change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process(): Condition "!is_inside_tree()" is true. Returning: Transform3D() scene/3d/node_3d.cpp:343 @ get_global_transform() change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process() E 0:00:07:0043 change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process(): Parameter "body->get_space()" is null. servers/physics_3d/godot_physics_server_3d.cpp:915 @ body_test_motion() change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process() E 0:00:07:0043 change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process(): Condition "!is_inside_tree()" is true. Returning: Transform3D() scene/3d/node_3d.cpp:343 @ get_global_transform() change_3d_scene_b.tscn::GDScript_7cg2h:34 @ _physics_process()

Note: Line 34 in the character script is just move_and_slide()


Possibly related issue (however that's a different error): https://github.com/godotengine/godot/issues/84681

Steps to reproduce

Use get_tree().change_scene_to_file("res://some_scene.tscn") as described above, or test the provided Minimal Reproduction Project below.

Minimal reproduction project (MRP)

change_scene_errors.zip

Mannega commented 9 months ago

I think if they are not affecting the game when run and not crashing it, I think it is just a Godot bug, it can happen, but if it is affecting the game, I think there is a thing.

akien-mga commented 9 months ago

Both situations are caused by the change in 4.2 that makes change_scene_to_* methods no longer being deferred internally. When you call those methods, the current scene is deleted immediately, and the new scene is added to the root shortly after.

In this case you'd need to defer both calls to change_scene_to_file to work around that change and make it work similarly to what used to happen in 4.1.

golddotasksquestions commented 9 months ago

I assume there are good reasons for this, but from a user perspective, it seems like more logical, user friendly and intuitive would be to deferred it internally as it was in earlier versions of Godot. Where can I read up on the reasoning for this change?

Either way, these errors do not help much to find the cause or the fix for this issue.

It's also not documented in the methods description:

Error change_scene_to_file(path: String) Changes the running scene to the one at the given path, after loading it into a PackedScene and creating a new instance. Returns OK on success, ERR_CANT_OPEN if the path cannot be loaded into a PackedScene, or ERR_CANT_CREATE if that scene cannot be instantiated. Note: See change_scene_to_packed() for details on the order of operations.

akien-mga commented 9 months ago

Where can I read up on the reasoning for this change?

It's discussed in https://godotengine.org/article/godot-4-2-arrives-in-style/#critical-and-breaking-changes with a link to the PR that changed it. There wasn't much discussion there at the time, as it was an attempt at fixing critical issues and we hadn't considered all implications. This was further discussed in #85251.

It's also not documented in the methods description:

See the note you quoted :)

Note: See change_scene_to_packed() for details on the order of operations.

golddotasksquestions commented 9 months ago

@akien-mga Thank you for the links!

See the note you quoted :)

This does not help. If the user is expected to only call this method deferred, it should say so in the method description. Moreover having to call_deferred() is also not mentioned in the other method this links to:

Error change_scene_to_packed ( PackedScene packed_scene ) Changes the running scene to a new instance of the given PackedScene (which must be valid). Returns @GlobalScope.OK on success, @GlobalScope.ERR_CANT_CREATE if the scene cannot be instantiated, or @GlobalScope.ERR_INVALID_PARAMETER if the scene is invalid. Note: Operations happen in the following order when change_scene_to_packed is called: 1) The current scene node is immediately removed from the tree. From that point, Node.get_tree called on the current (outgoing) scene will return null. current_scene will be null, too, because the new scene is not available yet. 2) At the end of the frame, the formerly current scene, already removed from the tree, will be deleted (freed from memory) and then the new scene will be instantiated and added to the tree. Node.get_tree and current_scene will be back to working as usual.

This ensures that both scenes aren't running at the same time, while still freeing the previous scene in a safe way similar to Node.queue_free.

akien-mga commented 9 months ago

It's not intended to only be called deferred. In most situations you can call it as is and have the scene change done right away.

The errors you get come from doing tree manipulation during a physics callback, as they mention. You would have the same issue trying to remove a physics node in _on_area_3d_body_entered - which is what changing the scene ends up doing too.

golddotasksquestions commented 9 months ago

It's not intended to only be called deferred. In most situations you can call it as is and have the scene change done right away.

Except for UI, I have a hard time to imagine what kind of situations these might be. Most scene changing situations in my games and most games I see made by others are triggered by a physics body entering an Area or interacting with an Area, triggering a custom signal. Such as I tried to model here in the MRP.

You would have the same issue trying to remove a physics node in _on_area_3d_body_entered - which is what changing the scene ends up doing too.

I just tested this in Scene A of the MRP and I'm not sure what you mean. Both body.get_parent().remove_child(body) and body.queue_free() work fine just as expected without any errors. Even body.free() works without any errors in the above MRP.

AThousandShips commented 9 months ago

You're not just removing that collision object when changing scenes though, so it's probably triggered by some of the other bodies, otherwise why would that warning be triggered?

KoBeWi commented 9 months ago

The physics callback comes from area, not body. If you do get_parent().remove_child(self) you should be getting the same error.

golddotasksquestions commented 9 months ago

Thanks to everyone who commented and tries to shine a light on these errors! As someone who has used Godot for years, I would like to state that I feel like non of this is transparently communicated to the user. Not through the errors, not through documentation, not through method names.

I don't think the method names makes it clear on how to use them, neither does the description, and even after you all explaining, I still don't have a firm grasp on what is going on, why it is necessary we now have changing scenes be much more complicated, and confusing when it worked fine before (at least in my usage).

That's from someone with more than 5 years of Godot experience. I can only imagine stuff like this would be a lot more confusing for someone starting out. Please keep in mind how most of you are daily work with the Godot source, that's not the experience or background knowledge of most Godot users.

I'm not sure what is the solution here, if it is just better documentation or something else, but I feel like something needs to be done to make something as central to using Godot as changing scenes a lot smoother and less error prone.

AThousandShips commented 9 months ago

The change was made with the goal of making scene transitions safer and smoother

And adding a specific error if changing the scene when in this state is being investigated, but the method name doesn't give any indication either way to the behavior, it's not named change_scene_to_file_now or something similar, most method don't communciate their timing except when there's a companion method that doesn't defer part of the call. Note that the method is equally confusing in the name in the previous behavior where it did all at the end of the frame, without any name indicating it wasn't instant.

But I don't see how it's not transparent? It is documented and in the release post. This change was done early in the 4.2 release cycle and has been tested extensively and been there for testing by people using this behavior 🙂

golddotasksquestions commented 9 months ago

most method don't communciate their timing except when there's a companion method that doesn't defer part of the call.

Maybe this means change_scene_to_file() needs a companion method then? Similarly to free() and queue_free()? So maybe queue_change_scene_to_file()?

As a new user, I learned very early on to use queue_free() rather than free(), even though I did not fully understand the implications. A similarly named queue_change_scene_to_file() which executes deferred by default would be similarly intuitive, I guess. At least it would match existing syntax and would be more self-explanatory.

Note that the method is equally confusing in the name in the previous behavior where it did all at the end of the frame, without any name indicating it wasn't instant.

I very much disagree. get_tree().change_scene() just worked. I can't recall it ever failing or produce any errors in all those years. Thanks to it, changing scenes was one of the least confusing aspects about Godot. Now with, not so much anymore.

But I don't see how it's not transparent? It is documented and in the release post.

I'm telling you as a long time user: It's not. For you, for akien or other contributors and C++ magicians, what is written in the release nodes and API description might sound totally obvious and self-explanatory. I have read both the documentation and the release post many times before creating this issue. To me, a non C++ user who does not work with the Godot source every day: Why we are getting these errors now, when used as explained in the OP and MRP, is totally opaque, non transparent. Please accept this point of view.

AThousandShips commented 9 months ago

Then how would it be transparent? How would it be more transparent than writing exactly how it changed? Please explain how you'd like it to be explained to be cleared than that 🙂

We can't really explain every are it affects, that's something that has to be worked out based on how you use it, because we can't anticipate every possible use case, unless people express or use them

golddotasksquestions commented 9 months ago

I don't understand what the problem was with get_tree().change_scene(), why it had to change, I don't understand fully why these errors appear now with the new method. I don't think I'm in the position to explain something I don't fully understand myself.

I thought I knew what's going on, but I'm obviously wrong. Which is why I opened this issue.

We can't really explain every are it affects, that's something that has to be worked out based on how you use it, because we can't anticipate every possible use case, unless people express or use them

Changing the scene when a character or other physics body enters an area or interacts with one is not a rare obscure usecase. It's THE most usecase for this method in any game.

AThousandShips commented 9 months ago

I'm not sure why this wasn't noticed, perhaps people were being safe and calling this method deferred to not do scene tree changes during physics callbacks

But as I said, we're looking into adding specific clear errors, and the change was done with the goal to fix a number of bugs in the scene change sequence

Lippanon commented 9 months ago

I don't understand what the problem was with get_tree().change_scene(), why it had to change...

The main relevant changes were done in https://github.com/godotengine/godot/pull/78988 and https://github.com/godotengine/godot/pull/85184, perhaps this will give you more context on the why.

I don't understand fully why these errors appear now with the new method.

This was explained here https://github.com/godotengine/godot/issues/85852#issuecomment-1843293368 - specifically 'manipulation during a physics callback'.

Personally, I've always called this method deferred to avoid any potential problems. Usually games also have animations, and loading, instead of an immediate transition. For what it's worth, a long existing demo (https://github.com/godotengine/godot-demo-projects/blob/master/loading/autoload/global.gd) explained how to make the change manually, with a deferred call for the reasons mentioned, that I used to use, in conjuction with threaded loading, to avoid errors with the old implementation of the methods in question.

luiscesjr commented 9 months ago

Personally, I've always called this method deferred to avoid any potential problems. Usually games also have animations, and loading, instead of an immediate transition. For what it's worth, a long existing demo (https://github.com/godotengine/godot-demo-projects/blob/master/loading/autoload/global.gd) explained how to make the change manually, with a deferred call for the reasons mentioned, that I used to use, in conjunction with threaded loading, to avoid errors with the old implementation of the methods in question.

I too used to use deferred to load my scenes, but somehow the latest engine changes broke it. Anyways, the following code:

get_tree().change_scene_to_file.bind("res://some_scene.tscn").call_deferred()

Did not work for me, somehow it returned null, but what was null was the current scene? Or something like that, I forgot, but it looked like it was freeing before it was supposed to, and not having anything on tree to change to, error happened. Apparently this is the new expected behavior, or so that's what I understood from all of it, sometimes I find confusing in my mind trying to translate English in my head.

Anyways, instead I used @Lippanon 's link, and that seems to be working here, with a little change on freeing the current scene:

    get_tree().current_scene.queue_free() # Instead of free()
    var packed_scene := ResourceLoader.load(path) as PackedScene
    var instanced_scene := packed_scene.instantiate()
    # Add it to the scene tree, as direct child of root
    get_tree().root.add_child(instanced_scene)
    # Set it as the current scene, only after it has been added to the tree
    get_tree().current_scene = instanced_scene

And while the docs tell you what is going on, the how to do it is a bit vague with lots of options to go to, but few examples.

Strangely tough, in the docs it tells you to free() instead of queue_free(), which not only results in error by attempting to free a locked object (Not calling it deferred), but also seems unsafe.

Lippanon commented 9 months ago

get_tree().change_scene_to_file.bind("res://some_scene.tscn").call_deferred()

I tried this in the MRP and it works with no errors, using v4.3.dev.custom_build [84692c625]. You can also do get_tree().call_deferred("change_scene_to_file", "res://change_3d_scene_a.tscn") and it also works, in both scene changes of the MRP, without any errors.

Notice that, in the demo above, the function that calls free() instead of queue_free() is being called deferred, which is why it says it's safe to do so.

As for the docs you mention, they probably should mention 'deferred' somewhere, I agree.

georgespatton commented 8 months ago

What would be the code to utilize this new behavior correctly? This print statement in this block returns null:

await get_tree().change_scene_to_file("res://scenes/new_area.tscn")
await get_tree().process_frame
print("CHANGED TO AREA:", get_tree().current_scene)
freshlybakedhot commented 8 months ago

I am a new user who is accustomed to using C #, so the correct code for this issue is: GetTree().CallDeferred("change_scene_to_file",path); And the original code was: GetTree().ChangeSceneToFile(path);

This is a bit against habit

Reintjuu commented 8 months ago

The workaround works properly, but how do you get the result from the change_scene_to_file method when deferring the call? call_deferred() is documented to always return null. I'd like to receive and handle the errors returned by the method.

MasterMann commented 7 months ago

@freshlybakedhot your snippet no longer works in 4.2.1. _Ready gets called on the new scene node, but calling GetTree() in that function returns null, which seems like a bug.

jahed commented 6 months ago

It feels like change_scene_to_x functions are broken in 4.2 and @Lippanon's workaround is the way to go for now. I haven't encountered any issues with it so far and there doesn't seem to be any other stable solution.

In my case, I noticed that on 4.2.1, using change_scene_to_file does not delete the previous scene. It's still available but orphaned and signal handlers are still called. The docs say the scene is deleted "immediately" but that isn't happening and it's impossible from a user perspective to know why it isn't happening. I'd have to dig into engine code. I tried to queue_free the orphaned scene/node after the call_deferred but it closes the game without errors.

DIPANJAN01 commented 6 months ago

I feel like these methods (change_scene_to_file and change_scene_to_packed) should in fact be called as deferred implicitly as they once used to, because that is arguably the safer approach. Its safer because otherwise it throws errors during physics callbacks, and changing scenes during a physics callback is a really common scenario.

If someone wants to change scene immediately (which is unsafe because it will give errors if invoked during physics callbacks, which again is really common), separate functions should be provided (such as change_scene_to_file_now and change_scene_to_packed_now).



My Elaboration: We should streamline it as much as possible for the casual user. From the perspective of the casual user, all he wants is to invoke a function and his scene should change without any errors (no matter how busy his scene is with physics objects in motion during the scene change). It can be mentioned in the docs that these methods change the scene as deferred, not immediately, for more clarity.

Now if someone wants to change the scene immediately instead of Godot doing it smoothly over time (to avoid errors) for him, he can invoke these methods appended with _now in their method names, which will change it immediately without defer. Think of the casual user and a streamlined experience.

You can argue against this all you want, I can't change your anyone's mind, but the fact stands that the user now has to type more and think more about his physics objects possibly getting offed while in motion resulting in errors, which is overall a less streamlined experience than before where the method took care of it.

fkeyzuwu commented 6 months ago

These are some of the most common methods in Godot that almost every game would use. I don't think we should make the main way of using them feel like a workaround, especially since it worked fine for the most part beforehand. There's also the case where you want to do access something in the scene afterwards right after you switch to it, you would have to defer everything as well, forcing you to either call every line with defer(which is very inconvenient) or make a separate function/lambda for everything you want to do and defer that. The workflow is very iffy.

sutyrin commented 2 months ago

Hey, +1 on this, and here is some input from a several-days-into-Godot noob. This is at 4.2.2-stable on Linux.

at physics process when I do

queue_free()
get_tree().change_scene_to_file("res://menu.tscn")

It works, but once I let myself await:

await anim.animation_finished
queue_free()
get_tree().change_scene_to_file("res://menu.tscn")

Then scene changing line breaks and I have to rewrite as per recommended here:

await anim.animation_finished
queue_free()
get_tree().change_scene_to_file.bind("res://menu.tscn").call_deferred()

Which then works.

From very outside it occurs to me that upon using await for the first time the function magicalliy switches to async mode, which makes all subsequent calls to be performed async (deferred) as well. A surprising experience.

UPD. Of course, as per docs, first await converts a function into a coroutine. Would a «static typing» of a function as coroutine help?

Anyways, from this useful conversation I take away a habit to change scenes asyncronously no matter what, thanks for that!

Hope this helps, thanks!

P.S. I think also it might be a good feature for editor to highlight such cases (if detectable statically) and suggest a correction (like «this future is never awaited» in PyCharm).