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

Make replacing_by emit slightly earlier #90479

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Apr 10, 2024

This mainly fixes bugs that occur when replacing scene roots with certain node types.

To reproduce the bug:

  1. Create a new scene,
  2. Add some visible 2D child nodes to the scene,
  3. Select the scene root node, right-click to open the menu, select Change Type, and then select the Camera2D type to change.
  4. The display of nodes in the 2D screen is offset.
248e5bf This PR
0 1

Fix #73009,

@ajreckof
Copy link
Member

This is introducing a compatibility breakage. Codes that would be broken by this change can most likely just await tree_entered on the node sent with the signal. Another solution would be to defer the connection to the signal which would put the call after children have been moved.

@ajreckof
Copy link
Member

To be clear even with the compatibility breakage I'm fully in favor of this being merged.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 3, 2024

I assume the problem is set_edited_scene() connected to replacing_by signal? Wouldn't it be enough to manually call set_edited_scene() when replacing root?
Also looks like the PR needs rebase, because it removes a file that doesn't seem to exist.

The `replacing_by` signal is mainly used to update the cached scene
root when replacing the scene root. Emit this signal a little early
to ensure that certain flags for the editor are not obsolete during
entering tree.
@Rindbee Rindbee force-pushed the make-replacing_by-emit-slightly-earlier branch from 8b9c56d to 382c230 Compare June 3, 2024 22:09
@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 3, 2024

Some classes require us to check the current scene when entering the tree in order to improve the interactivity of the editor.

As for the situation when the node exits the tree, I forgot whether I have investigated it.

If no class needs to check the current scene when exiting, it is OK to manually call set_edited_scene() before replace_by(). However, in this case, it may be necessary to check whether the replaced node is the current scene root.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 3, 2024

it may be necessary to check whether the replaced node is the current scene root.

That's what I mean. Whenever a node is being replaced (by scene tree dock action), check if it's the scene's root and set edited scene if it is.

@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 4, 2024

The choice to use signal was made in #68599, presumably because EditorNode::set_edited_scene() might automatically add new node to the tree, which would then fail when calling Node::replace_by().

godot/editor/editor_node.cpp

Lines 3726 to 3729 in 5f1184e

if (p_scene) {
if (p_scene->get_parent() != scene_root) {
scene_root->add_child(p_scene, true);
}

ERR_FAIL_COND(p_node->data.parent);

Node::replace_by() requires that the new node cannot have a parent node.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 4, 2024

So the solution is removing the node after setting it as scene 🙃

if (oldnode == edited_scene) {
	EditorNode::get_singleton()->set_edited_scene(newnode);
	newnode->get_parent()->remove_child(newnode);
}

It's best to avoid a core compatibility breakage due to editor issue, unless the new code has additional benefits (but it doesn't, no?).

@akien-mga
Copy link
Member

Superseded by #92760.

@akien-mga akien-mga closed this Jun 7, 2024
@akien-mga akien-mga removed this from the 4.3 milestone Jun 7, 2024
@Rindbee Rindbee deleted the make-replacing_by-emit-slightly-earlier branch June 7, 2024 22:20
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.

Editor freezes when changing node type Popup to AcceptDialog
5 participants