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

Correctly replace scene root when must_reload in EditorData::check_and_update_scene() #90432

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Apr 9, 2024

We need to update the scene root in multiple singletons to ensure that certain flags are correct. This is what EditorNode::set_edited_scene() does.

Usually we use replace_by to complete the replacement of the scene root. Call EditorNode::set_edited_scene() when the replacing_by signal is emitted to set the new scene root. This is suitable when using a single node to replace, but this may be problematic if the replacing node is a tree. Because during the call to replace_by(), the new node and its child nodes will enter tree during parent->add_child(p_node), and later emits the replacing_by signal. This results in some flags potentially being incorrect during the enter tree of the new node's children.

When the parent scene has to be reloaded because the child scene changes and switches to the parent scene, there is no need to use replace_by() since the scene's diffs are already saved.

Fix #90301.

@akien-mga
Copy link
Member

CC @ajreckof

I feel like we're going around in circles there, we modified replace_by for this but now it's no longer needed.
Might be best to go back to the original state and figure out again how to solve the issue without regression.

@AThousandShips
Copy link
Member

It does feel like where playing a game of whack-a-bug with this issue, so it'd indeed be good to revert to a clean slate and dig deeper figure out what's causing what, and make sure we have a fix that doesn't cause regression, it seems like some part of the process of replacement here is missed and that it is causing these problems, due to things occuring behind the scenes that were missed

@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 9, 2024

It seems inappropriate to use replace_by() inside tree when changing scene root (node type). The replacing_by signal is a bit late.

To prevent certain flags from being wrong when entering the tree due to the cached scene root not being updated. It seems that we need to first let the old scene leave the scene tree, then replace the scene root, and finally use EditorNode::set_edited_scene() to add the modified scene to the scene tree again.

@ajreckof
Copy link
Member

ajreckof commented Apr 9, 2024

I feel really dumb right now 😅 This is in fact what I wanted to do but I don't know why I thought I couldn't directly call it. Obviously some of my previous changes can be reverted but some others are still needed for this to work here is a recap of what should be reverted from my previous commits the rest are needed for this commit.
Rindbee/godot@correctly-replace-scene-root-when-must_reload...ajreckof:godot:reverts-where-needed-keep-where-usefull

@Rindbee Rindbee requested review from a team as code owners April 10, 2024 00:14
@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 10, 2024

Maybe the timing I chose for replacing_by in #68599 was inappropriate. Emitting it a little early helps ensure that certain types of nodes are configured correctly when entering the tree as the scene root.

Rindbee and others added 2 commits April 10, 2024 09:49
…_and_update_scene()`

We need to update the scene root in multiple singletons to ensure that
certain flags are correct. This is what `EditorNode::set_edited_scene()`
does.

Usually we use `replace_by` to complete the replacement of the scene
root. Call `EditorNode::set_edited_scene()` when the `replacing_by`
signal is emitted to set the new scene root. This is suitable when
using a single node to replace, which may be problematic if the
replacing node is a tree. Because during the call to `replace_by()`,
the new node and its child nodes will enter tree during `parent->
add_child(p_node)`, and later emits the `replacing_by` signal.

When the parent scene has to be reloaded because the child scene
changes and switches to the parent scene, there is no need to use
`replace_by()` since the scene's diffs are already saved.
scene/main/node.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.
The third commit should likely be merged with the first one.

@ajreckof
Copy link
Member

The third commit won't cause problems in the editor but since it is a signal hence exposed this is still a compatibility breakage. I'm not sure the bug this specifically fix warrant this compatibility breakage. Whatever I think it is better to keep separate if we finally decide to revert it because it was in the end too much of a compatibility breakage.
For us to have a better appreciation of what it fixes and if it is worth the compatibility breakage, it would be great to have a clear reproduction step to a bug that would be fixed by this.
I'm wondering if it shouldn't have it's own PR for those reasons.

@Rindbee Rindbee force-pushed the correctly-replace-scene-root-when-must_reload branch 3 times, most recently from 8b9c56d to 248e5bf Compare April 10, 2024 10:27
@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 10, 2024

Create PR #90479 for the third commit.

@akien-mga akien-mga merged commit 9c5e968 into godotengine:master Apr 10, 2024
16 checks passed
@Rindbee Rindbee deleted the correctly-replace-scene-root-when-must_reload branch April 10, 2024 12:36
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Saving sub-scenes will mess up the display of the main scene on the 2D screen
5 participants