determine if normals are flipped by a reflection in the normalMatrix instead of object.matrixWorld

[Korijn]

Determine if normals are flipped by a reflection in the normalMatrix instead of object.matrixWorld.

Fixes #14372.

This makes more sense, since the normalMatrix is the end-all transform that determines if the normals will be flipped or not. It's also more efficient, since the determinant of a Matrix3 is much cheaper to compute than that of a Matrix4.

[mrdoob]

What if the object that has the reflection is a parent?

[Korijn]

The normalMatrix is taken from the modelViewMatrix which is the product of the camera.matrixWorld and object.matrixWorld, so all parent transforms are included. This behaviour is correct and is not changed by this PR.

[mrdoob]

/cc @WestLangley

[mrdoob]

@Korijn Do you mind removing the builds commit?

[Korijn]

Yes! @berendkleinhaneveld is working on it

[Korijn]

@mrdoob CI failed due to an npm install timeout but as you can see the PR passed on the earlier commit that has identical changes. If you want to see CI pass, you'll have to re-run travis, since we don't have permission for that.

[Korijn]

Even if we disagree about the use case for negative camera scaling, I think we can agree on the fact that it makes more sense to check the determinant of the normalMatrix for this particular code path? :)

[mrdoob]

Even if we disagree about the use case for negative camera scaling, I think we can agree on the fact that it makes more sense to check the determinant of the normalMatrix for this particular code path? :)

Yes, I agree with that.

I'll let @WestLangley decide on this one though.

[WestLangley]

@mrdoob This is a good suggestion. Thanks @Korijn.

[mrdoob]

Thanks!

[WestLangley]

Note: This has been reverted in #15825. Reflections in the camera matrix (or camera world matrix) are not supported.

[Korijn]

Alright, too bad. Thanks for the heads up. We'll resort to workarounds then.

[WestLangley]

Sorry, @Korijn about reverting your feature. Sometimes we have to make judgment calls, and this was one of them. Glad to hear you have workarounds available.

[haxiomic]

Just to add a point of view on this one:

It makes a lot of sense to support reflections in camera matrices as this is the canonical approach to implement planar reflections (you take a camera's transform, reflect by your mirror plane and done :))
e.g: https://developer.arm.com/docs/100140/0302/advanced-graphics-techniques/combining-reflections/about-combining-reflections

The original normal matrix approach was the best from an overall correctness point of view. I think #15825 would be better fixed by a different approach. I expect this will lead to less bugs in the future

I'm on a full-time project right now so I'll just fork and restore the normalMatrix version for now but I'll take a look at fixing #15825 when I get a moment