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 GLTF embedded texture not reimporting when changed #85040

Closed

Conversation

Invertex
Copy link
Contributor

@Invertex Invertex commented Nov 18, 2023

Fixes #83154

The existing code skips re-extract/reimport of the embedded texture if an existing file at the extract path doesn't have a md5 value in the remap:generator_parameters section of its .import metadata, under the assumption the existing file wasn't an extracted version because it's missing it. But it wasn't writing the md5 value to the metadata when importing the first time, so this failed as a check.


Though I'm not sure this is a good long-term solution to tracking whether an existing texture is an extracted texture or not. I feel there should probably be more specific metadata related to this than just an md5, like an isExtractTexture bool and maybe an ownerUID entry...

@Invertex Invertex requested a review from a team as a code owner November 18, 2023 02:36
@Invertex Invertex force-pushed the gLTF-internal-texture-reimport branch from 29e684e to b886c1f Compare November 18, 2023 05:17
@Chaosus Chaosus added this to the 4.3 milestone Nov 18, 2023
@akien-mga akien-mga changed the title Fix gLTF embedded texture not reimporting when changed Fix GLTF embedded texture not reimporting when changed Nov 18, 2023
@fire
Copy link
Member

fire commented Nov 18, 2023

@lyuma you may want to take a look too.

@Invertex
Copy link
Contributor Author

Invertex commented Nov 19, 2023

One existing potential issue I'm noticing is that the texture the model is referencing after import doesn't seem to be the extracted texture? It seems it's still embedding its own copy and referencing that, so any edits to the extracted texture will not be reflected on the model unless the material properties are edited on it. This seems like counterintuitive behavior?

But if it was updated to use the extracted version, that then brings up another issue. What happens when we edit the external one, but then the model goes through reimport at some point, the md5 will mismatch and the edited external version will be overwritten by the older embedded version. This is more unforeseen behavior we probably don't want.

So perhaps it would be good to then introduce another import option of Extract Textures + Overwrite Existing and rename the other to Extract Textures + Reuse Existing , so a user can decide their workflow, if they intend to continue editing a texture externally or updating it in the model file.

Or another proposal could be for an import option on texture files to mark them as not being allowed for overwrite by Godot import processes (other than its own import process of course)

@akien-mga
Copy link
Member

Superseded by #86504. Thanks for the contribution nonetheless!

@akien-mga akien-mga closed this Jan 2, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Jan 2, 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.

glTF import won't update extracted texture
5 participants