vsg-dev / vsgXchange

Utility library for converting data+materials to/from VulkanSceneGraph
MIT License
65 stars 39 forks source link

Embedded texture-files in FBX are not being loaded by vsgXchange #151

Closed drywolf closed 1 year ago

drywolf commented 1 year ago

Hi,

I noticed that for .fbx files that have texture files embedded in them, those textures will not be loaded in my usage of VSG/vsgXchange. (regular textures on disk that are referenced by .fbx file are working just fine)

I traced the issue down to this block of code in the vsgXchange sources.

Also I already implemented the necessary fix and did some tests with it. Apparently AssImp has at some point changed their API for handling those embedded FBX textures. I am using this proposed fix, and it is working quite well for me.

I would like to create a PR for this fix, but I see one more scenario/code-path here that is also not being handled in the assimp.cpp code.

An aiTexture when coming from an FBX embedded image file can also be in the state such that mWidth > 0 && mHeight > 0 ... and in this case, the pcData field of aiTexture will contain an array of raw pixel data (color values).

The C++ comments in the AssImp sources tell me the following:

    /** Data of the texture.
     *
     * Points to an array of mWidth * mHeight aiTexel's.
     * The format of the texture data is always ARGB8888 to
     * make the implementation for user of the library as easy
     * as possible. If mHeight = 0 this is a pointer to a memory
     * buffer of size mWidth containing the compressed texture
     * data. Good luck, have fun!
     */
    C_STRUCT aiTexel* pcData;

Right now, only the case when mHeight == 0 is currently being handled by vsgXchange ?!

The second case, when mHeight > 0 could be easily handled, but there is one detail in the above C++ comment block that makes this tricky.

The format of the texture data is always ARGB8888

As far as I have seen, Vulkan does not have any VkFormat that would provide a pixel-swizzling order of ARGB ?!

This would mean to support this additional code-path ...

@robertosfield What are your thoughts on this? I can create a PR that would just fix the original issue, but hence this second issue is so very closely related, I wanted to also bring some attention to it.

Thanks

robertosfield commented 1 year ago

Thanks for the details an links. Could you provide an example .fbx file that illustrates the problem?

What version of Assimp are you using? Do you know what version the API/approach changed?

vsg::ImageView has the VkComponentMapping components:

https://github.com/vsg-dev/VulkanSceneGraph/blob/master/include/vsg/state/ImageView.h#L35

So potentially this could be used to swizzle the image into RGBA, this would be likely more efficient than doing the swizzle on the CPU.

drywolf commented 1 year ago

Could you provide an example .fbx file that illustrates the problem?

earth.zip (sorry I had to use .zip ... github doesn't allow pasting .fbx files 😅🤷)

Creating such an .fbx file in blender is pretty straightforward. When exporting a model from blender to .fbx, just make sure to ...

image

What version of Assimp are you using?

v5.2.5

(the exact commit that I am using to build AssImp from source is bee751b5f95999aac52102a904871a8166fb7cf9 ... there is no specific reason that I am using this particular commit, it was just the latest commit on master when I cloned the repo)

Do you know what version the API/approach changed?

In the above mentioned assimp issue discussion, user loebl mentions that for them:

the loading procedure for embedded textures from FBX changed in the last few weeks.

... this was posted on March 7 2018, so the change is not really a new one.

The first commit that seems to have changed the approach should be this one assimp/assimp@4623c2f14c121e4792e74169b6cef09631a4184a (at least that's what I could find, just having a quick look into the git history/blame) The commit is from Dec 18 2017 ... so it predates the issue discussion by a couple months.

PS: the code of aiScene::GetEmbeddedTexture() seems to have gotten some more changes since then, and now it looks to me like it just will handle both conventions automatically/transparently for the texture names)

drywolf commented 1 year ago

vsg::ImageView has the VkComponentMapping components:

https://github.com/vsg-dev/VulkanSceneGraph/blob/master/include/vsg/state/ImageView.h#L35

So potentially this could be used to swizzle the image into RGBA, this would be likely more efficient than doing the swizzle on the CPU.

Yeah, that's what I was also thinking about ... but the question is, should this automagically be done by the vsgXchange importer 🧙 ?! I am not yet informed enough about the vsg/vsgXchange code-base to know if this would have any weird/undesired consequences for users of the vsg/vsgXchange APIs ... do you think using the VkComponentMapping would be a proper solution in this case?

I am thinking, this is a very specific and subtle/niche change of Vulkan state, that might not be obvious to users of vsg/vsgXchange, and hence might lead to some confusion if they start tinkering with the internals of a scene-graph that was loaded from an .fbx file.

They might expect the Vulkan image to contain RGBA data, because that is what the vsg::Image format would be telling them, but actually it would be ARGB data.

robertosfield commented 1 year ago

I'm back a long weekend away and briefly my dev system. I have download the test file and when I run VSG/vsgXchange/assimp master master I get:

$ vsgviewer earth.fbx 
Warning: Failed to load texture:  texPath = earth.fbm\earth.jpg

I have a quick look at the vsgXchange::assimp code so have refreshed myself where any changes will likely need to be and have looked at the various links you provided. The proposed fix link looks pretty straight forward so I'll begin by experimenting with that. I can't finish this all today though, and am away much of tomorrow so it may not be later in the week that I can resolve this issue.

drywolf commented 1 year ago

For the first issue (the API change of assimp) the fix should be relatively straight-forward. This is what I am already using:

diff --git forkSrcPrefix/src/assimp/assimp.cpp forkDstPrefix/src/assimp/assimp.cpp
index bec8263f9ee8e41afd538d7f939305eb70a3a45f..a3c05ad93190c538aa97906e2ddfbcda2f38f001 100644
--- forkSrcPrefix/src/assimp/assimp.cpp
+++ forkDstPrefix/src/assimp/assimp.cpp
@@ -249,10 +249,8 @@ SamplerData SceneConverter::convertTexture(const aiMaterial& material, aiTexture
     {
         SamplerData samplerImage;

-        if (texPath.data[0] == '*')
+        if (auto texture = scene->GetEmbeddedTexture(texPath.C_Str()))
         {
-            const auto texIndex = std::atoi(texPath.C_Str() + 1);
-            const auto texture = scene->mTextures[texIndex];
             if (texture->mWidth > 0 && texture->mHeight == 0)
             {
                 auto imageOptions = vsg::Options::create(*options);

(this patch applied to vsgXchange allows me to load the provided earth.fbx just fine)

For the second "issue", when mWidth > 0 && mHeight > 0 I also do not yet know how to produce such .fbx files from any 3D tool (like Blender) I was only speculating how to potentially support this case (i.e. with VkComponentMapping swizzle, etc.)

robertosfield commented 1 year ago

I have created a AssimpEmbeddedTextures branch of vsgXchange:

https://github.com/vsg-dev/vsgXchange/tree/AssimpEmbeddedTextures

And have merged your proposed fix above: 063f79a644617acb7568b81bc3eb0ee24c8d0a43

Then I had a bash at implementing handling of the texture->mHeight != 0 case: 98839d9b1720d997b54f6e2e086b43074415c602

I have decided to just convert the ARGB to RGBA on the CPU, while it's not optimal for load performance it keeps usage simpler and avoids complexity shifting elsewhere. I do a test for texture->achFormatHint as I think this is appropriate argb8888 but as I don't have any test data for this case yet I don't know whether this is all correct yet.

We need to track down a model that can test out this code path before we can merge the AssimpEmbeddedTextures branch with vsgXchange master.

drywolf commented 1 year ago

That is great 🥳

We need to track down a model that can test out this code path before we can merge the AssimpEmbeddedTextures branch with vsgXchange master.

I will also keep looking if I can find such a model online, or a way to produce such files from any 3D tool. If I find one, I will let you know.

robertosfield commented 1 year ago

I have run a test against the test data in the assimp repo and found that it's reporting rgba8888 not the argb8888 that the comment suggests, this is what I get with a bit of extra debug info:

$ find . -type "f" -exec vsgconv {} test.vsgb \;  &> output.txt
robert@ledi:~/3rdParty/assimp/test/models$ grep Embedded output.txt 
info: Embedded compressed format texture->achFormatHint = tga
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded compressed format texture->achFormatHint = png
info: Embedded raw format texture->achFormatHint = 
Warning: Embedded texture format not supported, texture->achFormatHint = 
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = rgba8888
Warning: Embedded texture format not supported, texture->achFormatHint = rgba8888
info: Embedded raw format texture->achFormatHint = 
Warning: Embedded texture format not supported, texture->achFormatHint = 
robertosfield commented 1 year ago

To confuse things assimp texture.h says:

The // --------------------------------------------------------------------------------
/** @brief Helper structure to represent a texel in a ARGB8888 format
*
*  Used by aiTexture.
*/
struct aiTexel {
    unsigned char b,g,r,a;

I will relax the check and see what happens on screen with rgbs8888

robertosfield commented 1 year ago

I have refined the colour conversion code so that the embedded texture examples in assimp/tests/models now look sensible. The changes are now merged with vsgXchange master with PR #152.

I'll close this PR now as I believe the required functionality is now in place, if there area issues please open an new issue and provide links to models that exhibit an issue.