Add gram-schmidt fast approximation tangent generation algorithm

[aloucks]

Provide a fast alternative algorithm for tangent generation

Fixes: #17834
Related (maybe): #17877

Solution

Add a new enum TangentStrategy that gives the user the option to select how tangents are generated.

The GLTF loader now has a property to set which algorithm is used for models that are missing tangents.

The FastApproximation strategy is implemented with the Gram-Schmidt fast approximation technique. The HighQuality variant uses MikkTSpace.

Testing

Unit tests have been added to assert that both tangent computation strategies produce similar tangents for simple shapes. I also tried to check several examples to verify that the visual results are acceptable with both algorithms. I wasn't able to tell any visible difference.

How can other people (reviewers) test your changes? Is there anything specific they need to know?

I checked the deferred_rendering, parallax_mapping, and anisotropy examples. Suspiciously, when I produced intentionally invalid tangents, the deferred_rendering and parallax_mapping examples seemed to render fine. I didn't try the anisotropy example with intentionally invalid values. I'm curious if there is an unrelated bug in the shader where tangents are ignored (or the examples I tested somehow didn't leverage tangents).

Migration Guide

• GenerateTangentsError::MikktspaceError has been renamed GenerateTangentsError::AlgorithmError
• The Mesh::generate_tangents and Mesh::with_generated_tangents methods are now deprecated. They are replaced by compute_tangents and with_computed_tangents, both of which accept a TangentStrategy parameter. The naming conventino of these methods now matches compute_normals.

[JMS55]

Suggested change

	pub computed_tangent_strategy: TangentStrategy,
	pub tangent_calculation_strategy: TangentCalculationStrategy,

[aloucks]

renamed

[hukasu]

the anisotropy example does use tangents, this issue shows some issues caused by not optimal tangents #16179

[hukasu]

i was making tangent generation for the primitives, but with a very naive algorithm #17691, if your method works without causing seams then my PR becames redundant

[aloucks]

the anisotropy example does use tangents, this issue shows some issues caused by not optimal tangents #16179

The sphere looks much better with the gram-schmidt tangents. I actually left Sphere out of the tests that I added because there was at least one vertex where the mikktspace generated tangent differed from the gram-schmidt tangent enough that it was outside of the test threshold.

mikktspace

sphere_mikktspace.mp4

gram-schmidt

sphere_gramschmidt.mp4

[hukasu]

Can the strategy names be changed to just the name of the algorithm? Mikktspace is very dependant on the UVs, so just calling it HighQuality might be misleading

[aloucks]

Can the strategy names be changed to just the name of the algorithm? Mikktspace is very dependant on the UVs, so just calling it HighQuality might be misleading

Bikeshedding time! 🎨

@JMS55 had suggested to inject Calculation into the name, but I think it's less necessary if we change Strategy to Algorithm. The word "algorithm" implies the "calculation" part, imo. Also, do we want to keep HighQuality/Mikktspace the default?

@JMS55 / @hukasu what are your thoughts?

pub enum TangentAlgorithm {
  Mikktspace,
  GramSchmidt,
}

[aloucks]

@JMS55 Anymore feedback on the enum and variant naming? Do we want to leave mikktspace the default or change to gram-schmidt?

[IQuick143]

Is there a numerical reason to normalize here?
The next line already involves a normalization and is linear in the tangent variable.

On the other hand, I don't see the normal getting normalised, and that would cause problems in the G-S algo.

[aloucks]

Is there a numerical reason to normalize here?
The next line already involves a normalization and is linear in the tangent variable.

The examples I've seen either normalize the accumulated tangent it or average it by dividing by the number of vertices that were used to accumulate it. I don't think the direction would be correct if we only normalized the final result and omitted the normalizing here.

On the other hand, I don't see the normal getting normalised, and that would cause problems in the G-S algo.

This assumes that the vertex normals are normalized. Is that assumption false? If so, I'll need to normalize the normal here.

[IQuick143]

I'm not familiar with the code enough to determine whether the normals are normalised, although I would expect so.

Regarding the pre-normalisation of the tangent, that part is mathematically unnecessary as the projection is linear.

tangent = P(t).normalise()

where P is the projection operator x -> x - x.dot(normal) * normal, which is linear.

This tells us that if t is not normalised by a positive factor k we have:

P(k*t).normalise() = (k * P(t)).normalise() = P(t).normalise()

Where the first equality comes from the linearity of P, and the second from normalisation destroying these scalar factors.

So from a purely theoretical standpoint the normalisation has no effect.

The only reason I can think of for the normalisation would be to avoid some sort of numerical error, although I cannot really think of the reasons it would be relevant here given how floating point values work.

[aloucks]

@IQuick143

How about this?

let normal = Vec3A::from_array(normals[i]);
// Assume that the normal has not been pre-normalized
let normal = normal.normalize_or_zero();
// Although it is not mathematically necessary to normalize the accumulated
// tangent, doing so may provide better numerical stability in extreme cases.
let tangent = tangents[i].normalize();
// Gram-Schmidt orthogonalization
let tangent = (tangent - normal * normal.dot(tangent)).normalize();

[IQuick143]

You can save a few operations (a square root and some divides) by doing:

let tangent = (tangent - normal * (normal.dot(tangent) / normal.magnitude_squared())).normalize();

Instead of normalising the normal.

[aloucks]

You can save a few operations (a square root and some divides) by doing:

let tangent = (tangent - normal * (normal.dot(tangent) / normal.magnitude_squared())).normalize();

Instead of normalising the normal.

Unfortunately that causes other issues when the normal is broken and has length of zero. The cone unit test fails. Using normalize_or_zero instead of normalize when normalizing the normal guards against that case.

Using your suggestion above (or using normalize instead of normalize_or_zero) results in a tangent that has all NaN values.

thread 'gramschmidt::tests::cone' panicked at crates\bevy_mesh\src\gramschmidt.rs:172:13:
tangents differ significantly: hq = Vec3A(1.0, 2.0480125e-5, -1.684416e-8), fa = Vec3A(NaN, NaN, NaN), angle.to_degrees() = NaN

There's probably an argument to be made that the tangent is likely wrong in either case, but I guess this way it at least aligns with what mikktspace would have produced and doesn't contain NaN values.

[aloucks]

@hukasu I renamed the enum to TangentAlgorithm and the associated variants.

Any chance this can land before 0.16 is released?

[hukasu]

i don't have authority to say when it can be merged

[aloucks]

@alice-i-cecile Is this too late for 0.16?

[alice-i-cecile]

Yeah, we're down to "essential bug fixes and polish" at this stage, sorry!

[aloucks]

@alice-i-cecile Now that 0.16 is out, could this be considered for 0.17?