From fb4a67d4fe4287bb90856dc0e0e2b3c0e5097313 Mon Sep 17 00:00:00 2001 From: Tommy Date: Thu, 11 Jan 2018 17:30:17 +0100 Subject: [PATCH 1/2] Improve FBX material import. Some properties were being incorrectly interpreted, and by default it was relying on a legacy system. --- code/FBXConverter.cpp | 123 +++++++++++++++++++++++++++++++----------- code/FBXProperties.h | 20 +++++-- 2 files changed, 108 insertions(+), 35 deletions(-) diff --git a/code/FBXConverter.cpp b/code/FBXConverter.cpp index 66f541f17..b0336afe6 100644 --- a/code/FBXConverter.cpp +++ b/code/FBXConverter.cpp @@ -241,6 +241,10 @@ private: // ------------------------------------------------------------------------------------------------ aiColor3D GetColorPropertyFromMaterial( const PropertyTable& props, const std::string& baseName, bool& result ); + aiColor3D GetColorPropertyFactored( const PropertyTable& props, const std::string& colorName, + const std::string& factorName, bool& result, bool useTemplate=true ); + aiColor3D GetColorProperty( const PropertyTable& props, const std::string& colorName, + bool& result, bool useTemplate=true ); // ------------------------------------------------------------------------------------------------ void SetShadingPropertiesCommon( aiMaterial* out_mat, const PropertyTable& props ); @@ -1716,7 +1720,7 @@ unsigned int Converter::ConvertMaterial( const Material& material, const MeshGeo aiString str; - // stip Material:: prefix + // strip Material:: prefix std::string name = material.Name(); if ( name.substr( 0, 10 ) == "Material::" ) { name = name.substr( 10 ); @@ -2077,40 +2081,62 @@ void Converter::SetTextureProperties( aiMaterial* out_mat, const LayeredTextureM TrySetTextureProperties( out_mat, layeredTextures, "ShininessExponent", aiTextureType_SHININESS, mesh ); } -aiColor3D Converter::GetColorPropertyFromMaterial( const PropertyTable& props, const std::string& baseName, - bool& result ) +aiColor3D Converter::GetColorPropertyFactored( const PropertyTable& props, const std::string& colorName, + const std::string& factorName, bool& result, bool useTemplate ) { result = true; bool ok; - const aiVector3D& Diffuse = PropertyGet( props, baseName, ok ); - if ( ok ) { - return aiColor3D( Diffuse.x, Diffuse.y, Diffuse.z ); + aiVector3D BaseColor = PropertyGet( props, colorName, ok, useTemplate ); + if ( ! ok ) { + result = false; + return aiColor3D( 0.0f, 0.0f, 0.0f ); } - else { - aiVector3D DiffuseColor = PropertyGet( props, baseName + "Color", ok ); - if ( ok ) { - float DiffuseFactor = PropertyGet( props, baseName + "Factor", ok ); - if ( ok ) { - DiffuseColor *= DiffuseFactor; - } - return aiColor3D( DiffuseColor.x, DiffuseColor.y, DiffuseColor.z ); - } + // if no factor name, return the colour as is + if ( factorName.empty() ) { + return aiColor3D( BaseColor.x, BaseColor.y, BaseColor.z ); } - result = false; - return aiColor3D( 0.0f, 0.0f, 0.0f ); + + // otherwise it should be multiplied by the factor, if found. + float factor = PropertyGet( props, factorName, ok, useTemplate ); + if ( ok ) { + BaseColor *= factor; + } + return aiColor3D( BaseColor.x, BaseColor.y, BaseColor.z ); } +aiColor3D Converter::GetColorPropertyFromMaterial( const PropertyTable& props, const std::string& baseName, + bool& result ) +{ + return GetColorPropertyFactored( props, baseName + "Color", baseName + "Factor", result, true ); +} + +aiColor3D Converter::GetColorProperty( const PropertyTable& props, const std::string& colorName, + bool& result, bool useTemplate ) +{ + result = true; + bool ok; + const aiVector3D& ColorVec = PropertyGet( props, colorName, ok, useTemplate ); + if ( ! ok ) { + result = false; + return aiColor3D( 0.0f, 0.0f, 0.0f ); + } + return aiColor3D( ColorVec.x, ColorVec.y, ColorVec.z ); +} void Converter::SetShadingPropertiesCommon( aiMaterial* out_mat, const PropertyTable& props ) { - // set shading properties. There are various, redundant ways in which FBX materials - // specify their shading settings (depending on shading models, prop - // template etc.). No idea which one is right in a particular context. - // Just try to make sense of it - there's no spec to verify this against, - // so why should we. + // Set shading properties. + // Modern FBX Files have two separate systems for defining these, + // with only the more comprehensive one described in the property template. + // Likely the other values are a legacy system, + // which is still always exported by the official FBX SDK. + // + // Blender's FBX import and export mostly ignore this legacy system, + // and as we only support recent versions of FBX anyway, we can do the same. bool ok; + const aiColor3D& Diffuse = GetColorPropertyFromMaterial( props, "Diffuse", ok ); if ( ok ) { out_mat->AddProperty( &Diffuse, 1, AI_MATKEY_COLOR_DIFFUSE ); @@ -2126,29 +2152,64 @@ void Converter::SetShadingPropertiesCommon( aiMaterial* out_mat, const PropertyT out_mat->AddProperty( &Ambient, 1, AI_MATKEY_COLOR_AMBIENT ); } - const aiColor3D& Specular = GetColorPropertyFromMaterial( props, "Specular", ok ); + // we store specular factor as SHININESS_STRENGTH, so just get the color + const aiColor3D& Specular = GetColorProperty( props, "SpecularColor", ok, true ); if ( ok ) { out_mat->AddProperty( &Specular, 1, AI_MATKEY_COLOR_SPECULAR ); } + // and also try to get SHININESS_STRENGTH + const float SpecularFactor = PropertyGet( props, "SpecularFactor", ok, true ); + if ( ok ) { + out_mat->AddProperty( &SpecularFactor, 1, AI_MATKEY_SHININESS_STRENGTH ); + } + + // and the specular exponent + const float ShininessExponent = PropertyGet( props, "ShininessExponent", ok ); + if ( ok ) { + out_mat->AddProperty( &ShininessExponent, 1, AI_MATKEY_SHININESS ); + } + + // TransparentColor / TransparencyFactor... gee thanks FBX :rolleyes: + const aiColor3D& Transparent = GetColorPropertyFactored( props, "TransparentColor", "TransparencyFactor", ok ); + float CalculatedOpacity = 1.0; + if ( ok ) { + out_mat->AddProperty( &Transparent, 1, AI_MATKEY_COLOR_TRANSPARENT ); + // as calculated by FBX SDK 2017: + CalculatedOpacity = 1.0 - ((Transparent.r + Transparent.g + Transparent.b) / 3.0); + } + + // use of TransparencyFactor is inconsistent. + // Maya always stores it as 1.0, + // so we can't use it to set AI_MATKEY_OPACITY. + // Blender is more sensible and stores it as the alpha value. + // However both the FBX SDK and Blender always write an additional + // legacy "Opacity" field, so we can try to use that. + // + // If we can't find it, + // we can fall back to the value which the FBX SDK calculates + // from transparency colour (RGB) and factor (F) as + // 1.0 - F*((R+G+B)/3). + // + // There's no consistent way to interpret this opacity value, + // so it's up to clients to do the correct thing. const float Opacity = PropertyGet( props, "Opacity", ok ); if ( ok ) { out_mat->AddProperty( &Opacity, 1, AI_MATKEY_OPACITY ); } - - const float Reflectivity = PropertyGet( props, "Reflectivity", ok ); - if ( ok ) { - out_mat->AddProperty( &Reflectivity, 1, AI_MATKEY_REFLECTIVITY ); + else if ( CalculatedOpacity != 1.0 ) { + out_mat->AddProperty( &CalculatedOpacity, 1, AI_MATKEY_OPACITY ); } - const float Shininess = PropertyGet( props, "Shininess", ok ); + // reflection color and factor are stored separately + const aiColor3D& Reflection = GetColorProperty( props, "ReflectionColor", ok, true ); if ( ok ) { - out_mat->AddProperty( &Shininess, 1, AI_MATKEY_SHININESS_STRENGTH ); + out_mat->AddProperty( &Reflection, 1, AI_MATKEY_COLOR_REFLECTIVE ); } - const float ShininessExponent = PropertyGet( props, "ShininessExponent", ok ); + float ReflectionFactor = PropertyGet( props, "ReflectionFactor", ok, true ); if ( ok ) { - out_mat->AddProperty( &ShininessExponent, 1, AI_MATKEY_SHININESS ); + out_mat->AddProperty( &ReflectionFactor, 1, AI_MATKEY_REFLECTIVITY ); } const float BumpFactor = PropertyGet(props, "BumpFactor", ok); diff --git a/code/FBXProperties.h b/code/FBXProperties.h index 09b8aa94c..bf1b38e97 100644 --- a/code/FBXProperties.h +++ b/code/FBXProperties.h @@ -148,11 +148,23 @@ T PropertyGet(const PropertyTable& in, const std::string& name, const T& default // ------------------------------------------------------------------------------------------------ template inline -T PropertyGet(const PropertyTable& in, const std::string& name, bool& result) { - const Property* const prop = in.Get(name); +T PropertyGet(const PropertyTable& in, const std::string& name, bool& result, bool useTemplate=false ) { + const Property* prop = in.Get(name); if( nullptr == prop) { - result = false; - return T(); + if ( ! useTemplate ) { + result = false; + return T(); + } + const PropertyTable* templ = in.TemplateProps(); + if ( nullptr == templ ) { + result = false; + return T(); + } + prop = templ->Get(name); + if ( nullptr == prop ) { + result = false; + return T(); + } } // strong typing, no need to be lenient From aba8383283d6d5a0180236a718d1102495f20299 Mon Sep 17 00:00:00 2001 From: Tommy Date: Wed, 10 Jan 2018 19:18:11 +0100 Subject: [PATCH 2/2] Add unit test for correct FBX phong material import. --- test/models/FBX/phong_cube.fbx | Bin 0 -> 17084 bytes test/unit/utFBXImporterExporter.cpp | 30 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 test/models/FBX/phong_cube.fbx diff --git a/test/models/FBX/phong_cube.fbx b/test/models/FBX/phong_cube.fbx new file mode 100644 index 0000000000000000000000000000000000000000..851021d1c77e0480a8e29d41ae47097387c8509f GIT binary patch literal 17084 zcmc&+3y>Ved7eX$J3Wk^;Uxhl5E3Ay+mjR|6kzWy9ol>NR(C=`f#!DS?gs7dY-V@G z9c4q|kTOmPn4$_}8L+BsT)~u6b~yo;%f&+^DMMl-zd~F^@EO~&R1lX;g{Q#e`?`B( zdvAAUM<gzIU9-8TC6&CSMn#m?@3hxkB?nDc$DHvDR&y zVw+l5Kt*Su?Rs1bk|`@^_2$N$iUzG>Nr8sP%1H#k@UsF=Hi3{CVwlW3n`QtCcE>ioe$? z=C_xM6OaZ8j%e?jc3V@oIc~ZjYF^cAhOC1&?N8Lat<^MkI@zq1lbsOIFY`4sUp(kK zdCM)@R$*K0Aq|EKjGxYZ%LI#ty{Kjjm0+M-O9PL z>pY;)^_hfqQ-7-7d(b=nGuF!9DW3r_4DXN;CIV6Bol-6>hl^U{A;oWpq&QEb_*8p7 zpRrS7{*G9=H1w%Uj?@x|-9X_^+A18FR(MzA9manSemo9zynYk@pf(!$?=z>&jnE*K zNTmR2RREImAt7L#a`z~(J_*KY=Cg%aVC{i_x@BkI9w8;~MOjM8vq8`rYl*eC0Bw77 z>*gI>x9!++<)&CHHh+GF#+T{PJ$A+l8R{Rw-mXILl-MP>Af*NPNtlIV(mZIT162Mu zis4R4Lc#hZx{f02{AUXA8VPuA5MYQTJf&X@o1oI$Box9;5N{$_)zSK$3awwFomGM6 zX?d1ve_vr|B=#Eo_!gy2oc2`FK4`%?2|roe;x~Zy6x+@-dz3HK) zxWDw2E7Cutl&?Nj&b__myjYd=9 z`ui{g*qT8Z^8uD=4CmV??Sk<1Ho^#X(3>k-14nV4A~HOLpwW? zWM00;jiETIb+ZLHY}}4ZSxA>PW|lI=P6SZb4Df#|(Ht%(T}oMQ^B^+MJd!lHp)mz_ ziG(9h419fv+(~+1D_6>vgI+I&WIU=!)O_N&NCNbtlX5b($Y|QHD6%VXuT+%GnfZZS zj|;|wIz(gR983cana)HFnH?tcW)2GowIPu)8kN%~3rw&(0D^q)Eql0*o5HV;_Mu^63Gx%2{+!=!I+8giUsKP)~`t`EBV ztTwcTL)MrDU8Jl+Mf2}yT`j`owBb9jj46Ca^~urrp(~Atm$eEdb4ucT#AfoyAup5? z5-U6U*UF%{RJEL;G_)(1Iq<7NXcVPM9)&z_u8sB%td9UR!li0$HMPge<};iC)Md}; zq_33YY^F+u4ZH?)^0@&Q^huA>WffAcE%poo+!ujX1CfL=Tp;B9NjPxu;XVMZMA@gP zTkr&G>RnNXH~>j%7++GvY8p}jzV4%S|EfaRBD;qx&=DfA;IUHY_c=KFx>`<{Q`j|7 z_AtX{zO4YjZ@88P$k))?D-~LP0q8`&92dk*bKYy&;}gZ1!8b{6+q>`&%D%R@6x|v; zfsW3i5BoFXX4FA+VKebhFYnf1Zm=7zc@n`jsQF-Za~Q#apyppjb4KuXTwsPB)eqf= zzQ}Ss-c@z;6V-khH#HGzWqvVmPMDd9=HzSO}QY%>&^lM`mzxvdaH_X|aL1qdJ*n$wV#vXG0`qeROn zrRf4`3bm}L524Dn%}l$pv>8@(@_b1O8>?YOWEEDFEV*N5${OTO-|s-+l?rQm2^|o; z6&FTHv!q^>_ht#=1VF__lDmwIquM#sIv7uro=-@potK zLII+cZj12cMMj0^tz~fc1CJ!iaWLw6c-opuTS!$Po`}Du_-O~Bl0n4+s3tqoYrg2Y zPwnP>>>`mwiIl@raFMvlfSl6NxaqRT#!7`S;Z*r>->W}a&5eSK#LWkl+%V2{l(PBR z&g6dV(sFP;xFBkVmVU?0nwdUx)|mGLlk(moF=>NjQ5#;J<@D={scs2?ywBLJ0JFmw`;y{UeKX+P zOk#%t*@+vX=is@%5Pp^HX|Bh^)0hg<@rih!qE~4Xb)1~QIxJ@u3f+@=CkI7`W};i$ zz6=PeK!sZwr^YheHgR$xg93>t)zo@+TtpV^da-<0kB^T^zuN^&m;LUM$C}g;{ShCyU7G z=;f~$?maZj8piDrfc8ZJdLaVP4N-s^*2g-+6OF*Ry zx=ap|eq7@qG0Zmk{9gs%FX5SW`S6}Mm@W1IESs#W)hrhkKDM%hDA1Zg`6l)tps4{Bsu|*8 z{9QP*Ib`O>tw<5}m2iwm!F3fPL|;_59mFwDtQ8_QS1F(QrSx8WF(#cT`C)PE?L zZBkQAagdMkqj9=DH=c<$g;rs)Ms){)L>kckIv|mT^AJE7XH}@-a1xGlpuOmQ4Hh}- z%n$fKR8;T;p1BU^Wf~ev_mN^5%OcXTMTdhSw1L`{Q6@y~hE}LT~&gi$s@pTMm z?JOKIz=qF+iF47VeBQyC5yH**ph%R@RibbMLmjVaFXDLcD9?)L($-|~0WcoA_^v|1 zs(`G|$4`%IS+@s%$9$ujATiK|??tuv1e&oE3k16Teq88b34Ri(OtT)WYP~}2h3dtqna@?2wIWn2_MxbAwN1DX zx7vrR8ILs(?^g8PsKZFls-SOH>7|JS9${4im;Z=^GQ2s}(%D|7Xp{#%3g3kt&%tY2 zf0Os|GqHceUGl&U$7VEm?}e4{>*j7qf@aJu#wMJDF4{PplIHF%-y;1qCL2=wjk_98 zKELuaPkr_+b8F&)voBnC-`}nqi6_qAbl%vtH(g*H`9|~cR}@edB+Cb#%+$D(Bc3%W z%-<#?%YN~%geSWn!TQg@FYl?DlX&J-<2~*5eLQnI@SZH;@nin-nQcB)^|%#(3_<#J z_yWN;*WjAhogTLeHcgNPHYEzXBrL_ku60OS)&g9d51+ubQ^B#jC7uM;nepCF;pHJn zj{t12Tzqls&WKL~@{>w0UK@97ApTjNF;pDVD&~rlNo@Wj^_H3CS@P>jW>;w+d^7QW4xear1qbif9-#! zR#^KF?|EeCquAbGZf@N4?1lf@@pydj(*7szc=*84A3Xn?3%~xklLFrB+Z7X9X;(|$*?()(j}7~=)i&OhOE+>zPo!t+A{`zP;{1uir^tPA zT6p*$RY3(xanhQyX1;YT_Vx+664xn+^hQOt9Av>FrJV>v-{VNhKCQ_5P;tiaNE+We zMDK0*l|x_k&i;k{SGWJ_g`90 zkGvNY5%KLUM6n3|!&*9G_QJjrh`4?E^ zUT-1i&GQ=aLQed&P`K#T0~WtKIAK+(=xLc^nR}!8OAV+6qPUs6bdpRqHtoSpGf&I>wVE^%8srVDA;NgV38*1^ZlFar%Z$1y;XCyRdx1Moh-b zz>ildjOb|)QL7!eAUfKocP^&xcj0Mo2k)qNGaY{yc;pBv7x8G69$tj6!qf4dt0h0= zSE;i8IUBS^(YR3dH68zJz>ytg85~~pb2iiQpH_2I|C|kZ(RMX`DNV;$AnTjKP4sg% z)A2EoQ&-)%AR8KySR+L|gL15_B_*Gu@vke65Yh-qFJFd?bOz-_`~$_7W&(BIj_gsKx|$tP5H%p*o|@V!1Se!D z52i&y^yTFAO*tZUz{HxXPuDfGK3;w?w{=dq5H{5&cYj6MjjyI1l{#EJz%NG3~)H$jS literal 0 HcmV?d00001 diff --git a/test/unit/utFBXImporterExporter.cpp b/test/unit/utFBXImporterExporter.cpp index 975f2acef..d2415a8e5 100644 --- a/test/unit/utFBXImporterExporter.cpp +++ b/test/unit/utFBXImporterExporter.cpp @@ -46,6 +46,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include +#include +#include +#include using namespace Assimp; @@ -67,3 +70,30 @@ TEST_F( utFBXImporterExporter, importBareBoxWithoutColorsAndTextureCoords ) { const aiScene *scene = importer.ReadFile( ASSIMP_TEST_MODELS_DIR "/FBX/box.fbx", aiProcess_ValidateDataStructure ); EXPECT_NE( nullptr, scene ); } + +TEST_F( utFBXImporterExporter, importPhongMaterial ) { + Assimp::Importer importer; + const aiScene *scene = importer.ReadFile( ASSIMP_TEST_MODELS_DIR "/FBX/phong_cube.fbx", aiProcess_ValidateDataStructure ); + EXPECT_NE( nullptr, scene ); + EXPECT_EQ( (unsigned int)1, scene->mNumMaterials ); + const aiMaterial *mat = scene->mMaterials[0]; + EXPECT_NE( nullptr, mat ); + float f; aiColor3D c; + // phong_cube.fbx has all properties defined + EXPECT_EQ( mat->Get(AI_MATKEY_COLOR_DIFFUSE, c), aiReturn_SUCCESS ); + EXPECT_EQ( c, aiColor3D(0.5, 0.25, 0.25) ); + EXPECT_EQ( mat->Get(AI_MATKEY_COLOR_SPECULAR, c), aiReturn_SUCCESS ); + EXPECT_EQ( c, aiColor3D(0.25, 0.25, 0.5) ); + EXPECT_EQ( mat->Get(AI_MATKEY_SHININESS_STRENGTH, f), aiReturn_SUCCESS ); + EXPECT_EQ( f, 0.5 ); + EXPECT_EQ( mat->Get(AI_MATKEY_SHININESS, f), aiReturn_SUCCESS ); + EXPECT_EQ( f, 10.0 ); + EXPECT_EQ( mat->Get(AI_MATKEY_COLOR_AMBIENT, c), aiReturn_SUCCESS ); + EXPECT_EQ( c, aiColor3D(0.125, 0.25, 0.25) ); + EXPECT_EQ( mat->Get(AI_MATKEY_COLOR_EMISSIVE, c), aiReturn_SUCCESS ); + EXPECT_EQ( c, aiColor3D(0.25, 0.125, 0.25) ); + EXPECT_EQ( mat->Get(AI_MATKEY_COLOR_TRANSPARENT, c), aiReturn_SUCCESS ); + EXPECT_EQ( c, aiColor3D(0.75, 0.5, 0.25) ); + EXPECT_EQ( mat->Get(AI_MATKEY_OPACITY, f), aiReturn_SUCCESS ); + EXPECT_EQ( f, 0.5 ); +}