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

GLTF: Don't duplicate textures when importing blend files #98909

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

demolke
Copy link
Contributor

@demolke demolke commented Nov 6, 2024

blender imports will always start with res://.godot/imported because we first convert the blend to gltf, store it in res://.godot/imported and run the import from there, so resources linked from blend files end up with duplicate textures after #96778

Use simplify_path to differentiate between resources actually in the imported folder vs resource relative to GLTF in imported folder.

Introduce a new state where the resource exists on the disk but has not been imported yet.

Fixes: #98932

@fire
Copy link
Member

fire commented Nov 6, 2024

I think this makes sense, but what was the original problem being solved?

@demolke
Copy link
Contributor Author

demolke commented Nov 6, 2024

This is the issue #96778 (comment)

After #96778 all textures imported from blend files will get duplicated in the res:// folder.

fire
fire previously approved these changes Nov 6, 2024
@demolke
Copy link
Contributor Author

demolke commented Nov 6, 2024

Ah, me and github - I've uploaded only a part of the change, let me switch it to draft while I try to fix it.

@demolke demolke marked this pull request as draft November 6, 2024 23:41
@demolke demolke force-pushed the master branch 2 times, most recently from aa50764 to 99b42ce Compare November 7, 2024 00:43
@lyuma
Copy link
Contributor

lyuma commented Nov 7, 2024

Please file a new issue for the bug, with an attached reproduction project, and link to that bug in this PR.

A link to a PR comment makes it difficult to have a meaningful conversation on this issue, and there is enough nuance with this issue that I don't want to merge a PR without an issue


I'm missing some information here. For example, the issue purports to affect blend files, but the PR here only touches gltf_document code, which means that the issue must be reproducible from a standalone gltf file. I want to understand what cases are being changed.

Generally speaking, there are two cases for gltf:

  1. either the images are embedded, in which case they are extracted with the model name prefixed (texture inside model.gltf is extracted to model_texture.png), or
  2. the images are referenced externally from the gltf file, in which case, they are not extracted since they are external references. In this case, model.gltf would directly reference texture.png which already exists.

This PR is hinting at a possible third case, which I do not understand, in which model.gltf (or model.blend) references texture.png but somehow the gltf_document code also extracts model_texture.png.

I am skeptical about this fix, and we should also keep in mind that when blender gltf exports embed a texture file, there is sometimes a reason for it. I believe that, for example, blender will at times make modifications to the texture and embed it to comply with the gltf spec or to handle certain shader nodes.

@AThousandShips AThousandShips added this to the 4.4 milestone Nov 7, 2024
@fire fire dismissed their stale review November 7, 2024 16:56

Found places that need improvement.

@demolke
Copy link
Contributor Author

demolke commented Nov 7, 2024

The part about blender is somewhat significant, because .blend will get converted to .gltf and placed in res://.godot/imported - something the #96778 considers as impossible to import

if (!p_base_path.begins_with("res://.godot/imported")) {
so what happens we fail this check because texture linked from a blend file is considered as placed in imported folder and we fall through to writing it to the disk.

(It's very much possible to trigger this if you call gltf import methods yourself with paths relative to res://.godot/imported but no one is probably doing that.)

https://github.com/user-attachments/files/17640095/nonembed.zip

So on disk it looks like this before the image processing

.godot/imported/model.gltf
assets/model.blend
assets/texture.png

The code above will get res://.godot/imported/../../assets/texture.png dismisses it as unimportable, writes the data to the disk res://assets/model-texture.png and imports that.

This PR is hinting at a possible third case, which I do not understand, in which model.gltf (or model.blend) references texture.png but somehow the gltf_document code also extracts model_texture.png.

Yes, exactly - that's the behavior #96778 introduced.

I'll port it into an issue - #98932

@demolke
Copy link
Contributor Author

demolke commented Nov 7, 2024

The change

  • does uri.simplify_path(); to make uri absolute
  • removes the p_base_path.begins_with("res://.godot/imported") check - I think it's meaningless - ResourceLoader will tell you if the texture has been imported or not
  • if I've would have left _parse_image_save_image as is, all un-imported textures that have valid uri would get read from the disk and then written back for no reason

@demolke demolke force-pushed the master branch 2 times, most recently from 69bad78 to 5adc9c7 Compare November 7, 2024 19:28
@demolke demolke requested a review from fire November 7, 2024 19:35
@demolke demolke marked this pull request as ready for review November 7, 2024 19:35
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.

In the future, please make a new branch for your PRs, instead of using master.

Thank you for working on this. It makes sense to separate the must_write and must_import booleans. And yeah, I completely forgot to consider the case of res://.godot/imported/../../ which results in things being not inside the res:://.godot/ folder after all.

However, this doesn't seem to work in practice. PR #96778 was designed to be a GLTF-only PR to make way for PR #96782.

With only PR #96782, the unpacking works correctly (the GLTF material points to the imported texture):

Screenshot 2024-11-07 at 3 13 28 PM

With both PR #96782 and this PR, it errors and does not extract (the material's texture is directly loaded an image):

Screenshot 2024-11-07 at 3 10 35 PM

The test file here is the MRP in issue #82455 with "Materials -> Unpack Enabled" set to false (this option should be renamed to "Unpack Originals" in the future).

@demolke
Copy link
Contributor Author

demolke commented Nov 8, 2024

@aaronfranke thanks for the pointer. I only tested it with embedded textures and unpack enabled, so will take a look.

@demolke
Copy link
Contributor Author

demolke commented Nov 14, 2024

I think I've addressed all the permutations. Added a couple of tests, but they do not capture the full breadth of the problem (that would require blender installation during unit test) - only some of the basic use cases. I am not very happy with them - they write to disk, so are not idempotent. I'm wondering if to include them at all. It would be nice if we had memory backed res:// support.

One interesting thing to note is that there is a race condition in _parse_image_save_image with "Materials -> Unpack Enabled" set to true (i.e. the implicit default) even before #96778. On first run the blender gltf export will create textures directory which will not be scanned by the time we try to parse it, so EditorFileSystem::get_singleton()->reimport_append will not find those files. On the import we will load the textures directly and on the second re-import the folder will be found and the resource linkage succeeds.

@demolke demolke force-pushed the master branch 3 times, most recently from 0b60a81 to 535c6bc Compare November 14, 2024 00:53
demolke added a commit to demolke/godot that referenced this pull request Nov 29, 2024
Having memory backed res:// folder is useful for hermetic tests that need to mutate contents of the resource directory.

For example in godotengine#98909 we want the res:// folder not to contain a texture imported on previous run to always test the same code path.

It requires changes to core/os/os.h so that we can re-initialize the file/dir handlers during test setup.
demolke added a commit to demolke/godot that referenced this pull request Nov 29, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Nov 29, 2024
Similar to editor_hint, this introduces a hint if the current execution is under test. It's very hard to setup the import process the same in tests - especially with nested imports - i.e. blend imports gltf which imports a texture.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Nov 29, 2024
EditorNode is a very heavy object that current test harness cannot create, so after godotengine#96544 editor import cannot be tested.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Nov 29, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Nov 29, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Nov 30, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Dec 1, 2024
EditorNode is a very heavy object that current test harness cannot create, so after godotengine#96544 editor import cannot be tested.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Dec 1, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Dec 2, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Dec 2, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
demolke added a commit to demolke/godot that referenced this pull request Dec 3, 2024
Having memory backed res:// folder is useful for hermetic tests that need to mutate contents of the resource directory.

For example in godotengine#98909 we want the res:// folder not to contain a texture imported on previous run to always test the same code path.

It requires changes to core/os/os.h so that we can re-initialize the file/dir handlers during test setup.
wyvrtn pushed a commit to wyvrtn/godot that referenced this pull request Dec 5, 2024
Otherwise asan complains if a test tries to use these.

Split off from godotengine#98909
@demolke demolke force-pushed the master branch 2 times, most recently from 68cd8f0 to 764b4e9 Compare December 5, 2024 22:50
demolke added a commit to demolke/godot that referenced this pull request Dec 17, 2024
Having memory backed res:// folder is useful for hermetic tests that need to mutate contents of the resource directory.

For example in godotengine#98909 we want the res:// folder not to contain a texture imported on previous run to always test the same code path.

It requires changes to core/os/os.h so that we can re-initialize the file/dir handlers during test setup.
modules/gltf/tests/test_gltf.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
modules/gltf/tests/test_gltf_images.h Outdated Show resolved Hide resolved
demolke added a commit to demolke/godot that referenced this pull request Dec 18, 2024
Having memory backed res:// folder is useful for hermetic tests that need to mutate contents of the resource directory.

For example in godotengine#98909 we want the res:// folder not to contain a texture imported on previous run to always test the same code path.

It requires changes to core/os/os.h so that we can re-initialize the file/dir handlers during test setup.
demolke added a commit to demolke/godot that referenced this pull request Dec 18, 2024
Having memory backed res:// folder is useful for hermetic tests that need to mutate contents of the resource directory.

For example in godotengine#98909 we want the res:// folder not to contain a texture imported on previous run to always test the same code path.

It requires changes to core/os/os.h so that we can re-initialize the file/dir handlers during test setup.
reimport_append is used by gltf_document, fbx_document and editor_import_plugin. The first two will never call it when importing == false. It's only the editor_import_plugin that should guard against that.
https://docs.godotengine.org/en/stable/classes/class_editorimportplugin.html#class-editorimportplugin-method-append-import-external-resource

The motivation of removing the check from gltf_document call path is to be able to test nested imports (texture embedded in gltf).
Blender imports will always start within `.godot/imported`  folder because we first convert the .blend file to .gltf, store it in `.godot/imported` and run the import from there, so on-disk resources linked from .blend files end up with duplicate textures.
@demolke
Copy link
Contributor Author

demolke commented Dec 18, 2024

Thanks for the review and the suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Not Critical
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

Blender import duplicates textures
5 participants