Skip to content
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.

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

Update instances of scenes which have been reimported. #57606

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Feb 3, 2022

This is a PR meant to address the limitation that reimporting a modified scene from something like a GLTF or FBX file will not have these changes immediately reflected in instances of the currently open scenes. This should resolve most issues, the one thing it doesn't yet do is reload resources merely referenced in a reimported scene. This can be addressed by another pass to recursive walk all nodes and properties to correct any stray resources, but an easier and faster method could be to add a means to directly the override the resources in the cache.

Closes #55420
Closes #55372
Closes #14338
Closes #38853
Closes #38853

@lyuma
Copy link
Contributor

lyuma commented Feb 5, 2022

Note from yesterday:
Modifying the base glb for an inherited scene while the inherited scene is open crashes

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.alpha.custom_build (7e2586f140a3921c6a87774304b8912c670bc824)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] HashMap<Variant,List<Pair<Variant const *,Variant>,DefaultAllocator>::Element *,VariantHasher,VariantComparator,3,8>::getptr (S:\repo\godot-pr-pr\core\templates\hash_map.h:317)
[1] HashMap<Variant,List<Pair<Variant const *,Variant>,DefaultAllocator>::Element *,VariantHasher,VariantComparator,3,8>::has (S:\repo\godot-pr-pr\core\templates\hash_map.h:276)
[2] OrderedHashMap<Variant,Variant,VariantHasher,VariantComparator,3,8>::has (S:\repo\godot-pr-pr\core\templates\ordered_hash_map.h:237)
[3] Dictionary::has (S:\repo\godot-pr-pr\core\variant\dictionary.cpp:166)
[4] Object::has_meta (S:\repo\godot-pr-pr\core\object\object.cpp:931)
[5] EditorNode::get_object_icon (S:\repo\godot-pr-pr\editor\editor_node.cpp:4087)
[6] EditorNode::_update_scene_tabs (S:\repo\godot-pr-pr\editor\editor_node.cpp:332)
[7] EditorNode::_notification (S:\repo\godot-pr-pr\editor\editor_node.cpp:567)
[8] EditorNode::_notificationv (S:\repo\godot-pr-pr\editor\editor_node.h:99)
[9] Object::notification (S:\repo\godot-pr-pr\core\object\object.cpp:848)
[10] SceneTree::_notify_group_pause (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:857)
[11] SceneTree::process (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:455)
[12] Main::iteration (S:\repo\godot-pr-pr\main\main.cpp:2693)
[13] OS_Windows::run (S:\repo\godot-pr-pr\platform\windows\os_windows.cpp:629)
[14] widechar_main (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:168)
[15] _main (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:190)
[16] main (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:204)
[17] WinMain (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:218)
[18] __scrt_common_main_seh (D:\agent\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[19] BaseThreadInitThunk
-- END OF BACKTRACE --
================================================================

ERROR: Condition "!is_ancestor_of(p_node)" is true. Returning: false
at: Node::is_editable_instance (scene\main\node.cpp:1861)
WARNING: res://fox_inherited.tscn:3 - ext_resource, invalid UUID: uid://cvnixq8b1uru - using text path instead: res://Fox.glb
at: ResourceLoaderText::load (scene\resources\resource_format_text.cpp:429)
ERROR: Condition "!is_ancestor_of(p_node)" is true.
at: Node::set_editable_instance (scene\main\node.cpp:1846)
ERROR: Condition "!owner_valid" is true.
at: Node::set_owner (scene\main\node.cpp:1497)
ERROR: Condition "!owner_valid" is true.
at: Node::set_owner (scene\main\node.cpp:1497)
ERROR: Cannot get path of node as it is not in a scene tree.
at: (scene\main\node.cpp:1588)
ERROR: Condition "!is_inside_tree()" is true. Returning: false
at: Node::can_process (scene\main\node.cpp:701)
ERROR: Cannot get path of node as it is not in a scene tree.
at: (scene\main\node.cpp:1588)

@SaracenOne
Copy link
Member Author

SaracenOne commented Jun 17, 2022

Still not quite ready yet, but big update on this:

  • Fix reimporting instances inside of instances
  • Fix orphaned resource references on scene states
  • Fix reimporting instanced inherited scenes
  • Add support for preserving groups
  • Add support for preserving connections
  • Fix updating history
  • Fix recaching EditorPlugins
  • Add recache request notification for edited scene nodes
  • Fix reimporting on inherited scene when its opened directly
  • Update all resource references on properties
  • Update internal resource references inside tool script ScriptInstances (probably won't do this, would likely be better off just relying on new NOTIFICATION for now unless we can do direct resource cache overrides)
  • Add EditorSetting feature flag

@SaracenOne
Copy link
Member Author

Okay, with some recent changes I'd actually say this is almost ready. I'd quite like to add abitrary property resource remapping too, but at the very least, I'd say this 'ready' now, in that, it does what it says and doesn't seem to crash or throw errors anymore. Although this seemed like a pretty simple feature, it turned out to be a massive pain with loads of special-case bespoke code required to make the reimporting appear as seamless as possible. Lots of bespoke edge-cases which I had to track down and fix, and while I've debugged this pretty intensely already, this will likely still require some meaningful testing. At the very least though, I've added an editor setting feature flag so we can treat it as a more experimental feature if needed.

@SaracenOne SaracenOne force-pushed the update_on_reimport branch 5 times, most recently from b78f7e2 to af68ba4 Compare June 21, 2022 06:48
@SoapSpangledGames
Copy link

Does this also handle the case where a glTF (or other file type) is made local to the scene (or children made editable), then re-exported from Blender? All the replies I've read mention inheriting a new scene, but not when the model is made local to an existing scene or when the children are made editable.

My workflow is:

  1. Drag a model from the FileSystem tab to the Scene tab.
  2. Make the model local.
  3. Add BoneAttachment3D nodes as needed to the now local Skeleton3D node.
  4. Add children to the BoneAttachment3D nodes, changing the transform of the children as needed.
  5. Create AnimationTree sequences based on the imported AnimationPlayer.

If I then add a new animation to a model in Blender, and re-export the file, Godot will re-import the file without the new animation. I have to delete the model from the scene, restart Godot, then repeat steps 1-5. Changing scene tabs doesn't help, closing and re-opening the scene doesn't help, restarting Godot without deleting the model from the scene doesn't help.

Having Godot pick up the animation changes automatically would be awesome, and would save me tons of time.

I'm using Godot 4.0 alpha 11.

@fire fire force-pushed the update_on_reimport branch 2 times, most recently from 70b9aca to 26568b2 Compare December 27, 2022 18:22
@fire
Copy link
Member

fire commented Dec 27, 2022

Found a bug.

node_3d->get_global_transform(); becomes node_3d->get_relative_transform(node_3d->get_owner()); Similar for 2d.

The error case is the origin is different when called getting global transform (universe) versus it's local "global" transform (scene global) in nested scenes.

@fire
Copy link
Member

fire commented Dec 27, 2022

Test plan global transforms:

  1. Open blender.
  2. Put a default monkey in the center
  3. Save the scene as a blend
  4. Go to this enhancement executable
  5. Make a new 3d scene in a new project
  6. Instance the blend file path into the scene
  7. When you move the monkey in blender and save
  8. When you go back to godot it should update.

Variations

  1. Go to blender set the monkey to be 10 on the right side
  2. Make a child scene and make instance the blend scene
  3. Go to the child scene and make the root 10 units to the top
  4. Add a main scene with the nested scene and make parent node 10 units forward.
  5. Move the Blender scene monkey forward 10 units.

@fire fire force-pushed the update_on_reimport branch 3 times, most recently from 1c13e80 to d99916e Compare December 27, 2022 19:26
@fire
Copy link
Member

fire commented Dec 27, 2022

I patched. So the change was I calculated the transform of the node 2d and node 3d like new_additive_node_entry.transform_3d = node_3d->get_relative_transform(node_3d->get_parent()); and saved it in the data structure. Notice that it is a local transform. Not a global transform. Also removed the editor flag to disable reloading.

There was also a missing path to that scene that is saved when a node is instanced.

@fire

This comment was marked as outdated.

@fire fire force-pushed the update_on_reimport branch 2 times, most recently from b9e7c35 to d8cbe4d Compare December 27, 2022 19:51
@@ -3874,7 +3876,7 @@ Error EditorNode::load_scene(const String &p_scene, bool p_ignore_broken_deps, b
Ref<SceneState> state = sdata->get_state();
state->set_path(lpath);
new_scene->set_scene_inherited_state(state);
new_scene->set_scene_file_path(String());
new_scene->set_scene_file_path(lpath);
Copy link
Member

Choose a reason for hiding this comment

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

@godotengine/import Does anyone know what this does? I did this to cover a case where opening a nested scene file loses a reference to the blend file as per the test case when updating.

@reduz
Copy link
Member

reduz commented Jan 23, 2023

The approach is very interesting, although EditorNode is need of a massive clean up after 4.0.

@akien-mga akien-mga merged commit 11e2278 into godotengine:master Jan 23, 2023
@akien-mga
Copy link
Member

Thanks!

@Flavelius
Copy link
Contributor

Reimporting a .blend file crashes the editor for me (4.0 beta15), but only if the scene is used in any currently open scenes, so i guess this is related to this change. Is this a known issue or unsupported special case with imported .blends?

@PastMoments
Copy link
Contributor

Just want to confirm. This change only fixes the reimport issue #38853 for Godot 4 right?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment