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

Fix missing material override after two glb reimports #96446

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Sep 1, 2024

The problem was because both objects in the scene inherited from the same resource. When the first inherited scene was loaded, the MeshInstance3D connected itself to the mesh's changed signal. Then, when the second inherited scene was loaded, the EditorNode called ResourceLoader::load with ResourceFormatLoader::CACHE_MODE_REPLACE_DEEP, forcing the mesh to reload a second time. However, since the first MeshInstance3D was already connected, when the loader updated the name of the mesh, it emitted the changed signal, triggering MeshInstance3D::_mesh_changed. The issue was that set_name was called before the mesh surfaces were loaded, causing the surface_override_materials to be resized to zero on the first MeshInstance3D.

I replaced ResourceFormatLoader::CACHE_MODE_REPLACE_DEEP with ResourceFormatLoader::CACHE_MODE_REPLACE. This should not cause any problems because EditorNode::reload_instances_with_path_in_edited_scenes already reloads every scene in the inherited chain. In fact, this should improve scene reloading since, with the deep parameter, the mesh was being reloaded for each inherited level!

I also retested this MRP that I used to test scene reimportation:
test-godot-blender-reimport-missing-node-v6.zip

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@SaracenOne
Copy link
Member

SaracenOne commented Sep 2, 2024

Good catch, quite a deeply nested problem, but I think this change should be okay (I am a little nervous about what might happen with references to individual resources contained in the reimported scene though; we might need more test cases).

By the way, since you've discovered quite a few edge cases with the system already, consider taking a look at this PR too (#96144). I tested it with the MRP you submitted and I believe it's somewhat related to the point you made. It also should fix the issue with the instanced scene which gets its editable_child's rotation reset upon reimport.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Didn't test but makes sense to me.

@fire fire requested a review from a team September 2, 2024 07:28
@AThousandShips AThousandShips added this to the 4.4 milestone Sep 2, 2024
@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 3, 2024
@akien-mga akien-mga merged commit f4cc60f into godotengine:master Sep 3, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

Missing material override after two glb reimports
5 participants