Save branch as scene saves children without owner set

[marcinn1]

Godot version

4.2.dev [44e399e]

System information

Linux

Issue description

I have node with a @tool script. The script adds child nodes needed only for the editor.

I'm saving the branch of CustomNode as a new scene, and the CSGBox saves too, which is unexpected behaviour:

The manual says clearly, that nodes without owners will not be saved to the PackedScene:

Unfortunately "Save Branch as Scene" changes ownership of all nodes. Tool script has no possibility to prevent that.

The process of "Saving Branch as Scene" and reverting it by "Make Local" isn't reversible now - it adds next child nodes each time.

Steps to reproduce

• create a new scene
• add node
• attach script:

@tool
extends Node3D

var _box = CSGBox3D.new()

func _enter_tree():
    add_child(_box)
    
func _exit_tree():
    remove_child(_box)

• save added node as a new scene
• open saved scene

Minimal reproduction project

save_branch_bug.zip

1. open the project
2. open save_branch_bug.tscn if it is not opened already
3. RMB on the CustomNode and select "Save branch as a new scene"
4. Open saved scene

You will see CSGBox in a nodes tree. Expected result: no CSGBox node visible in the tree. To make sure: click on the _CSGBox3D_<number> node and change it's position. You will notice two boxes. Should be just one.

Possible fix

The patch I pasted here previously was wrong - it was causing data loss

The old Node::duplicate_and_reown contained conditional owner setting (not sure what p_reown_map was, but if owner was NULL the new owner was not set):

-       Node *owner = get_owner();
-
-       if (p_reown_map.has(owner)) {
-               owner = p_reown_map[owner];
-       }
-
-       if (owner) {
-               NodePath p = get_path_to(owner);
-               if (owner != this) {
-                       Node *new_owner = node->get_node(p);
-                       if (new_owner) {
-                               node->set_owner(new_owner);
-                       }
-               }
-       }

The issue was probably introduced in 0559fc5.

---

EDIT: I've reverted 0559fc5 with small changes to match HEAD. The old code was working as expected. There is no saved nodes without owner, there is no duplicated boxes, and other children with owner set are saved. So this is the confirmation that 0559fc5 introduced this bug. Please revert it Can't be reverted.

[KoBeWi]

#61966 attempted to fix a similar issue. The question is whether we want to handle unowned nodes in the editor (like it was before #45937) or it's fine to have an exception, as saving branch as scene is not the same as using pack(). If we go with the latter then you are supposed to use internal children if you don't want them saved.

[marcinn1]

The question is whether we want to handle unowned nodes in the editor or it's fine to have an exceptio

Hi. I will always be in favor of consistency and predictability, mostly in terms of UI/UX (i.e. workflow), not only consistency of pack(). You have a possibility to make instanced branch local ("Make local"), and also you can save branch as a separate scene ("Save branch..."). This procedure should be reversible, i.e. saving branch as scene, then making it local again should give same result as before (and vice versa). This was possible before 0559fc5

You can also think about this that way: saving branch as a scene in fact destroys original subtree when you have some unowned childs. You're getting a modified (freezed?) copy, with all ghosts visible. This is not expected, because you, as the person using the editor, never created / touched these nodes in orginal subtree.

[KoBeWi]

I was going to suggest using internal children, but they seem bugged for whatever reason. If you add internal child from script, it still has the issue you described. I wonder how nodes with internal children (like ScrollContainer) are not affected.

[marcinn1]

I was going to suggest using internal children, but they seem bugged for whatever reason

Hmm, I'll check this again. I haven't tried adding tool nodes as internal.

Regardless, please bear in mind that subtree can be unexpectedly modified by adding unowned nodes when saving branch as a scene, i.e. the workflow "local node <-> saved branch" is not reversible anymore. This can affect other use cases. If someone really wants to save all nodes, including ghosts/unowned, I would suggest adding an "Include unowned children" checkbox somewhere, disabled by default. Disabled, because most users wants to save what they see (-> WYSIWYG).

like it was before (#45937)

So data loss in editable nodes was solved by changing the structure of a subtree? Or maybe it wasn't intended?

[KoBeWi]

I don't think it was intended. The regression was fixed afterwards, but the fixed behavior was still different.

[marcinn1]

Thanks.

I'm attaching the patch I'm currently testing. Maybe you or someone else would like to check it against regressions. It just reverts 0559fc5, so I'm not too happy with that. But most important for now is to fix the regression.

0001-Revert-of-0559fc5-as-potential-fix-for-82756.patch.txt

[KoBeWi]

Reverting that will most likely re-introduce #42611, so it's not acceptable.

[marcinn1]

it's not acceptable

Right. I've converted MRP for #42611 and reproduced the issue.

[marcinn1]

@KoBeWi There is unused duplimap which comes from 0559fc5. The solution is to use duplimap and check that the original nodes had an owner set.

[Rindbee]

It seems that the original node can be referenced to save the branch, because the nodes visible in the scene tree are all nodes with an owner. What we want to save are these visible nodes. And their owners should have the same owner as the new scene root.

godot/editor/gui/scene_tree_editor.cpp

Lines 200 to 209 in 51f81e1

	if (!display_foreign && p_node->get_owner() != get_scene_node() && p_node != get_scene_node()) {
	if ((show_enabled_subscene || can_open_instance) && p_node->get_owner() && (get_scene_node()->is_editable_instance(p_node->get_owner()))) {
	part_of_subscene = true;
	//allow
	} else {
	return;
	}
	} else {
	part_of_subscene = p_node != get_scene_node() && get_scene_node()->get_scene_inherited_state().is_valid() && get_scene_node()->get_scene_inherited_state()->find_node_by_path(get_scene_node()->get_path_to(p_node)) >= 0;
	}

[marcinn1]

Godot currently saves not only visible nodes in scene tree. It saves unvisible / unowned, too (i.e. added by editor tools). This is unexpected.

[Rindbee]

Godot currently saves not only visible nodes in scene tree. It saves unvisible / unowned, too (i.e. added by editor tools). This is unexpected.

The solution is to take advantage of the fact that they are not visible in the scene dock and exclude them.

godot/editor/scene_tree_dock.cpp

Line 2675 in 51f81e1

Node *base = selection.front()->get();

The child nodes in base store owner information.

[marcinn1]

Yes. And you can do that by checking ownership of the original node (before duplication). They're not a subscenes yet, and the logic of saving a branch is not using _add_node.

ae3a477

EDIT:
The logic based on checking ownership of the source node is taken from the code removed in #0559fc58d17c82983bfa6d37207642178932ebde So Godot worked that way before this bug was introduced.