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

AssimpImporter: Fix loading of embedded textures from FBX files #128

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Aug 6, 2022

Hi @mosra,

As promised, here's the fix for #127 .

The test file was created with Blender 3.1.0 as follows:

  • Delete default scene
  • Add plane
  • Set the diffuse color to image texture with this image: embedded
  • File > Export > FBX, chose path mode "copy" and press the button next to the dropdown, which enables embedding images

The file now has the absolute path of the PNG embedded (C:/Users/Squareys/Desktop/embedded.png), similar to how I've seen it in "real world files", but the image data is embedded, so the loader doesn't actually access this file ever. This was ensured by renaming the image file and opening it with the Windows 3D viewer, the texture was still correctly displayed.

The test was added first, to ensure this was actually the culprit, it fails on the image expression, i.e. image was not found, exactly what I was observing in #127.

Since the GetEmbeddedImage() was added with 5.0.0, I kept the legacy path behind the ASSIMP_IS_VERSION_5_OR_GREATER, but I did not test the code (really hoping the CI does this, would be a hassle to set up the CI is testing this, luckily).

I double checked the drive-by std::string => StringView/String changes, let's hope that won't blow up in my face like on the EmscriptenApplication 😅

Looking forward to the review!

Best,
Jonathan

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the assimp-embedded-images branch 2 times, most recently from bb255f4 to 60a8a4e Compare August 6, 2022 20:47
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

Squareys commented Aug 6, 2022

linux-arm64 failed with StbResizeImageConverterTest sefaulting 🤔

@mosra mosra added this to the 2022.0a milestone Aug 8, 2022
@mosra mosra merged commit 8e92178 into mosra:master Aug 8, 2022
@mosra
Copy link
Owner

mosra commented Aug 8, 2022

Thanks! Especially for the extra care to put the test in an earlier commit.

Merged with one minor change -- I just made the #ifdef a bit less surprising in 1db2003.

@Squareys
Copy link
Contributor Author

Squareys commented Aug 8, 2022

Amazing, thank you! 🎉 🙏

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

Successfully merging this pull request may close these issues.

2 participants