Fix the normal matrix when det < 0. #3105
Conversation
|
For future reference, the following commit is when we switched from transpose(inverse()) to cof() |
| @@ -199,6 +199,12 @@ void FScene::updateUBOs(utils::Range<uint32_t> visibleRenderables, backend::Hand | |||
| mat3f m = mat3f::getTransformForNormals(model.upperLeft()); | |||
| m *= mat3f(1.0f / std::sqrt(max(float3{length2(m[0]), length2(m[1]), length2(m[2])}))); | |||
|
|
|||
| // Since our "getTransformForNormals" matrix omits the divide-by-determinant step, | |||
| // it has the wrong sign when the determinant is negative. | |||
| if (sceneData.elementAt<REVERSED_WINDING_ORDER>(i)) { | |||
There was a problem hiding this comment.
Should we do this in getTransformForNormals?
There was a problem hiding this comment.
Maybe, but if we move it then we will be computing the determinant twice, which is somewhat redundant. I'm on the fence. @pixelflinger : you made the original switch to cof(), what do you think?
There was a problem hiding this comment.
Also note that getTransformForNormals is used in a couple other places, e.g. when determining dominant light direction. So yeah maybe we should make it slower and just add a divide-by-det in there.
There was a problem hiding this comment.
Dale Weiler's GitHub post makes the argument that we should just use the raw cofactor matrix, but I didn't understand one of the steps in his algebra. Moreover Lengyel never makes such a claim. In practice, dividing the cof by the det (which is exactly equivalent to the inverse-transpose) seems to fix this particular issue.
prideout
force-pushed
the
pr/fixmirror
branch
2 times, most recently
from
37d4afe
to
8eb0e05
Compare
prideout
force-pushed
the
pr/fixmirror
branch
from
8eb0e05
to
dd8c65c
Compare
|
Actually, let me close and research this a little more. Using the cofactor matrix should be correct. |
This was tested by replacing the node 0 scale in BusterDrone with [-1, 1, 1].
Fixes #3001.