guillaumeblanc / ozz-animation

Open source c++ skeletal animation library and toolset
http://guillaumeblanc.github.io/ozz-animation/
Other
2.46k stars 302 forks source link

Skinning job doesn't preserve tangent handedness #164

Open sherief opened 1 year ago

sherief commented 1 year ago

When using a format with the tangent space handedness stored in the tangent's w component, much like the sample mesh format, skinning job doesn't preserve the handedness in the out tangent array. The cause seems to lie in the tangent tranformation code:

  TRANSFORM_PN_INNER();                                                   \
  const math::SimdFloat4 in_t = math::simd_float4::LoadPtrU(in_tangents); \
  const math::SimdFloat4 out_t = TransformVector(it_transform, in_t);     \
  math::Store3PtrU(out_t, out_tangents);

and

  TRANSFORM_PN_OUTER();                                                    \
  const math::SimdFloat4 in_t = math::simd_float4::Load3PtrU(in_tangents); \
  const math::SimdFloat4 out_t = TransformVector(it_transform, in_t);      \
  math::Store3PtrU(out_t, out_tangents);

Regardless of how in_t is loaded, only the first 3 elements are stored into out_tangents, so in_t.w is undefined in the output array. When passing this to the GPU and computing bitangent as cross(normal, tangent.xyz) * tangent.w, the value is corrupted.

I have a local fix that Works In My Case (tm), but I'd like to know what an ideal fix would be and I'd be happy to incorporate it into the library. My local fix is an SSE only hack with no ref implementation, no NEON variant, etc.

guillaumeblanc commented 8 months ago

Hi,

the behavior you're describing is actually expected. The skinning job only expects 3 components for tangents, as described in the documentation.

The reasoning is:

  1. it does not force tangents to have a handedness component, and not necessarily at w position.
  2. handedness are constant values, aka not transformed by the skinning process. Like vertex colors or uvs, they shall be copied to an immutable vertex buffer to be available in shader code.

Does it make sense for your use case?

Cheers, Guillaume