get_tree() returning null immediately after calling change_scene_to_file() in _process or _physics_process()

[nenad-jalsovec]

Godot version

v4.2.beta4.official [93cdacb]

System information

Windows 10

Issue description

get_tree() returns null when called immediately after calling change_scene_to_file() in _process() or _physics_process(). The error printed in debugger: scene1.gd:14 @ _process(): Parameter "data.tree" is null. Regardless of this the scene gets properly changed.

Doing the same from _ready() works as expected.

Steps to reproduce

Make a scene with a node and another test scene to change to. Attach the following script to the node in the first scene:

extends Node
func _process(delta):
	if Input.is_action_just_pressed("ui_accept"):
		print(get_tree())
		get_tree().change_scene_to_file("res://scene2.tscn")
		print(get_tree())

Press enter and the output will be:

<SceneTree#25987908681>
<Object#null>

Minimal reproduction project

N/A

[akien-mga]

Confirmed in both 4.2-rc1 and fa4a653, it's a regression in 4.2.

It started in 4.2-dev2.
List of changes: https://godotengine.github.io/godot-interactive-changelog/#4.2-dev2

Presumably a regression from #78988. #85184 didn't solve that part.

CC @RandomShaper

[RandomShaper]

Different possibilities here:
a) Verify that #78988 didn't fix anything in the end. Checking some related issue threads, it may even have created new issues? I'm not sure, hard to track. If we believe that's the case, let's just revert it plus its fixups.
b) Consider the current implementation does the right thing by immediately deattaching the current scene from the tree upon calling change_scene_to_file(). It may be a behavioral change, but in this case we'd consider it has more sense than the classic one.
c) Add some kind of fix to the current implementation.

[akien-mga]

a) Verify that #78988 didn't fix anything in the end. Checking some related issue threads, it may even have created new issues? I'm not sure, hard to track. If we believe that's the case, let's just revert it plus its fixups.

I tested the two linked issues, and they seem to highlight different problems, only one of which was fixed by #78988.

The MRP in #78788 seems fixed for me, i.e. scene changing no longer causes the slot >= slot_max error on that scene.

#70910 on the other hand is more complex, it's a problem that could be triggered by changing scenes (as seen in #78788), but not only. It seems to be a more general issue with removing/adding children. The recent MRP in #70910 (comment) still triggers an issue as of 4.2-rc1, even worse than in 4.1.3-stable for me (in 4.1.3 it spams errors, in 4.2-rc1 it segfaults).

Overall #78988 did not address the main issue which is a difference in behavior between DEBUG_ENABLED and non-DEBUG_ENABLED builds (release exports). The changes to scene loading impact both types of builds the same and likely made the bug harder to reproduce, but it's still there.

So #78988 might have made things better in some cases, but not solved the root of the problem. So maybe a revert for now is the better approach, to re-assess for 4.3. On the other hand change_scene_* is the main way people handle scene changes, and this might well have mitigated the issue for a lot of cases.

If we keep the change as is (scenario (b)), we would need to document this behavior clearly, advising to yield for a frame before accessing the scene tree after changing scene.

Edit: Notably, this bug doesn't seem too critical / game breaking. To be clear, the behavior in 4.1.3 would be that get_tree() would still return the current/old scene because the actual scene change is deferred. It's not clear whether that was obvious to users and useful to still have access to a soon-to-be-deleted scene tree.

So a combination of (b) and maybe (c) to avoid getting a runtime error could be nice.

But it's a bit worrying that we're only discovering this regression at 4.2-rc1 for a change merged in 4.2-dev2... so it might well need a bit more time to cook given the impact it can have on projects.

[akien-mga]

Worth noting that the same "bug" can be reproduced in 4.1.3-stable with this:

print(get_tree())
get_parent().remove_child(self)
print(get_tree())

And something like this doesn't execute the second print at all:

print(get_tree())
get_tree().change_scene_to_file("res://scene2.tscn")
await get_tree().process_frame
print(get_tree())

So the change in assumption here is that change_scene_* immediately removes the current node from the tree, but still finishing running the current function, and users need to be clearly aware of that, as if they were removing self.

IMO documenting this as an intentional change is enough for 4.2.

[YuriSizov]

So to reiterate, as of 4.2 calls to change the current scene are processed immediate and the node which you are replacing is removed immediately. This is consistent with other types of tree manipulation, like adding and removing children yourself, as illustrated above.

Calling get_tree fails and errs because your node is no longer inside of the tree. You can use is_inside_tree to guard against errors, but you can no longer access the tree at this point, and this is expected.

Prior to 4.2 the change would only be processed at the end of the frame, so the node would remain in the tree. I would say, you should not rely on this behavior in general. This is not the case with any other method of tree/child manipulation. The engine may defer some steps internally for performance or logical reasons, but you should always consider the node as detached after you requested it to be so.

Store a tree reference before replacing the node if you need it afterwards. Or better yet, this should probably be called outside of the node being replaced. From an autoload perhaps.

[nenad-jalsovec]

@YuriSizov It that's the case, it is a rather big change in respect to all other previous versions where changing the scene was considered a deferred call (stated explicitly in docs). Although I think it's better for scene change to take effect "immediately" it may fundamentally break many people's existing code.

If this is intentional design choice, it should be prominently highlighted on "what's new" page.

[YuriSizov]

@nenad-jalsovec We'll see how big it is, I guess. So far this is the first report on this matter, and I'm not sure people who use such a high level method consider low level implications of it being a deferred call or not.

I have no problem listing it among breaking changes in the release article.

[nenad-jalsovec]

@YuriSizov I'm reporting this after trying to help a forum user with a scene swapping related problem. Because of this change the approach to solve it would differ rather significantly between 4.1 and 4.2.

[YuriSizov]

Feel free to post the exact problem if you want to discuss it. But from your simplified example the changes would be minimal. Although as I mentioned before, calling for a change of scene from the scene that is going to be changed away — that always has been a bad idea in my opinion.

[AThousandShips]

I'd agree that the exact timing of methods which do not have specific timing declared is an implementation detail, and not compatibility enforcing

Further anything that changes things like this should in my opinion be seen as "no use beyond this point", similar with queue_free, once you call these methods you should treat the thing your operating on as unsafe, regardless of when the actual change happens

[nenad-jalsovec]

@YuriSizov I'll describe the specific situation here if you don't mind. change_scene_to_fille() was called from some node's _process(). At the same time there is an autoloaded node in existence that calls get_tree() every frame in its _process() and that call returned null when the scene change took place. Sounds problematic to me.

I made initial trivial reproduction example under assumption that the scene change is deferred.

[YuriSizov]

At the same time there is an autoloaded node in existence that calls get_tree() every frame in its _process() and that call returned null when the scene change took place. Sounds problematic to me.

Are you saying that get_tree() that returned null was called from an autoload and NOT the node from the old scene being removed? That shouldn't happen and not what your example in the OP is demonstrating. Would you mind providing an MRP for your case?

[nenad-jalsovec]

@YuriSizov Oops, I was wrong there. The autoload works fine.

Can you then 100% confirm that this change in change_scene_to_file() timing is by intent?

[YuriSizov]

@nenad-jalsovec Yes, the change to the timing is done intentionally and is a core part of the fix. We will mention the difference in behavior introduced by it in the release article.

[nenad-jalsovec]

@YuriSizov It'd also be good to remove the following paragraph from the SceneTree reference page in docs:

Note: The new scene node is added to the tree at the end of the frame. 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(). As such, you won't be able to access the loaded scene immediately after the change_scene_to_file() call.

[nenad-jalsovec]

This can be closed I guess. Thanks for the quick response guys 👍

[AThousandShips]

This should still be documented, please leave open until resolved

[RandomShaper]

@YuriSizov It'd also be good to remove the following paragraph from the SceneTree reference page in docs:

Note: The new scene node is added to the tree at the end of the frame. 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(). As such, you won't be able to access the loaded scene immediately after the change_scene_to_file() call.

I think that paragraph is still true. The docs fix would consist in adding some info about the fate of the current scene, wouldn't it?

[YuriSizov]

Yes, good point. What is mentioned in this note is still true, but we should probably mention the current/leaving scene and its fate explicitly.

[nenad-jalsovec]

@YuriSizov Hm, yes. Also what happens with the loaded scene? If querying SceneTree::current_scene immediately after calling change_scene_to_file() it will be null. So what's the benefit of doing it immediately if it's not immediately available and still loaded deferred? The only thing that seemingly happens is that replaced scene is deleted and there is a gap until the end of current frame processing in which we don't have a current scene. Maybe I'm missing something here but at least it looks like that.

[YuriSizov]

Also what happens with the loaded scene? If querying SceneTree::current_scene immediately after calling change_scene_to_file() it will be null.

That's what the note you quoted says, yes.

So what's the benefit in doing it immediately

The current scene is removed as soon as possible so everything can finish processing gracefully and without overlapping with the incoming scene.

[nenad-jalsovec]

@YuriSizov

The current scene is removed as soon as possible so everything can finish processing gracefully and without overlapping with the incoming scene.

Isn't that precisely what's already accomplished by doing it deferred, but without losing the access to current scene until the end of current frame?

[LunaticInAHat]

I believe I may have tripped over this issue (or something adjacent to it) when I was converting a project from Godot3 to Godot4 a few weeks ago. I had some code in _unhandled_input() that ended up calling SceneTree::set_current_scene(), before doing a get_viewport().set_input_as_handled(). This sequence worked fine in 3.x (with the obvious substitution of get_tree() for get_viewport()), but crashed in 4.x, due to get_viewport() returning null.

I "fixed" it by just reversing the order of operations, but can confirm that putting it back the old way still crashes in 4.2.

If this was indeed related, then updating the documentation to just warn against get_tree() may not be enough to keep people from tripping over the behavior. If it was not related, then my apologies for introducing an irrelevant anecdote :)