GLTFIO morph target tangents

[rawnsley]

I'm not sure that morph target tangents are being processed properly when importing GLTF models into Filament. Consider AnimatedMorphCube, which has a sloping animated face. With a single directional light source we would expect to see some variation on the sloping face as seen in this PR. Instead we see something like this:

I'm trying to do the same import in my own engine and I was hoping to copy from GLTFIO. Looking at the code I can't quite work out how it can use the same function to combine GLTF normals and tangents in the morph targets that is uses for the base primitive normals and tangents. The morph targets are deltas, but the base attributes are absolute values. I may of course be missing something subtle that is happening prior to getting to that point in the code.

[prideout]

Thanks for another excellent bug report! We have a plan for changing our implementation to be "more correct" than what we're doing now, which is hopefully sufficient to make us pass the test. One gripe I have with the spec is that interpolation scheme for normal vectors isn't really specified; see nlerp vs slerp.

Let me try to formalize this a bit. First let's pretend that we live in a simplified world where there's only one morph target and that there exists a function that transforms a normal into a tangent such that f(n)=t. We'll denote the morph weight with w0, the base normal with n, the target normal with n0, and the interpolated result with n'.

So, today we're doing:

// ResourceLoader.cpp
t = f(n);
t0 = f(n0); // this is wrong because n0 is a delta

// main.vs
n' = normalize(f'(t) + w0 * f'(t0));

We plan on changing this to:

// ResourceLoader.cpp
t = f(n);
t0 = f(normalize(n+n0)) - t;

// main.vs
n' = normalize(f'(t + w0 * t0));

This is still a bit dodgy because it involves component-wise lerping of quaternions but it seems more reasonable than we're doing today. At the very least, I think it's crucial to have correct anisotropic lighting for the base shape (when all weights are zero) and the planned fix will be sufficient for that.

[rawnsley]

@prideout this seems like a sensible way forward with no impact on current runtime performance. It would take a keen eye to detect the difference between nlerp and slerp in the normals anyway.

[romainguy]

@prideout Was this fixed?

[prideout]

This has not yet been fixed.

[prideout]

This is somewhat tangential but I believe the AnimatedMorphCube sample is non-conformant because it has VEC3 tangents, whereas the spec says that tangents must be VEC4. I added a comment about this to the following ticket in the samples repo.

https://github.com/KhronosGroup/glTF-Sample-Models/issues/170

[prideout]

Let me summarize this bug and a proposed fix one more time, just for my own sanity. This new proposal is simpler from my previous one.

The following pseudocode is grossly simplified because there is no general bijection between normals and tangents. Square brackets denote data that may or may not be available.

Here is what we're doing today, which is hugely incorrect because we are passing deltas into the orientation helper, which expects final values not deltas. (today's codebase triggers an assert for this reason)

// ResourceLoader computeTangents
// ------------------------------

for bvert in base_vertices:
    base_tangent = normal_to_tangent(bvert.normal_vec3, [bvert.tangent_vec4]);
for mvert in morph_vertices_0:
    morph_tangent_0 = normal_to_tangent(mvert.normal_delta, [mvert.tanget_delta]);
for mvert in morph_vertices_1:
    morph_tangent_1 = normal_to_tangent(mvert.normal_delta, [mvert.tanget_delta]);

Here is what I would now like the "fixed" version to look like:

// ResourceLoader computeTangents
// ------------------------------

for bvert in base_vertices:
    base_tangent = normal_to_tangent(bvert.normal_vec3, [bvert.tangent_vec4]);
for mvert in morph_vertices_0:
    bvert = get_matching_base_vert(mvert)
    temp_normal = bvert.normal_vec3 + mvert.normal_delta
    [temp_tangent = bvert.tangent + mvert.tangent_delta]
    morph_tangent_0 = normal_to_tangent(temp_normal, [temp_tangent]);
for mvert in morph_vertices_1:
    bvert = get_matching_base_vert(mvert)
    temp_normal = bvert.normal_vec3 + mvert.normal_delta
    [temp_tangent = bvert.tangent + mvert.tangent_delta]
    morph_tangent_1 = normal_to_tangent(temp_normal, [temp_tangent]);

I think our vertex shader can remain as is, which simply does:

base_normal = tangent_to_normal(base_tangent)
morph_normal_0 = tangent_to_normal(morph_tangent_0)
morph_normal_1 = tangent_to_normal(morph_tangent_1)
final_normal = normalize(base_normal + weight_0 * morph_normal_0 + weight_1 * morph_normal_1)

Yes, componentwise lerping of normal vectors is wrong but it's not a big deal.