PackedScene: Avoid saving signal connections twice #100965

cixil · 2024-12-31T01:25:36Z

Summary

Fixes #48064 fixes #86532 fixes #85372 (all variations of the same issue)
(could also close #42161, but no way to test)

When packing a tree of nodes to a PackedScene, signal connections were getting saved twice in some circumstances, when they should have been ignored.

See PR update below for a better explanation of the problem and solution.

PR history

I originally made PR #97303 which was partially reverted by #100235 because it inadvertently caused #100097.

~~This fixes #48064 without causing #100097 using @KoBeWi's suggestion from 100097.~~

~~It changes the line committed in #66289 which fixed 66154 to use GEN_EDIT_STATE_MAIN instead of GEN_EDIT_STATE_INSTANCE, I've verified this change does not reopen #66154~~

cixil · 2024-12-31T01:30:24Z

@akien-mga

editor/export/editor_export_platform.cpp

RandomShaper

Somehow TIWAGOSly, I'm leaving my approval as the changes look good to me and I gues it will help keep things moving forward. It'd be great to have this thing covered in tests, though.

akien-mga · 2025-01-08T11:30:43Z

Could you amend the commit message to formatted like a normal sentence/commit title? E.g. like this PR's title.

KoBeWi · 2025-01-08T23:03:43Z

This causes the issue I mentioned in #100097 (comment)
In my plugin I remove part of the scene, pack it and then re-instantiate (sort of built-in PackedScene). This PR makes the signals stripped with this operation.
MRP:
Brokensignals.zip
Open the Bug bottom panel and click the buttons.

I'm not sure how this could be resolved though. Maybe don't apply CONNECT_INHERITED when edit state is 0? Though it's such a corner-case that it's probably not blocking this PR.
My workaround is manually stripping the flag from signals. It's unexposed though, which makes it even more hacky.

cixil · 2025-01-09T17:21:32Z

This causes the issue I mentioned in #100097 (comment)

I feel like its better practice to save the button as a separate scene and just instantiate that, which solves the issue. Unless that somehow doesn't work for your original use case

@export var button_scene:PackedScene
...
	for i in 10:
		add_child(button_scene.instantiate())

But I'd hate to break something else by fixing something. This probably deserves a little bit more testing to see if this case affects any other scenarios before its merged.

cixil · 2025-01-09T17:23:23Z

Did #100160 solve this case?

KoBeWi · 2025-01-09T20:36:20Z

Yes.

I feel like its better practice to save the button as a separate scene and just instantiate that

The point is to avoid excessive scenes, like with built-in scripts. It's a niche use-case though, so it's something that can be fixed later (or not, as there is workaround).

cixil · 2025-02-03T15:13:55Z

I have not found a resolution to that bug but I think I have found other places where connections are falsely ignored. This PR should NOT be merged until it is updated .

cixil · 2025-02-06T15:06:09Z

PR Update

I redid the whole PR. The current solution fixes the issues and does not cause the bug mentioned by Kobewi above.

Problem

Signals are being saved to packed scene when they should be ignored in these cases:

connection from editable instance child to other editable instance child/root (made within the instance, not the scene they are editable to)
connection from root of instanced scene to itself or its children (within the instance)

Reasoning

Consider the case where the root node of an instanced scene has a signal connected to itself. There can be such a connection within the scene itself, and also such a connection made by the scene containing it. Therefor when packing the scene, there must be a way to distinguish which scene a connection belongs to based on the connection itself, not just the node it comes from and goes to.

Solution

When instantiate is called, a unique id for the scene is assigned to and shared by every node and signal connection in that scene. There is one unique id per scene instance. This id is different from owner as it is also applied to connections, it doesn't require nodes to be in the SceneTree, and a scene owner node will have the same id as the nodes it "owns". If a node is packed as a scene, only connections with the same id as that node are packed, thus ignoring any connections that belong to other scenes.

cixil · 2025-02-06T17:30:48Z

This is ready for feedback.

The code is finished but the tests for signal packing from my old PR need rewritten. I wanted to get feedback that this is a viable solution before writing tests.

KoBeWi · 2025-03-15T19:09:39Z

tests/scene/test_packed_scene.h

-	// FIXME: This subcase requires GH-48064 to be fixed.
+	/* TODO needs fixed


Closing the issue does not fix this case?

I started working on tests/scene/test_packed_scene.h but decided to wait for feedback on the rest of the code because I wasn't sure if the solution in this PR would be too complex.

so tests/scene/test_packed_scene.h should be ignored for now

The tests were for a solution that used CONNECT_INHERITED, so they won't work for this PR now

cixil · 2025-04-06T00:56:32Z

scene/resources/packed_scene.cpp


-			//find if this connection already exists
-			Node *common_parent = target->find_common_parent_with(p_node);
-
-			ERR_CONTINUE(!common_parent);
-
-			if (common_parent != p_owner && common_parent->get_scene_file_path().is_empty()) {
-				common_parent = common_parent->get_owner();
-			}
-
-			bool exists = false;
-
-			//go through ownership chain to see if this exists
-			while (common_parent) {
-				Ref<SceneState> ps;
-
-				if (common_parent == p_owner) {
-					ps = common_parent->get_scene_inherited_state();
-				} else {
-					ps = common_parent->get_scene_instance_state();
-				}
-
-				if (ps.is_valid()) {
-					NodePath signal_from = common_parent->get_path_to(p_node);
-					NodePath signal_to = common_parent->get_path_to(target);
-
-					if (ps->has_connection(signal_from, c.signal.get_name(), signal_to, base_callable.get_method())) {
-						exists = true;
-						break;
-					}
-				}
-
-				if (common_parent == p_owner) {
-					break;
-				} else {
-					common_parent = common_parent->get_owner();
-				}
-			}
-
-			if (exists) { //already exists (comes from instance or inheritance), so don't save
-				continue;
-			}
-
-			{
-				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) {


worth mentioning that this code seems to solve the issue for the editor since there's never an error when saving a scene in the editor. When packing a scene it checks to see if the connection was already saved, and if it was, not pack it again.

When calling pack from code, however, this doesn't work as the connections are saved multiple times. I think scene packing was originally just meant to be used by the editor, not in code, which could be why this bug was not caught when this code was written. I admittedly did not figure out exactly why this block wasn't working despite spending a couple days trying to debug it.

However I've been using a custom build of 4.4 with the changes in this PR almost daily for about a month now without any problems.

KoBeWi · 2025-05-07T11:59:26Z

fixes #86532

It doesn't, the scene still gets spammed with invalid connections.

cixil requested a review from a team as a code owner December 31, 2024 01:25

cixil changed the title ~~avoid-saving-signals-twice~~ Avoid saving signals twice Dec 31, 2024

Chaosus added bug topic:core labels Dec 31, 2024

Chaosus added this to the 4.4 milestone Dec 31, 2024

KoBeWi mentioned this pull request Dec 31, 2024

Use CONNECT_INHERITED only in edited scene #100160

Open

cixil commented Jan 3, 2025

View reviewed changes

editor/export/editor_export_platform.cpp Outdated Show resolved Hide resolved

akien-mga requested review from KoBeWi and a team January 7, 2025 22:02

RandomShaper approved these changes Jan 8, 2025

View reviewed changes

akien-mga changed the title ~~Avoid saving signals twice~~ PackedScene: Avoid saving signal connections twice Jan 8, 2025

akien-mga added the needs testing label Jan 8, 2025

cixil force-pushed the avoid-saving-signals-twice branch from 2f80214 to 9bac8bb Compare January 8, 2025 15:31

PackedScene: Avoid saving signal connections twice

2a31298

cixil force-pushed the avoid-saving-signals-twice branch from 9bac8bb to 2a31298 Compare January 8, 2025 15:34

This comment was marked as outdated.

Sign in to view

akien-mga modified the milestones: 4.4, 4.5 Jan 28, 2025

This comment was marked as outdated.

Sign in to view

cixil mentioned this pull request Feb 4, 2025

PackedScene Instantiate from Tool Script is preserving Signals when saved #86532

Open

cixil changed the title ~~PackedScene: Avoid saving signal connections twice~~ WIP: PackedScene: Avoid saving signal connections twice Feb 4, 2025

Merge branch 'godotengine:master' into avoid-saving-signals-twice

1f93850

cixil requested a review from a team as a code owner February 6, 2025 04:18

cixil requested a review from RandomShaper February 6, 2025 19:28

cixil marked this pull request as draft February 6, 2025 19:43

better solution for signal packing

5feeb11

cixil force-pushed the avoid-saving-signals-twice branch from abe2378 to 5feeb11 Compare February 6, 2025 23:02

cixil changed the title ~~WIP: PackedScene: Avoid saving signal connections twice~~ Want feedback: PackedScene: Avoid saving signal connections twice Feb 7, 2025

cixil marked this pull request as ready for review February 14, 2025 19:39

This was referenced Mar 10, 2025

"Signal already connected" error for inherited scenes Godot3.2 #42161

Open

Avoid duplicating signals from scene instances into packed scenes (reverted) #97303

Merged

Merge branch 'godotengine:master' into avoid-saving-signals-twice

7f371b3

KoBeWi reviewed Mar 15, 2025

View reviewed changes

cixil changed the title ~~Want feedback: PackedScene: Avoid saving signal connections twice~~ PackedScene: Avoid saving signal connections twice Mar 28, 2025

cixil commented Apr 6, 2025

View reviewed changes

		// FIXME: This subcase requires GH-48064 to be fixed.
		/* TODO needs fixed

Uh oh!

PackedScene: Avoid saving signal connections twice #100965

Are you sure you want to change the base?

PackedScene: Avoid saving signal connections twice #100965

Conversation

cixil commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

PR history

Uh oh!

cixil commented Dec 31, 2024

Uh oh!

Uh oh!

RandomShaper left a comment

Choose a reason for hiding this comment

Uh oh!

akien-mga commented Jan 8, 2025

Uh oh!

This comment was marked as outdated.

KoBeWi commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cixil commented Jan 9, 2025

Uh oh!

cixil commented Jan 9, 2025

Uh oh!

KoBeWi commented Jan 9, 2025

Uh oh!

This comment was marked as outdated.

cixil commented Feb 3, 2025

Uh oh!

cixil commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Update

Problem

Reasoning

Solution

Uh oh!

cixil commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi Mar 15, 2025

Choose a reason for hiding this comment

Uh oh!

cixil Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

cixil Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

cixil Apr 6, 2025

Choose a reason for hiding this comment

Uh oh!

KoBeWi commented May 7, 2025

Uh oh!

Uh oh!

cixil commented Dec 31, 2024 •

edited

Loading

KoBeWi commented Jan 8, 2025 •

edited

Loading

cixil commented Feb 6, 2025 •

edited

Loading

cixil commented Feb 6, 2025 •

edited

Loading