OGRECave / ogre-next

aka ogre v2 - scene-oriented, flexible 3D C++ engine
https://ogrecave.github.io/ogre-next/api/latest
Other
1.02k stars 224 forks source link

Normals not handled correctly for non-uniform scaling #373

Open jwwalker opened 1 year ago

jwwalker commented 1 year ago

System Information

Detailed description

When the model-view transformation includes a non-uniform scale, the correct way to handle normal vectors is to multiply by the inverse transpose of the upper left 3x3 part of the model-view matrix. As far as I can tell, this is not what happens in Ogre. In 800.VertexShader_piece_ps.any, I see the line

@property( hlms_normal || hlms_qtangent )   outVs.normal    = mul( @insertpiece(local_normal), toMidf3x3( worldViewMat ) );@end

which seems to be just multiplying the local normal by the 3x3 part of the model-view matrix.

A visual illustration: First, an unscaled cylinder using lit PBS shading. shot_00002 - Scene 1, Shot 2  unscaled, Ogre

Here's what it looks like when the cylinder is stretched in the horizontal direction perpendicular to the camera's viewing direction: shot_00004 - Scene 1, Shot 4  scaled, Ogre (Notice that the specular highlight is basically unchanged, in spite of the surface being flatter there.)

And here is what I think it ought to look like, as rendered by a different rendering engine: shot_00003 - Scene 1, Shot 3  stretched, classic

darksylinc commented 1 year ago

There's discussion here: https://forums.ogre3d.org/viewtopic.php?t=96656

Most people would like to simply ignore this problem due to the performance implications.

But for those who want correctness, the suggested:

@property( accurate_non_uniform_scaled_normals )
   normalMat = transpose( inverse( worldMat ) );
@end

could be a good solution (note: IIRC Metal and HLSL don't implement inverse() as an intrinsic since it's expensive).

And then the normals are multiplied by normalMat, instead of worldMat (when disabled normalMat can be a macro to worldMat).

But IIRC our normals are in view space (a decision I regret) but I think transpose( inverse( worldViewMat ) ); should still work.

jwwalker commented 1 year ago

@darksylinc OK, I looked at the forum thread, but I don't quite understand why it's such a big deal for performance, since the 3x3 normal matrix does not need to be computed and sent to shaders any more often than the 4x4 model-view matrix. Since I am interested in using Metal and Direct3D render systems, doing it in the vertex shader is not going to work for me.

If I wanted to add that to my fork of Ogre, can you give me any clue on how uniforms are sent to shaders? I don't understand about UNPACK_MAT4 and all that.

darksylinc commented 1 year ago

If I wanted to add that to my fork of Ogre, can you give me any clue on how uniforms are sent to shaders? I don't understand about UNPACK_MAT4 and all that.

I will reiterate this is a bad idea. If you need resources on that end, you can see this entry in the FAQ.

As to why it's a bad idea:

Since I am interested in using Metal and Direct3D render systems, doing it in the vertex shader is not going to work for me.

I apologize, I forget not everyone realizes has what is going on in my mind: Metal & HLSL don't have inverse() intrinsics but you can implement it via code using Matrix3::inverse and/or Matrix4::inverse as reference.

The reason I mentioned GLSL having it as an intrinsic is because for a quick prototype it is easier and faster to write, since you know for certain GLSL's inverse() works and you can focus on the actual algorithm.

When writing your version with HLSL/Metal you have to first make sure everything is alright (most notably row-major vs column-major and order of operands supplied to float3x3 constructors).

Note that you have helpers like toFloat3x3 and buildMatrix3x3 defined in each CrossPlatformSettings_piece_all variant.

Furthermore calculating it in the vertex shader, although more expensive in terms of ALU computation, due to how Hlms works, it can be toggled dynamically without much cost for those who need it vs those who don't.

It could be made a global toggle (i.e. per pass), a material toggle (all materials using it), or even made per Renderable (i.e. only that specific object must always do correct normals).

Note: The reason Matrix3::inverse (and 4x4 too) can return failure is because not all matrices have an inverse. Most notably a matrix full of zeroes doesn't have one (i.e. it's similar to normal multiplication: 0 * x = y -> x = y / 0 or as in exponentiation x⁰ = y -> x = ?? ) and trying to calculate one could result in 0s, Infs or NaNs. But we can ignore that because a regular well-constructed world/worldView/worldViewProj matrix should always have a valid inverse.

jwwalker commented 1 year ago

@darksylinc I have implemented changes to Hlms/Pbs/Any/Main/800.VertexShader_piece_vs.any controlled by a property accurate_non_uniform_scaled_normals, and it's looking right with Metal, Direct3D, and Vulkan. If I were to make a PR for this, should there be any API to control it, other than setting accurate_non_uniform_scaled_normals to 1 in an Hlms listener?

darksylinc commented 1 year ago

Now that you've merged your PR, I forgot to ask: Where do you think this toggle is best?

  1. Per Item: Possible but a bit clunky to support & maintain from OgreNext side
  2. Per Material: Easy
  3. Global: also easy

The main benefit of being per material is that you can toggle it for some materials that will use Items that need it; without sacrificing the perf cost to everything else.

The main benefit of global is that you don't have to worry about forgetting to toggle it on for a material/Item pair that needs it; but you penalize all the objects that don't require it.

jwwalker commented 1 year ago

@darksylinc Per material would not be good for me. I have a cache of materials, and one material may be used for some items that are non-uniformly scaled and some that are not. Per item would probably be best for me, but I don't mind it being global.

darksylinc commented 1 year ago

I have a cache of materials, and one material may be used for some items that are non-uniformly scaled and some that are not

You don't have to strictly apply it to just the ones that need it.

You can toggle it on if one Item requires it, and if the other Items don't; then those Items are penalized. But other Items using other materials won't be penalized.

Does that scheme make sense?

jwwalker commented 1 year ago

@darksylinc Yes, I see what you mean, I could do that.