Fix unable to determine which scene a connection belongs to when packing outside the editor #85606

Rindbee · 2023-12-01T12:25:41Z

Previously, the lack of scene inherited/instance state outside the editor made it impossible to determine which scene a connection in a descendant scene with editable children enabled belonged to. This may result in connections already recorded in a descendant scene being recorded again into a new PackedScene when packed outside the editor.

Exclude two cases to speed up parse:

A scene root that does not have editable children enabled does not have to parse its child nodes.
It is not necessary to parse descendant nodes where p_owner is not in the descendant node's owner chain.

Fix #85372, fix partially #48064.

KoBeWi

This does fix the issues and is probably alright, but I don't really understand what GEN_EDIT_STATE_DISABLED is for. The PR makes it technically unused (the only "use" is that you can't use another EDIT STATE in non-editor builds).

Rindbee · 2023-12-14T11:18:16Z

Squeezed the commit.

YuriSizov · 2023-12-18T15:34:27Z

I don't really understand what GEN_EDIT_STATE_DISABLED is for. The PR makes it technically unused (the only "use" is that you can't use another EDIT STATE in non-editor builds).

Seems like it always has been a thing. Let's try asking @reduz, maybe he can remember the use case for this.

reduz · 2024-02-14T15:56:41Z

This PR does not make a lot of sense honestly, you don't want to keep the instance state around when running outside the editor (this has a memory/performance impact) so keeping it just to fix a very corner case does not seem like the right way to solve it.

reduz · 2024-02-14T15:59:39Z

My feeling is that you probably want to instantiate in the right mode yourself when running outside the editor (by forcefully generating the instance state). If you don't, then packing will not work as expected and I understand this is on purpose.

Change is not alright, it keeps instance states around during game, which is unintended.

Rindbee · 2024-02-15T15:13:09Z

This PR does not make a lot of sense honestly, you don't want to keep the instance state around when running outside the editor (this has a memory/performance impact) so keeping it just to fix a very corner case does not seem like the right way to solve it.

In this case, it appears that the behavior described in #48064 will be an expected (tolerable) behavior.

Maybe I should get the sub-scene's instance states by instantiating the sub-scene file again.

Rindbee · 2024-02-17T08:28:31Z

scene/main/node.cpp

@@ -1998,7 +1998,7 @@ void Node::_clean_up_owner() {
 	data.OW = nullptr;
 }

-Node *Node::find_common_parent_with(const Node *p_node) const {
+Node *Node::find_common_owner_with(const Node *p_node) const {


The original method find_common_parent_with() is used as a pre-step to find the common owners.

But looking up the common owner along the ownership chain seems to be more efficient, especially when editable_children is enabled. For example when checking for nodes belonging to the main scene that are added under the nodes of a sub-scene (deeply nested).

Rindbee · 2024-02-17T08:32:20Z

scene/resources/packed_scene.cpp

-			{
-				Node *nl = p_node;
-
-				bool exists2 = false;
-
-				while (nl) {
-					if (nl == p_owner) {
-						Ref<SceneState> state = nl->get_scene_inherited_state();
-						if (state.is_valid()) {
-							int from_node = state->find_node_by_path(nl->get_path_to(p_node));
-							int to_node = state->find_node_by_path(nl->get_path_to(target));
-
-							if (from_node >= 0 && to_node >= 0) {
-								//this one has state for this node, save
-								if (state->is_connection(from_node, c.signal.get_name(), to_node, base_callable.get_method())) {
-									exists2 = true;
-									break;
-								}
-							}
-						}
-
-						nl = nullptr;
-					} else {
-						if (!nl->get_scene_file_path().is_empty()) {
-							//is an instance
-							Ref<SceneState> state = nl->get_scene_instance_state();
-							if (state.is_valid()) {
-								int from_node = state->find_node_by_path(nl->get_path_to(p_node));
-								int to_node = state->find_node_by_path(nl->get_path_to(target));
-
-								if (from_node >= 0 && to_node >= 0) {
-									//this one has state for this node, save
-									if (state->is_connection(from_node, c.signal.get_name(), to_node, base_callable.get_method())) {
-										exists2 = true;
-										break;
-									}
-								}
-							}
-						}
-						nl = nl->get_owner();
-					}
-				}
-
-				if (exists2) {
-					continue;
-				}
-			}
-


This code seems to have the same effect as the previous check along the ownership chain. Doesn't seem to catch any different cases.

…ing outside the editor Previously, the lack of scene inherited/instance state outside the editor made it impossible to determine which scene a connection in a descendant scene with editable children enabled belonged to. This may result in connections already recorded in a descendant scene being recorded again into a new `PackedScene` when packed outside the editor. Exclude two cases to speed up parse: 1. A scene root that does not have editable children enabled does not have to parse its child nodes. 2. It is not necessary to parse descendant nodes where `p_owner` is not in the descendant node's owner chain.

Rindbee · 2024-02-19T02:07:39Z

scene/resources/packed_scene.cpp

-			//signal and method strings are stored..
-
-			for (int i = 0; i < connections.size(); i++) {
-				if (connections[i].from == p_node && connections[i].to == p_to_node && connections[i].signal == signal_idx && connections[i].method == method_idx) {


has_connection() has the same purpose as is_connection(), but connections[i].from may also be an masked index in names, so the results of has_connection() may be more correct.

cixil · 2024-09-22T00:49:38Z

Hi I may have a simpler solution to the signal duplication bugs: #97303

I just noticed this PR after submitting mine and I'm not sure what the state of it is, but I wanted to link mine here because having the same reviewers might be helpful.

akien-mga requested review from KoBeWi and SaracenOne December 1, 2023 12:35

akien-mga added bug topic:core labels Dec 1, 2023

akien-mga added this to the 4.3 milestone Dec 1, 2023

akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 1, 2023

Rindbee marked this pull request as draft December 1, 2023 12:50

Rindbee force-pushed the fix-parsing-connections branch from 26f2313 to 788c8d1 Compare December 2, 2023 04:48

Rindbee changed the title ~~Don't parse connections in sub-scenes~~ Fix a connection being parsed multiple times when packing Dec 2, 2023

Rindbee force-pushed the fix-parsing-connections branch from 788c8d1 to 9fd3a65 Compare December 2, 2023 11:21

Rindbee changed the title ~~Fix a connection being parsed multiple times when packing~~ Fix unable to determine which scene a connection belongs to when packing outside the editor Dec 2, 2023

Rindbee marked this pull request as ready for review December 2, 2023 11:24

Rindbee force-pushed the fix-parsing-connections branch from 9fd3a65 to ba34815 Compare December 2, 2023 13:15

KoBeWi previously approved these changes Dec 12, 2023

View reviewed changes

Rindbee force-pushed the fix-parsing-connections branch from ba34815 to 356b0f4 Compare December 14, 2023 11:17

Rindbee force-pushed the fix-parsing-connections branch from 356b0f4 to 3d55c8e Compare February 17, 2024 08:09

Rindbee requested a review from a team as a code owner February 17, 2024 08:09

Rindbee force-pushed the fix-parsing-connections branch 2 times, most recently from aee4bcf to d7b7108 Compare February 17, 2024 08:24

Rindbee commented Feb 17, 2024

View reviewed changes

Rindbee force-pushed the fix-parsing-connections branch from d7b7108 to aa95b22 Compare February 19, 2024 01:59

Rindbee commented Feb 19, 2024

View reviewed changes

KoBeWi removed this from the 4.3 milestone Aug 3, 2024

KoBeWi added this to the 4.4 milestone Aug 3, 2024

KoBeWi added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 3, 2024

KoBeWi mentioned this pull request Dec 7, 2024

4.4-dev6: Signals don't work in exported builds if .godot dir was created by dev6 #100097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unable to determine which scene a connection belongs to when packing outside the editor #85606

Fix unable to determine which scene a connection belongs to when packing outside the editor #85606

Rindbee commented Dec 1, 2023 •

edited

Loading

KoBeWi left a comment

Rindbee commented Dec 14, 2023

YuriSizov commented Dec 18, 2023

reduz commented Feb 14, 2024 •

edited

Loading

reduz commented Feb 14, 2024

Rindbee commented Feb 15, 2024 •

edited

Loading

Rindbee Feb 17, 2024 •

edited

Loading

Rindbee Feb 17, 2024 •

edited

Loading

Rindbee Feb 19, 2024 •

edited

Loading

cixil commented Sep 22, 2024

Fix unable to determine which scene a connection belongs to when packing outside the editor #85606

Are you sure you want to change the base?

Fix unable to determine which scene a connection belongs to when packing outside the editor #85606

Conversation

Rindbee commented Dec 1, 2023 • edited Loading

KoBeWi left a comment

Choose a reason for hiding this comment

Rindbee commented Dec 14, 2023

YuriSizov commented Dec 18, 2023

reduz commented Feb 14, 2024 • edited Loading

reduz commented Feb 14, 2024

Rindbee commented Feb 15, 2024 • edited Loading

Rindbee Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Rindbee Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Rindbee Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

cixil commented Sep 22, 2024

Rindbee commented Dec 1, 2023 •

edited

Loading

reduz commented Feb 14, 2024 •

edited

Loading

Rindbee commented Feb 15, 2024 •

edited

Loading

Rindbee Feb 17, 2024 •

edited

Loading

Rindbee Feb 17, 2024 •

edited

Loading

Rindbee Feb 19, 2024 •

edited

Loading