Fix normals during mesh scaling

[lynn-lumen]

Objective

• Fixes scaling normals and tangents of meshes

Solution

• When scaling a mesh by Vec3::new(1., 1., -1.), the normals should be flipped along the Z-axis. For example a normal of Vec3::new(0., 0., 1.) should become Vec3::new(0., 0., -1.) after scaling. This is achieved by multiplying the normal by the reciprocal of the scale, cheking for infinity and normalizing. Before, the normal was multiplied by a covector of the scale, which is incorrect for normals.
• Tangents need to be multiplied by the scale, not its reciprocal as before

[alice-i-cecile]

Can you please add a regression test for this? This seems very testable, and important to make sure we don't screw up.

[geckoxx]

Scale is also handled in transform_by. So it should apply there too.

[alice-i-cecile]

Great, thanks! A couple comments in the test to explain why the values are correct would be helpful, but I won't block on it.

[IceSentry]

Shouldn't this also scale tangents?

Either way, I'm not sure this is correct, but I don't really understand the covector_scale thing.

@Jondolf any opinions since you wrote the initial code for transform_by?

[Jondolf]

I'm pretty sure this is wrong. Just multiplying the normal by the scale does not take non-uniform scaling into account properly, and can lead to the normal no longer being perpendicular to the surface. This article illustrates it pretty well.

In most resources I've seen, including the one I linked, the "correct" way to transform normals in a way that accounts for non-uniform scaling is to use the transpose of the inverse of the transformation matrix. I imagine the covector scale thing is supposed to achieve the same thing without having to compute the transformation matrix and do a bunch of operations on it, but honestly I'm not that knowledgeable on the math / don't remember it well enough right now.

For background on the covector scale thing (originally I also used naive vector scaling), see this thread and the comments by @atlv24.

[IceSentry]

Could the covector_scale be multiplied with a sign only vector? That way it would flip things but not rescale anything?

[lynn-lumen]

I'm pretty sure this is wrong. Just multiplying the normal by the scale does not take non-uniform scaling into account properly, and can lead to the normal no longer being perpendicular to the surface. This article illustrates it pretty well.

The article suggests that for a given scale $s = (s_x, s_y, s_z)$, one should multiply the inverse of the scale $s^{-1} = (\frac{1}{s_x}, \frac{1}{s_y}, \frac{1}{s_z})$ by the normal $n$ to receive the scaled normal $n' = s^{-1} \cdot n$ which will then have to be normalized again.

Reasoning

Suppose the scaling transform matrix $M$ for any vertex in the mesh is

$$M = \begin{pmatrix} s_x & 0 & 0 & 0\\\ 0 & s_y & 0 & 0\\\ 0 & 0 & s_z & 0\\\ 0 & 0 & 0 & 1 \end{pmatrix}$$

Then the correct transform matrix to apply to the normals would be the transpose of the inverse of the matrix:

$$M^{-1T} = M^{-1} = \begin{pmatrix} \frac{1}{s_x} & 0 & 0 & 0\\\ 0 & \frac{1}{s_y} & 0 & 0\\\ 0 & 0 & \frac{1}{s_z} & 0\\\ 0 & 0 & 0 & 1 \end{pmatrix}$$

When applying $M$ to any normal $n$, we get

$$n' = M^{-1T} \cdot n = \begin{pmatrix} \frac{n_x}{s_x} & \frac{n_y}{s_y} & \frac{n_z}{s_z} & 1 \end{pmatrix}$$

This is equivalent to componentwise multiplication with the $n$ and the inverse of $s$, $s^{-1}$.

When $s_n = 0$, no inverse matrix exist. But since we are normalizing anyways after this operation, we can compensate by using $s^{-1} = (a_1, a_2, a_3)$ where $a_i = 0$ for all $i \neq n$ and $a_n = 1$

Is everyone ok with this?

[NthTensor]

Hey, thanks for looking in to this and taking the time to write out our reasoning. It was very helpful.

I'm sure multiplying by scale is wrong. We do want to multiply by inverse scale. Speaking of which, the following two lines are equivalent.

let scaled_normal = (normal.xyz * scale.yzx * scale.zxy).normalized();
let scaled_normal = (normal.xyz / scale.xyz).normalized();

To verify, expand the the latter and then simplify.

So it looks like dividing by scale as proposed above would be the same as what we have currently, except perhaps with slightly increased numerical error.

Edit: I think theres an absolute value sneaking in there during the simplification.

[lynn-lumen]

Speaking of which, the following two lines are equivalent.

let scaled_normal = (normal.xyz * scale.yzx * scale.zxy).normalized();
let scaled_normal = (normal.xyz / scale.xyz).normalized();

Hi, you are absolutely right, a sign is dropped, but that is not the only issue. Please consider the following example:

let normal = Vec3::new(1., 1., 0.).normalize();
let scale = Vec3::new(1., 1., 0.);

//  When scaling this normal, the result should still be Vec3::new(SQRT_2, SQRT_2, 0.) 
// since that is what the result would be for the scale Vec3::new(1., 1., s) as s approaches 0
// This method yealds Vec3::ZERO though since its z component is 0. / 0. == NaN and the Vec3 can't be normalized
let scaled_normal = (normal / scale).normalize_or_zero();

// Likewise, and more importantly / frequent, the following scaled_normal should  be Vec3::new(0., 0., 1.) but is Vec3::ZERO instead.
let normal = Vec3::new(1., 1., 0.0001).normalize();
let scaled_normal = (normal / scale).normalize_or_zero();

[lynn-lumen]

I am also not sure that tangents are correctly calculated either. I think that tangents should just be multiplied by the scale and not the covector scale. I am not sure about that though

Edit: I am pretty sure now 😅

This shows an unscaled normal and tangent

This image shows the same normal and tangent multiplied by the scale $(2,1)$ or its reciprocal. Please note that multiplying the tangent by scale seems to be correct

[NthTensor]

Tangents are covariant, so they should be scaled. I suspect with the normals we should just multiply by a sign vector, and keep the covector form.

Here, I'll try to make time for a proper review.

[atlv24]

Yeah this is the correct tangent behavior. I suggest PR'ing the normalize_or_zero fix to glam itself.