GLTF: Don't duplicate textures when importing blend files by demolke · Pull Request #98909 · godotengine/godot

demolke · 2024-11-06T22:10:31Z

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 · 2024-11-06T22:35:11Z

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

demolke · 2024-11-06T22:39:04Z

This is the issue #96778 (comment)

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

demolke · 2024-11-06T23:41:33Z

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.

lyuma · 2024-11-07T08:22:46Z

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:

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
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.

Found places that need improvement.

demolke · 2024-11-07T18:14:31Z

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

godot/modules/gltf/gltf_document.cpp

Line 4089 in 77d6283

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 · 2024-11-07T19:04:08Z

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

aaronfranke

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):

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

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 · 2024-11-08T20:49:54Z

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

demolke · 2024-11-14T00:28:53Z

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.

Otherwise asan complains if a test tries to use these. Split off from godotengine#98909

modules/gltf/tests/test_gltf.h

modules/gltf/tests/test_gltf_images.h

modules/gltf/tests/test_gltf.h

modules/gltf/tests/test_gltf_images.h

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 · 2024-12-18T20:25:58Z

Thanks for the review and the suggestions.

akien-mga · 2025-01-06T22:02:11Z

Thanks!

demolke requested a review from a team as a code owner November 6, 2024 22:10

demolke mentioned this pull request Nov 6, 2024

GLTF: Don't give up loading a texture if importing it fails #96778

Merged

fire previously approved these changes Nov 6, 2024

View reviewed changes

demolke marked this pull request as draft November 6, 2024 23:41

demolke force-pushed the master branch 2 times, most recently from aa50764 to 99b42ce Compare November 7, 2024 00:43

AThousandShips added bug topic:import labels Nov 7, 2024

AThousandShips added this to the 4.4 milestone Nov 7, 2024

demolke force-pushed the master branch 2 times, most recently from 69bad78 to 5adc9c7 Compare November 7, 2024 19:28

demolke requested a review from fire November 7, 2024 19:35

demolke marked this pull request as ready for review November 7, 2024 19:35

aaronfranke reviewed Nov 7, 2024

View reviewed changes

demolke force-pushed the master branch from 5adc9c7 to 68d9396 Compare November 14, 2024 00:26

demolke requested review from a team as code owners November 14, 2024 00:26

demolke force-pushed the master branch 3 times, most recently from 0b60a81 to 535c6bc Compare November 14, 2024 00:53

demolke force-pushed the master branch from d8d6cae to 0065730 Compare November 29, 2024 23:38

demolke mentioned this pull request Nov 30, 2024

Move reimport check to EditorImportPlugin #99858

Closed

demolke force-pushed the master branch from 0065730 to ebcf6c1 Compare December 1, 2024 09:00

demolke added a commit to demolke/godot that referenced this pull request Dec 2, 2024

Destroy EditorFilesystem/EditorSettings on test shutdown

35bb827

Otherwise asan complains if a test tries to use these. Split off from godotengine#98909

demolke force-pushed the master branch from ebcf6c1 to 3c479a2 Compare December 2, 2024 22:34

demolke force-pushed the master branch from 3c479a2 to 32167ca Compare December 3, 2024 21:09

demolke force-pushed the master branch 2 times, most recently from 68cd8f0 to 764b4e9 Compare December 5, 2024 22:50

demolke force-pushed the master branch from 764b4e9 to cd06612 Compare December 17, 2024 06:34

aaronfranke approved these changes Dec 17, 2024

View reviewed changes

AThousandShips reviewed Dec 17, 2024

View reviewed changes

demolke added 2 commits December 18, 2024 21:24

demolke force-pushed the master branch from 458e7d0 to e649e7e Compare December 18, 2024 20:24

akien-mga approved these changes Jan 6, 2025

View reviewed changes

akien-mga mentioned this pull request Mar 18, 2025

Unit tests for GLTF print some unwanted errors #104308

Open

Uh oh!

Conversation

demolke commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fire commented Nov 6, 2024

Uh oh!

demolke commented Nov 6, 2024

Uh oh!

demolke commented Nov 6, 2024

Uh oh!

lyuma commented Nov 7, 2024

Uh oh!

demolke commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

demolke commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronfranke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demolke commented Nov 8, 2024

Uh oh!

demolke commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

demolke commented Dec 18, 2024

Uh oh!

akien-mga commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

demolke commented Nov 6, 2024 •

edited

Loading

demolke commented Nov 7, 2024 •

edited

Loading

demolke commented Nov 7, 2024 •

edited

Loading

aaronfranke left a comment •

edited

Loading

demolke commented Nov 14, 2024 •

edited

Loading