[osg-submissions] FBX plugin improvement regarding texture support

[Alessandro Terenzi]

Hi,
based on my experience during the last months I noticed that many times some authoring software tends to export FBX models using FbxLayeredTexture instead of FbxFileTexture even if only 1 texture is really used. When loading such models with the current version of the FBX plugin, the model would look un-textured.

I'd like to propose to support FbxLayeredTexture along with FbxFileTexture properties, I've already a modified version of the fbxMaterialToOsgStateSet.cpp file in order to read at least the first layer of a layered texture.

My idea is to replace this:


Code:
int lNbTex = lProperty.GetSrcObjectCount<FbxFileTexture>();
for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++)
{
FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(lProperty.GetSrcObject<FbxFileTexture>(lTextureIndex));
if (lTexture)
{
result.diffuseTexture = fbxTextureToOsgTexture(lTexture);
result.diffuseChannel = lTexture->UVSet.Get();
result.diffuseScaleU = lTexture->GetScaleU();
result.diffuseScaleV = lTexture->GetScaleV();
}

//For now only allow 1 texture
break;
}



with this (for every supported channel, not only the diffuse one of course):


Code:
// check if layered textures are used...
int layeredTextureCount = lProperty.GetSrcObjectCount<FbxLayeredTexture>();
if(layeredTextureCount)
{
FbxLayeredTexture* layered_texture = FbxCast<FbxLayeredTexture>(lProperty.GetSrcObject<FbxLayeredTexture>(0));
int lNbTex = layered_texture->GetSrcObjectCount<FbxFileTexture>();

if(lNbTex)
{
// at least 1 texture, use 1st...
for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++)
{
FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(layered_texture->GetSrcObject<FbxFileTexture>(lTextureIndex));
if (lTexture)
{
result.diffuseTexture = fbxTextureToOsgTexture(lTexture);
result.diffuseChannel = lTexture->UVSet.Get();
result.diffuseScaleU = lTexture->GetScaleU();
result.diffuseScaleV = lTexture->GetScaleV();
}

//For now only allow 1 texture
break;
}
}
else
OSG_WARN << "FBX: Missing Textures in FbxLayeredTexture." << std::endl;
}
else
{
int lNbTex = lProperty.GetSrcObjectCount<FbxFileTexture>();
for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++)
{
FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(lProperty.GetSrcObject<FbxFileTexture>(lTextureIndex));
if (lTexture)
{
result.diffuseTexture = fbxTextureToOsgTexture(lTexture);
result.diffuseChannel = lTexture->UVSet.Get();
result.diffuseScaleU = lTexture->GetScaleU();
result.diffuseScaleV = lTexture->GetScaleV();
}

//For now only allow 1 texture
break;
}
}





What do you think?

Alessandro

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=72794#72794

[Alessandro Terenzi]

Hi,
regarding the (initial) support of layered textures in the FBX plugin, my proposal is attached to this message.

Let me know what you think and if we can merge the file. Thanks.

Cheers,
Alessandro

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=72820#72820




Attachments:
http://forum.openscenegraph.org//files/fbxmaterialtoosgstateset_915.cpp

[Robert Osfield]

Hi Alessandro,

I just had quick look at the differences between your code and the
original code, there are a great number of changes required to handle
just one aspect of different source data. The code also has repeated
comments of //For now only allow 1 texture which is a sure indication
that the code isn't yet fully functional.

I haven't yet reviewed the StateSetContent class that is being set by
this convert function, my guess is that this is a bit too limited and
need to be improved to handle multiple textures as per the source
data, and then for the code that unpacks the various ways textures can
be sourced and assigned in a reusable ways so that there isn't these
repeated blocks with tiny differences in them. Instead there should
be some helper methods, or perhaps functors to help improve code reuse
and avoid these mega long methods.

Once this is done it should be easier to slot in the different ways
that textures can be specified.

BTW, I'm not the original author of this code and don't have access to
a modelling tool that provides FBX files so I'm having to learn stuff
just from the source code and review whether it makes sense form a
high level.

Robert.

[Alessandro Terenzi]

Hi Robert,
yes, I agree that is quite a lot of code and also that each block seems to repeat over and over, but even though I could have somehow refactored the code I still think that the method would not be as readable as the original and would not be considerable shorter because of the many checks that should be added. Also, considering the time I could spend on this, I tried to keep the changes with respect to the original code as minimal as possible.

Moreover, I thought that when one would decide to add complete support for layered textures then each block would, presumably, differ a lot with respect to the others and also with the code that currently looks similar (the simple "file texture" case), so one would have to 'roll back' to the proposed initial implementation and then start adding things afterwards.

These were the reasons for me to opt for the cumbersome repeating blocks, even the comments were intentionally duplicated because they still make sense when the layered texture contains exactly 1 texture.

For sure the proposed solution is not the optimal implementation, I saw it more like a 'workaround' and as a starting point for someone that could be willing to keep adding support for layered textures... anyway there's no hurry to merge this until eventually a more complete support will be added from scratch.

If I manage to find more time I will also consider to refactor the code myself and come back with a new solution, just cannot plan it now :)

Cheers,
Alessandro

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=72838#72838

[Robert Osfield]

HI Alessandro,

I have a further look at fbxMaterialToSOsgStateSet.h + .cpp and it's
pretty clear to me that it needs to be refactored before any further
modifications are done. There is so much in the header and the ,cpp
that is repeated when it should have been written with written
differently.

The right way to do it would be to have a class to wrap all the
texture properties then have an instance of this for each of the
diffuse, opacity etc. This would result in far less code and greater
clarity.

I can't tackle this right away, but once I've cleared my present work
enough I'll return to this.

Robert.

[Robert Osfield]

Hi Alessandro et. al,

Post by Robert Osfield
HI Alessandro,
I have a further look at fbxMaterialToSOsgStateSet.h + .cpp and it's
pretty clear to me that it needs to be refactored before any further
modifications are done. There is so much in the header and the ,cpp
that is repeated when it should have been written with written
differently.
The right way to do it would be to have a class to wrap all the
texture properties then have an instance of this for each of the
diffuse, opacity etc. This would result in far less code and greater
clarity.
I can't tackle this right away, but once I've cleared my present work
enough I'll return to this.

I now have some time to look into this. If you have any further
work/insights to chip in as I dive into the original and proposed
changes let me know.

Cheers,
Robert.

[Robert Osfield]

HI Alessandro,

I have added FbxLayeredTexture supported to the the new
FbxFileTexture* FbxMaterialToOsgStateSet::selectFbxFileTexture(const
FbxProperty& lProperty) that wraps up the selection of the FileTexture
to use. I don't haven't test any models that have FbxLayeredTexture
in them yet so don't know if it works yet. Could you checked git
master and let me know if it works fine for your models.

The commit that adds FbxLayeredTexture is:

https://github.com/openscenegraph/OpenSceneGraph/commit/957bd4b8866b35a9fa1c5ab708fc6c9d897efd22

Cheers,
Robert.

[Alessandro Terenzi]

Ok, thank you Robert.

I will be glad to help and contribute of course.

Cheers,
Alessandro

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=72842#72842