Fix instanced .blend/GLTF scenes lose all children after update until .tscn is reopened

[yahkr]

closes #94056

When making modifications to a .blend file that has been instantiated into the scene, upon re import any child nodes that were previously there disappear until the scene is reloaded.

removing this condition fixes the issue:

godot/editor/editor_node.cpp

Line 4345 in e57312d

if (p_node->get_parent()->get_owner() != nullptr && p_node->get_parent()->get_owner() != p_edited_scene) {

[SaracenOne]

Looking over this, I think this is an okay fix. I can only assume the checks here were an artefact of an early iteration of the system and that this bug occured due to testing mainly with nodes parented to editable children.

[Hilderin]

I tested this PR and it seems to work nicely.

I just found a little side-effect and I think that why the condition on the ownership was there. If the child added is another scene with child, I get the error: Invalid owner. Owner must be an ancestor in the tree. when reimporting the .blend file.

There is a simple modified version of the MRP:
test-godot-blender-reimport-missing-node.zip

[yahkr]

I tested this PR and it seems to work nicely.

I just found a little side-effect and I think that why the condition on the ownership was there. If the child added is another scene with child, I get the error: Invalid owner. Owner must be an ancestor in the tree. when reimporting the .blend file.

Modifying the original condition to the following partially fixes this issue, however it just pushes the issue down.

if (p_node->get_owner() == p_edited_scene) {

if you add a child to CSGBox3D I'm seeing the same error, will continue to dig, it appears the parent of the current_ancestor (Label3D2) isnt being set properly.

godot/editor/editor_node.cpp

Line 6011 in e57312d

current_ancestor->set_owner(ancestor_owner);

[yahkr]

Ok, i believe I found the issue, it has to do with the ownership_table. Please correct me if there is something I'm missing.

with the new condition:

if (p_node->get_owner() == p_edited_scene) {

godot/editor/editor_node.cpp

Lines 4361 to 4368 in e57312d

	// Gathers the ownership of all ancestor nodes for later use.
	HashMap<Node *, Node *> ownership_table;
	for (int i = 0; i < p_node->get_child_count(); i++) {
	Node *child = p_node->get_child(i);
	update_ownership_table_for_addition_node_ancestors(child, ownership_table);
	}
	new_additive_node_entry.ownership_table = ownership_table;

This will add any and all children of the node. However, now that we are not skipping "p_node->get_parent()->get_owner() != p_edited_scene" it attempts to process the children of instantiated scenes. in doing so it populates some ownership_tables with NULL values for parent/owner, which then breaks

godot/editor/editor_node.cpp

Lines 6002 to 6012 in e57312d

	// Restore the ownership of its ancestors
	for (KeyValue<Node *, Node *> &E : additive_node_entry.ownership_table) {
	Node *current_ancestor = E.key;
	Node *ancestor_owner = E.value;
	if (ancestor_owner == original_node) {
	ancestor_owner = instantiated_node;
	}
	current_ancestor->set_owner(ancestor_owner);
	}

So far in my testing removing these sections has not caused any other side effects, but would love another opinion/review.

These changes are pushed to this pr.
New MRP:
94093-fix-3.zip

[Hilderin]

Seriously good job! I tested with a lot of different situations and your last modifications seem to work perfectly. Effectively, changing owner should not be required since moving object in the scene does not change the owner anyway.

I tested these situations:

• Added node in Godot to Blender object
• Multiple levels of added nodes
• Multiple levels on nodes from Blender with added nodes in Godot
• Moving nodes in Blender
• Modified properties on Blender objects
• Modified properties on added Godot objects
• Signals on Blender objects
• Reference to an Blender object in a script on an added Node
• Reference to an Blender object in a script on an added Blender object
• Reference to an child of an added Scene in a script on an added Blender object
• Removing or renaming nodes in Blender with attached script, child added node

[SaracenOne]

Since this removes more things now, I'll test this tomorrow and get back to you.

[SaracenOne]

Also, it is possible that the ownership tracking might be an artefact of a more naive earlier implementation, but I'm a little skeptical that this wouldn't break something else. I would recommend testing reloading inherited scenes, since was an area I remember struggling to get behaving correctly.

[yahkr]

Also, it is possible that the ownership tracking might be an artefact of a more naive earlier implementation, but I'm a little skeptical that this wouldn't break something else. I would recommend testing reloading inherited scenes, since was an area I remember struggling to get behaving correctly.

Yup, inherited totally broken until the scene is reloaded, but rolling back this PR the issue persists. Will tinker with that today.

[yahkr]

To add to the oddities, the blend updates dont always work when an inherited scene and it's base scene are open and you update a .blend.

Steps to recreate:
1. Open simple_base.tscn, 
2. Observe updates to .blend work fine
3. Open simple_inherited.tscn
4. Update blend again
5. See blend_child vanish in simple_inherited
6. Go back to simple_base.tscn
7. Update blend and see no change 😠

This appears only an issue when the .blend is instanced inside an inherited scene. as I tried the same process with simple_base.tscn and complex.tscn. Updating blend and switching back and forth works perfectly fine.

94093-fix-4.zip

[Hilderin]

I'll look into your last MRP, maybe together we can find a miracle solution :)

[Hilderin]

Wow, ok, there are weird things about ownership I did not known about.
Added nodes in base scene have the inherited scene as owner.

That cause problem in update_reimported_diff_data_for_node because node_part_of_subscene is set to true causing the node to never be added in p_addition_list.

I changed this condition and now the children "blend_child" is ok when reloading the scene:

bool node_part_of_subscene = p_node != p_edited_scene &&
		p_edited_scene->get_scene_inherited_state().is_valid() &&
		p_edited_scene->get_scene_inherited_state()->find_node_by_path(p_edited_scene->get_path_to(p_node)) >= 0 &&
		p_node->get_owner() != p_edited_scene;

I publish a branch for you to see my modifications:
1fcb52c

[yahkr]

Awesome! Is that screenshot taken after the change to test.blend?

I did stumble across that change to node_part_of_subscene, but was too focused on the lack of updating to notice blend_child didn't vanish. So one problem down, one to go. It's almost like the instantiated node gets de-referenced and thus doesn't get updated.

I suspect this has to do with an instance of it being in the inherited scene and getting its owner or some other property updated (which affects the base scene for some reason) and so it no longer receives the signal to update? just a theory.

[Hilderin]

The screenshot was taken before the reload. I'll now focus on the problem of scene not reloading. I don't known where it could come from at this moment.

[Hilderin]

I think I understand why the scenes are not updated when the 2 scenes are opened. The problem is that on the inherited scene, the get_modified_properties_for_node is populated with the transform and the mesh because EditorPropertyRevert::get_property_revert_value returns the value of the base scene and not the value from blender. Theses props are updated on scene reload in the inherited scene with the old values and it seems that both MeshInstance3D in the two scene are using the same RID for the mesh. So updating the mesh in the inherited scene cause the mesh to revert back to the old one in the base scene. Same think for the transform.
I'm not sure how to fix EditorPropertyRevert::get_property_revert_value, I'm looking into it.

[Hilderin]

After a while, I was able to fix the remaining problems (that I found!). It’s a lot of modifications to manage the inherited scenes correctly.

It's all in this branch: https://github.com/Hilderin/godot/tree/fix-reimport-missing-node

Fixed problems:

• Mesh not refreshing & properties reverting in inherited scene: When the MeshInstance is in an inherited scene, the comparison between the old and the new mesh was done after the mesh was updated in the node. That caused issues with EditorPropertyRevert::get_property_revert_value. I fixed this by adding the signal resources_reimporting in EditorFileSystem and pre-calculating the modified properties before the reimportation.
• Inherited scene containing a Blender node: Properties were lost when reloading. This was mainly because it's difficult to only get the modified properties and initialize a new node with exactly the right properties. My solution was to instantiate the whole base scene and grab the Blender node from it already initialized. I did not find a way to only initialize one node from a PackedScene.
• Direct inheritance of the Blender scene: The reload was not working. Previously, a new instance of current_packed_scene was created to replace the edited scene. The problem is current_packed_scene contains all the added nodes as well, so after a reload, all added nodes were duplicated. I calculated a base_packed_scene, which is the PackedScene from the base scene (one level down), and used a new instance of it to replace the current edited scene (this also solves multiple levels of inheritance).

Tested:

• Added node in Godot to Blender object
• Multiple levels of added nodes
• Multiple levels of nodes from Blender with added nodes in Godot
• Moving nodes in Blender
• Modified properties on Blender objects
• Modified properties on added Godot objects
• Signals on Blender objects
• Reference to a Blender object in a script on an added node
• Reference to a Blender object in a script on an added Blender object
• Reference to a child of an added scene in a script on an added Blender object
• Removing or renaming nodes in Blender with attached scripts or child added nodes
• Inherited scene from Blender object
• Editing inherited scenes from Blender object with added nodes and updated properties
• Adding inherited scenes from Blender object to a scene and adding nodes and updating properties
• Multiple nodes of the same scene or inherited scene in the same scene
• Multiple opened scenes with the Blender object in their tree
• Multilevel inheritance with a Blender object

Some demo:

blender_Cgm6Ud8OT6.mp4

Test project:
test-godot-blender-reimport-missing-node-v4.zip

[AThousandShips]

You included an unrelated commit into your history, please clean that up using rebasing and removing the incorrect commit, see here

[yahkr]

After a while, I was able to fix the remaining problems (that I found!). It’s a lot of modifications to manage the inherited scenes correctly.
...

Good stuff!! What a doozy of an issue. Pulled in your changes.
Reconfirming your tested cases and so far so good 🎉

[KoBeWi]

I'm not very familiar with scene import code, but this fixes the issue without causing visible regressions.

[Hilderin]

@Yahkub-R Are you ok to make these modifications or you prefer I create a PR to your branch?

[akien-mga]

Could you amend the commit title to be more explicit? It shouldn't refer to the issue but actually say what problem it's solving (the PR title is a good option "Fix instanced .blend/GLTF scenes lose all children after update until .tscn is reopened").

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[Hilderin]

Thanks @Yahkub-R for your help and teamwork!

[yahkr]

Thanks @Yahkub-R for your help and teamwork!

You too @Hilderin! Good stuff