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

Make reimported models reimport their owner. #96144

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Aug 27, 2024

First PR in an attempt to triage #91211. Does not fix the whole issue, but does address an important part of it.

Changes the behaviour of the scene hot-reload system so that if the scene which needs to be reimported is owned by another instance, reload that instance instead.

The reason for this is the issue uncovered another oversight in the reimport system. The original approach walked the nodes in each editable scene and made a copy of every scene which required reimport and attempted to reconstruct it as accurately as possible, retaining additional nodes and properties edits. The properties are determined by checking with the standard EditorPropertyRevert::get_property_revert_value and PropertyUtils::is_property_value_different methods. However, the problem arises if your reimported scene is part of another instance, and that instance has its own set of property modifications to the reimported scene. These will appear lost upon reimport since those methods are checking the property values relative to the currently edited scenes but will ignore any changes made by their owned instances.

Fortunately, the fix for this seems fairly straightforward: if a reimported scene is owned by an instance other than the edited scene, mark that instance to be reimported instead. The current code will still walk that instance and preserve all the current changes. Other smaller changes are keeping a full HashMap of nodes marked as editable and folded, and a few typo corrections in the comments.

Still in draft for now since this will need to go through more testing, but I'm reasonably confident that this is the right approach. It may also prove to be an optimization for certain large scenes.

@SaracenOne SaracenOne added this to the 4.4 milestone Aug 27, 2024
@SaracenOne SaracenOne force-pushed the reimport_owner_instance branch 2 times, most recently from 781eb06 to 56766d7 Compare September 2, 2024 01:25
@SaracenOne SaracenOne marked this pull request as ready for review September 2, 2024 01:28
@SaracenOne SaracenOne requested a review from Hilderin September 2, 2024 01:33
Copy link
Contributor

@Hilderin Hilderin left a comment

Choose a reason for hiding this comment

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

Real nice works! Another edge case solved for the scene reloading!! Tested it with success.
I added this situation in the MRP I used to test scene reloading. I created the scene container_editable containing the blender asset, activated editable children and added this scene into simple_inherited without this PR, the modifications in ContainerEditable where lost in simple_inherited on reload and the foldable were resetted in ContainerEditable.
test-godot-blender-reimport-missing-node-v7.zip

Note: If you test the MRP from #91122, it's still broken because the skeleton is now considered modified in the scene. But if you create another scene exactly like 'actor_x', I was not able to reproduce the problem where the skeleton get broken when I modify it in blender and reexport the gltf.

editor/editor_node.h Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@SaracenOne SaracenOne force-pushed the reimport_owner_instance branch from 56766d7 to 1fe0730 Compare September 7, 2024 01:00
Copy link
Contributor

@Hilderin Hilderin left a comment

Choose a reason for hiding this comment

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

Sound good!! Nice work!

editor/editor_node.h Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@SaracenOne SaracenOne force-pushed the reimport_owner_instance branch from 1fe0730 to bd92e88 Compare September 8, 2024 02:12
Changes the behaviour of the scene hot-reload system
so that if the scene which needs to be reimported is
owned by another instance, reload that instance instead.
@SaracenOne SaracenOne force-pushed the reimport_owner_instance branch from bd92e88 to 86ce15f Compare September 8, 2024 02:12
@SaracenOne
Copy link
Member Author

@AThousandShips Fixed!

@akien-mga akien-mga merged commit 7a4c034 into godotengine:master Sep 8, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@WhalesState
Copy link
Contributor

void update_instance_resource(String p_path, Ref<PackedScene> p_packed_scene); should also be removed from packed_scene.h in the same commit, just in case this commit was reverted for any reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants