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

Call EditorNode::set_edited_scene() manually instead of via the replacing_by signal #92760

Merged

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Jun 4, 2024

Should not change scene_root's child node in EditorNode::set_edited_scene() if replaced later using replace_by. Therefore, it is better to allow the child nodes of scene_root not to be modified automatically.

Supersedes #90479.

Fix #73009.

@Rindbee Rindbee requested a review from a team as a code owner June 4, 2024 14:52
@AThousandShips AThousandShips added this to the 4.3 milestone Jun 4, 2024
@AThousandShips
Copy link
Member

Is this the only case when replace_by can be called? Or can it be triggered elsewhere?

@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 4, 2024

This is for the case of replacing the edited scene root. If you want to replace the original scene root with a new node, you need to manually call EditorNode::set_edited_scene() to set the new node. However, for replace_by, it is best not to modify the parent node of the new and old nodes first to meet certain conditions.

Is this the only case when replace_by can be called? Or can it be triggered elsewhere?

The main purpose of using replace_by is to make the new node inherit some data of the old node. If the new node has already obtained this data, there is no need to use replace_by.

@AThousandShips
Copy link
Member

Just as long as there aren't cases where the editor depends on the signal alone that should be fine, but I'm unsure if it doesn't depend on it for other contexts or I feel signals just wouldn't have been used for this it's quite odd

@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 4, 2024

What confuses me is that EditorNode::set_edited_scene updates 3 edited_scene_root, and it seems unclear which one should be the main flag.

editor/editor_node.cpp Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the deprecate-replacing_by-signal branch 3 times, most recently from c11f5c0 to 41dfea7 Compare June 6, 2024 12:53
@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 6, 2024

Allow the previous node to be set again as before, as we are not sure which one is used as the flag, we may need to unify the 3 edited_scene_root.

editor/editor_node.cpp Outdated Show resolved Hide resolved
Node *old_edited_scene_root = get_editor_data().get_edited_scene_root();
if (old_edited_scene_root) {
ERR_FAIL_COND_MSG(p_scene && p_scene != old_edited_scene_root && p_scene->get_parent(), "Non-null nodes that are set as edited scene should not have a parent node.");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this condition is trying to do 🤔
If p_auto_add is false, it doesn't matter if p_scene has a parent.

Copy link
Contributor Author

@Rindbee Rindbee Jun 6, 2024

Choose a reason for hiding this comment

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

This is to improve the condition when adding child:

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

add_child() does not allow the adding node already has a parent.

Put it here to prevent scene_root from removing old_edited_scene_root but not adding p_scene. The main purpose is to provide more detailed error messages instead of just relying on the error messages in add_child.

…placing_by` signal

Cannot change `scene_root`'s child node in `EditorNode::set_edited_scene()`
if replaced later using `replace_by`.
@Rindbee Rindbee force-pushed the deprecate-replacing_by-signal branch from 41dfea7 to 85a1662 Compare June 6, 2024 14:44
@akien-mga akien-mga merged commit 1fb05cb into godotengine:master Jun 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Rindbee Rindbee deleted the deprecate-replacing_by-signal branch June 7, 2024 22:21
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
4 participants