Backface culling winding order reversed

[hyazinthh]

Currently, backface culling does not work as intended as the winding order is reversed. For example, applying Sg.cullMode' CullMode.Back and Sg.frontFace' WindingOrder.Clockwise will lead to triangles with CW winding order being culled. This behavior is consistent in both backends:

• In GL we translate front to back and vice-versa (https://github.com/aardvark-platform/aardvark.rendering/blob/master/src/Aardvark.Rendering.GL/Instructions/Translations.fs#L74)
• In Vulkan we translate the state directly, but since Vulkan NDC space is right-handed we get the same result as in GL.

Changing the behavior is not an option, so I suggest we simply rename the front face state to back face (e.g. Sg.backFace instead of Sg.frontFace).

[haraldsteinlechner]

wow. cool we are cleaning up on this one, and thanks for the analysis of this relict. What are the drawbacks of your approach -- Clearly confusion is one. Maybe we could accompany it with a consolidated page on conventions (and refer to it in the comments). This could then be extended towards: right-handed left handed, row-column major matrices and memory layout, uv space, z-up, depth ranges, automatic depth transformations, image layout - those come to my mind quickly.

[hyazinthh]

Drawback is that you have to know that you can just replace Sg.frontFace with the new Sg.backFace. I'd mark the old function obsolete warning that it has been renamed. That way transitioning should be painless. Also this rasterizer state is called front face in both Vulkan and GL, so naming it back face is a bit unintuitive probably.

[luithefirst]

Changing the name to Sg.backFace seems like one step forward and one step back (although not as bad as having flipped culling behavior). We could drop the obsolete attribute of Sg.frontFace in 5.5 and everything is fine, we would then just have both versions?

I also don't think that many projects made use of Sg.frontFace (I introduced it for a Hilite use case)?

[haraldsteinlechner]

well, with this strategy when you move from say 5.3 to 5.5 you won't recognize the change.

I also don't think that many projects made use of Sg.frontFace?

Most likely, but if so, it is even harder to find. Maybe what lui proposed, but just keep the warning longer?

[luithefirst]

One more thing: I assume if we try to keep the current behavior, we would change the default value of Sg.frontFace, and then all projects that use Sg.cullMode do not have a changed behavior. I we want a clean API it would be ideal to have defaults like in a certain graphics API (e.g. OpenGL). I just would have guessed counter clockwise is the default back face, but its actually clockwise in GL (DX is ccw, that's were we come from).
Probably very few know what the defaults are so one more vote to create a convention page like Harri suggested (we should then also highlight to what graphics API default values our match) 👍

Note: I didn't find Sg.frontFace in any repository I have checked out (except the definitions in rendering and media).

[hyazinthh]

It's not just Sg.frontFace by the way, this would also affect any places where someone builds ROs and the pipeline state manually (see https://github.com/aardvark-platform/aardvark.rendering/blob/master/src/Aardvark.Rendering/Pipeline/PipelineState.fs#L66)

I don't think it's worth the hassle to risk silent breaking changes just to match the naming convention. We could rename the rasterizer state, keep the "broken" Sg functions around and eventually remove the obsolete warnings some major versions down the line.

[krauthaufen]

Btw in most regards we're using OpenGL conventions (texcoords, z-range, etc) so it would be natural (and easy in OpenGL) to stick with these conventions.

Also Sg.frontFace is rather new(ish) and i don't think many projects use it (although many may rely on the default setting)

Maybe just rename it to Sg.frontFaceWinding and correct it to make all the problems apparent?

[hyazinthh]

Another alternative name is Sg.frontFacing which would be consistent with the GLSL input gl_FrontFacing. So I'd be in favor of marking the old functions as obsolete and adding frontFacing variants. For the RasterizerState record we can't really create an alias, so just rename and add an obsolete FrontFace member?

aardvark.rendering/src/Aardvark.Rendering/Pipeline/PipelineState.fs

Lines 63 to 70 in e2f0891

	type RasterizerState =
	{
	CullMode : aval<CullMode>
	FrontFace : aval<WindingOrder>
	FillMode : aval<FillMode>
	Multisample : aval<bool>
	ConservativeRaster : aval<bool>
	}

[hyazinthh]

I'll write some unit tests for this. The way we translate the two states in Vulkan is not completely consistent with the GL backend. Currently, gl_FrontFacing will have different values in Vulkan and GL. We need to flip both the front face and cull mode state in Vulkan to get consistent results.

[hyazinthh]

Should be fixed with 640a7e2:

Sg.frontFace is now obsolete but still works, as it simply reverses the input and passes it to the new Sg.frontFacing. In shaders gl_FrontFacing has the correct value now.