Skip to content

PackedScene: Avoid saving signal connections twice #100965

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+147 −102
Open

PackedScene: Avoid saving signal connections twice #100965

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a31298
PackedScene: Avoid saving signal connections twice
cixil Dec 31, 2024
1f93850
Merge branch 'godotengine:master' into avoid-saving-signals-twice
cixil Feb 5, 2025
5feeb11
better solution for signal packing
cixil Feb 6, 2025
7f371b3
Merge branch 'godotengine:master' into avoid-saving-signals-twice
cixil Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions editor/scene_tree_dock.cpp
View file
Original file line number Diff line number Diff line change
@@ -1328,7 +1328,8 @@ void SceneTreeDock::_tool_selected(int p_tool, bool p_confirm_override) {
undo_redo->add_do_method(node, "set_scene_file_path", "");
undo_redo->add_undo_method(node, "set_scene_file_path", node->get_scene_file_path());
_node_replace_owner(node, node, root);
_node_strip_signal_inheritance(node);
_node_replace_conn_id(node, node, root);
_node_strip_signal_inheritance(node); // FIXME: this should also be undone
NodeDock::get_singleton()->set_node(node); // Refresh.
undo_redo->add_do_method(scene_tree, "update_tree");
undo_redo->add_undo_method(scene_tree, "update_tree");
@@ -1757,6 +1758,35 @@ void SceneTreeDock::_notification(int p_what) {
} break;
}
}
void SceneTreeDock::_node_replace_conn_id(Node *p_base, Node *p_node, Node *p_root) {
// Ensures correct signals are packed for any scene involving these nodes

EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton();

int root_conn_id = p_root->_get_scene_connection_id();
int node_conn_id = p_base->_get_scene_connection_id();
undo_redo->add_do_method(p_node, "_set_scene_connection_id", root_conn_id);
undo_redo->add_undo_method(p_node, "_set_scene_connection_id", node_conn_id);

// Loop through connections to set correct connection id
List<MethodInfo> _signals;
p_node->get_signal_list(&_signals);

for (const MethodInfo &E : _signals) {
List<Node::Connection> conns;
p_node->get_signal_connection_list(E.name, &conns);

for (const Node::Connection &F : conns) {
const Node::Connection &c = F;
undo_redo->add_do_method(p_node, "_set_connection_id", E.name, c.callable, root_conn_id);
undo_redo->add_undo_method(p_node, "_set_connection_id", E.name, c.callable, node_conn_id);
}
}

for (int i = 0; i < p_node->get_child_count(); i++) {
_node_replace_conn_id(p_base, p_node->get_child(i), p_root);
}
}

void SceneTreeDock::_node_replace_owner(Node *p_base, Node *p_node, Node *p_root, ReplaceOwnerMode p_mode) {
if (p_node->get_owner() == p_base && p_node != p_root) {
@@ -1778,11 +1808,9 @@ void SceneTreeDock::_node_replace_owner(Node *p_base, Node *p_node, Node *p_root
} break;
case MODE_DO: {
undo_redo->add_do_method(p_node, "set_owner", p_root);

} break;
case MODE_UNDO: {
undo_redo->add_undo_method(p_node, "set_owner", p_root);

} break;
}
}
1 change: 1 addition & 0 deletions editor/scene_tree_dock.h
View file
Original file line number Diff line number Diff line change
@@ -194,6 +194,7 @@ class SceneTreeDock : public VBoxContainer {
MODE_UNDO
};

void _node_replace_conn_id(Node *p_base, Node *p_node, Node *p_root);
void _node_replace_owner(Node *p_base, Node *p_node, Node *p_root, ReplaceOwnerMode p_mode = MODE_BIDI);
void _node_strip_signal_inheritance(Node *p_node);
void _load_request(const String &p_path);
41 changes: 41 additions & 0 deletions scene/main/node.cpp
View file
Original file line number Diff line number Diff line change
@@ -1622,6 +1622,16 @@ void Node::_add_child_nocheck(Node *p_child, const StringName &p_name, InternalM

p_child->data.parent = this;

// set connection id of children that are not scenes
// (scenes will get their own id when instantiated)
if (p_child->get_scene_file_path() == "") {
if (get_owner()) {
p_child->_set_scene_connection_id(get_owner()->_get_scene_connection_id());
} else {
p_child->_set_scene_connection_id(_get_scene_connection_id());
}
}

if (!data.children_cache_dirty && p_internal_mode == INTERNAL_MODE_DISABLED && data.internal_children_back_count_cache == 0) {
// Special case, also add to the cached children array since its cheap.
data.children_cache.push_back(p_child);
@@ -2450,6 +2460,34 @@ int Node::get_persistent_group_count() const {
return count;
}

/* Used by PackedScene */
void Node::_set_scene_connection_id(int conn_id) {
// conn_id should never be 0
data.conn_id = conn_id;
}

int Node::_get_scene_connection_id() const {
return data.conn_id;
}

void Node::_set_connection_id(const StringName &p_signal, const Callable &p_callable, int conn_id) {
// conn_id should never be 0, that is the value returned by get_connection_id when no id is found
if (!data.connection_owners.has(p_signal)) {
HashMap<Callable, int, HashableHasher<Callable>> conn_to_owner;
data.connection_owners.insert(p_signal, conn_to_owner);
}
data.connection_owners.get(p_signal).insert(p_callable, conn_id);
}

int Node::_get_connection_id(const StringName &p_signal, const Callable &p_callable) const {
if (data.connection_owners.has(p_signal) && data.connection_owners.get(p_signal).has(p_callable)) {
return data.connection_owners.get(p_signal).get(p_callable);
}
return 0;
}

/* *** */

void Node::print_tree_pretty() {
print_line(_get_tree_string_pretty("", true));
}
@@ -3630,6 +3668,9 @@ void Node::_bind_methods() {
ClassDB::bind_method(D_METHOD("get_groups"), &Node::_get_groups);
ClassDB::bind_method(D_METHOD("set_owner", "owner"), &Node::set_owner);
ClassDB::bind_method(D_METHOD("get_owner"), &Node::get_owner);
ClassDB::bind_method(D_METHOD("_set_scene_connection_id", "conn_id"), &Node::_set_scene_connection_id);
ClassDB::bind_method(D_METHOD("_get_scene_connection_id"), &Node::_get_scene_connection_id);
ClassDB::bind_method(D_METHOD("_set_connection_id", "signal", "callable", "conn_id"), &Node::_set_connection_id);
ClassDB::bind_method(D_METHOD("get_index", "include_internal"), &Node::get_index, DEFVAL(false));
ClassDB::bind_method(D_METHOD("print_tree"), &Node::print_tree);
ClassDB::bind_method(D_METHOD("print_tree_pretty"), &Node::print_tree_pretty);
11 changes: 11 additions & 0 deletions scene/main/node.h
View file
Original file line number Diff line number Diff line change
@@ -256,6 +256,10 @@ class Node : public Object {

mutable NodePath *path_cache = nullptr;

// connection_owners and conn_id are used by PackedScene::Instantiate and PackedScene::pack (in _parse_connections)
// to ensure correct signals are saved when packing a scene
HashMap<StringName, HashMap<Callable, int, HashableHasher<Callable>>> connection_owners;
int conn_id;
} data;

Ref<MultiplayerAPI> multiplayer;
@@ -448,6 +452,13 @@ class Node : public Object {
NOTIFICATION_UNSUSPENDED = 9004
};

/* Used by PackedScene */
void _set_scene_connection_id(int conn_id);
int _get_scene_connection_id() const;

void _set_connection_id(const StringName &p_signal, const Callable &p_callable, int conn_id);
int _get_connection_id(const StringName &p_signal, const Callable &p_callable) const;

/* NODE/TREE */

StringName get_name() const;
138 changes: 43 additions & 95 deletions scene/resources/packed_scene.cpp
View file
Original file line number Diff line number Diff line change
@@ -123,6 +123,20 @@ Ref<Resource> SceneState::get_remap_resource(const Ref<Resource> &p_resource, Ha
return remap_resource;
}

int SceneState::_generate_unique_connection_id() const {
// Generate a unique enough hash to serve as a temporary connection id
OS::DateTime dt = OS::get_singleton()->get_datetime();
uint32_t hash = hash_murmur3_one_32(OS::get_singleton()->get_ticks_usec());
hash = hash_murmur3_one_32(dt.year, hash);
hash = hash_murmur3_one_32(dt.month, hash);
hash = hash_murmur3_one_32(dt.day, hash);
hash = hash_murmur3_one_32(dt.hour, hash);
hash = hash_murmur3_one_32(dt.minute, hash);
hash = hash_murmur3_one_32(dt.second, hash);
hash = hash_murmur3_one_32(Math::rand(), hash);
return hash;
}

Node *SceneState::instantiate(GenEditState p_edit_state) const {
// Nodes where instantiation failed (because something is missing.)
List<Node *> stray_instances;
@@ -137,6 +151,11 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {
p_name = ret_nodes[p_id & FLAG_MASK]; \
}

// `conn_id` serves as a kind of scene id, all nodes and connections in the same scene receive the same id
// unlike the `owner` property of nodes, the root nodes of instantiated scenes will have the same id as their children.
// When the scene is packed this unique id can be used to differentiate which signals actually belong to the scene.
int conn_id = _generate_unique_connection_id();

int nc = nodes.size();
ERR_FAIL_COND_V_MSG(nc == 0, nullptr, vformat("Failed to instantiate scene state of \"%s\", node count is 0. Make sure the PackedScene resource is valid.", path));

@@ -299,6 +318,8 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {
// may not have found the node (part of instantiated scene and removed)
// if found all is good, otherwise ignore

node->_set_scene_connection_id(conn_id);

//properties
int nprop_count = n.properties.size();
if (nprop_count) {
@@ -608,6 +629,7 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {
}

cfrom->connect(snames[c.signal], callable, CONNECT_PERSIST | c.flags | (p_edit_state == GEN_EDIT_STATE_MAIN ? 0 : CONNECT_INHERITED));
cfrom->_set_connection_id(snames[c.signal], callable, conn_id);
}

//Node *s = ret_nodes[0];
@@ -1034,23 +1056,24 @@ Error SceneState::_parse_node(Node *p_owner, Node *p_node, int p_parent_idx, Has
return OK;
}

/**
* A subfunction of `pack`.
* Recursively parses and saves connections that should be packed in the scene.
*/
Error SceneState::_parse_connections(Node *p_owner, Node *p_node, HashMap<StringName, int> &name_map, HashMap<Variant, int, VariantHasher, VariantComparator> &variant_map, HashMap<Node *, int> &node_map, HashMap<Node *, int> &nodepath_map) {
// Ignore nodes that are within a scene instance.
// Ignore nodes that are children of scene instances (and not editable).
if (p_node != p_owner && p_node->get_owner() && p_node->get_owner() != p_owner && !p_owner->is_editable_instance(p_node->get_owner())) {
return OK;
}

// Loop through connections and decide whether to pack them or not
List<MethodInfo> _signals;
p_node->get_signal_list(&_signals);
_signals.sort();

//ERR_FAIL_COND_V( !node_map.has(p_node), ERR_BUG);
//NodeData &nd = nodes[node_map[p_node]];

for (const MethodInfo &E : _signals) {
List<Node::Connection> conns;
p_node->get_signal_connection_list(E.name, &conns);

conns.sort();

for (const Node::Connection &F : conns) {
@@ -1061,15 +1084,19 @@ Error SceneState::_parse_connections(Node *p_owner, Node *p_node, HashMap<String
continue;
}

// only connections that originate or end into main saved scene are saved
// everything else is discarded

Node *target = Object::cast_to<Node>(c.callable.get_object());

if (!target) {
continue;
}

// Don't save the connection if the target is not in the tree being packed.
Node *common_parent = target->find_common_parent_with(p_node);
if (!common_parent) {
continue;
}

// Get connection callable
Vector<Variant> binds;
int unbinds = 0;
Callable base_callable;
@@ -1090,96 +1117,17 @@ Error SceneState::_parse_connections(Node *p_owner, Node *p_node, HashMap<String
base_callable = c.callable;
}

//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) {
Comment on lines 1119 to -1178
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// Don't save connections that are inherited or belong to scene instances
int conn_id = p_node->_get_connection_id(E.name, base_callable);
if (conn_id) {
if (conn_id != p_owner->_get_scene_connection_id()) {
continue;
}
}
// if there is no conn_id this connection was just added to the working scene (not through instantiation)
// which means it belongs to the current working scene and should be saved.

/// Add Connection
int src_id;

if (node_map.has(p_node)) {
@@ -1213,7 +1161,7 @@ Error SceneState::_parse_connections(Node *p_owner, Node *p_node, HashMap<String
cd.to = target_id;
cd.method = _nm_get_string(base_callable.get_method(), name_map);
cd.signal = _nm_get_string(c.signal.get_name(), name_map);
cd.flags = c.flags & ~CONNECT_INHERITED; // Do not store inherited.
cd.flags = c.flags & ~CONNECT_INHERITED; // Do not store inherited flag.
cd.unbinds = unbinds;

for (int i = 0; i < binds.size(); i++) {
3 changes: 3 additions & 0 deletions scene/resources/packed_scene.h
View file
Original file line number Diff line number Diff line change
@@ -104,8 +104,11 @@ class SceneState : public RefCounted {
#ifdef TOOLS_ENABLED
public:
typedef void (*InstantiationWarningNotify)(const String &p_warning);
#endif

private:
int _generate_unique_connection_id() const;
#ifdef TOOLS_ENABLED
static InstantiationWarningNotify instantiation_warn_notify;
#endif

21 changes: 17 additions & 4 deletions tests/scene/test_packed_scene.h
View file
Original file line number Diff line number Diff line change
@@ -75,6 +75,11 @@ TEST_CASE("[PackedScene] Signals Preserved when Packing Scene") {
main_scene_root->add_child(sub_scene_root);
sub_scene_root->set_owner(main_scene_root);

main_scene_root->_set_scene_connection_id(123);
sub_node->_set_scene_connection_id(123);
sub_scene_root->_set_scene_connection_id(456);
sub_scene_node->_set_scene_connection_id(456);

SUBCASE("Signals that should be saved") {
int main_flags = Object::CONNECT_PERSIST;
// sub node to a node in main scene
@@ -95,16 +100,24 @@ TEST_CASE("[PackedScene] Signals Preserved when Packing Scene") {
CHECK_EQ(state->get_connection_count(), 3);
}

/*
// FIXME: This subcase requires GH-48064 to be fixed.
/* TODO needs fixed
Comment on lines -99 to +103
Copy link
Member

Choose a reason for hiding this comment

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

Closing the issue does not fix this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

SUBCASE("Signals that should not be saved") {
int subscene_flags = Object::CONNECT_PERSIST | Object::CONNECT_INHERITED;
Callable p_callable;
// subscene node to itself
p_callable = callable_mp(sub_scene_node, &Node::is_ready);
sub_scene_node->connect("ready", callable_mp(sub_scene_node, &Node::is_ready), subscene_flags);
sub_scene_root->set_connection_id("ready", p_callable, 456);
// subscene node to subscene root
sub_scene_node->connect("ready", callable_mp(sub_scene_root, &Node::is_ready), subscene_flags);
p_callable = callable_mp(sub_scene_root, &Node::is_ready);
sub_scene_root->connect("ready", p_callable , subscene_flags);
sub_scene_root->set_connection_id("ready", p_callable, 456);
//subscene root to subscene root (connected within sub scene)
sub_scene_root->connect("ready", callable_mp(sub_scene_root, &Node::is_ready), subscene_flags);
p_callable = callable_mp(sub_scene_root, &Node::is_ready);
sub_scene_root->connect("ready", p_callable, subscene_flags);
sub_scene_root->set_connection_id("ready", p_callable, 456);
// Pack the scene.
Ref<PackedScene> packed_scene;