Support GL face culling

[brunoabinader]

This is a common OpenGL optimization technique that skips rendering e.g. back-facing triangles. For that, we need to apply a common winding order (either clockwise or counter-clockwise) when ordering the triangle vertices we use when drawing.

When enabling face culling, GL culls back-facing triangles and assumes front-facing triangles being ones ordered counter-clockwise by default. This required some changes in our winding order when populating our buckets.

We enable face culling by default. We apply counter-clockwise winding order for culling back-facing triangles when rendering in the main buffer, and clockwise winding order for culling front-facing triangles when rendering offscreen.

Some render tests have changed expectations (IMO for the better):

fill-extrusion-pattern/@2x

previous	actual

fill-extrusion-pattern/function

previous	actual

fill-extrusion-pattern/literal

previous	actual

fill-extrusion-pattern/opacity

previous	actual

line-opacity/property-function

previous	actual

GL native implementation: mapbox/mapbox-gl-native#12725

[brunoabinader]

@jfirebaugh as predicted, the updated results in the fill-extrusion render tests indicates we no longer render the back-facing sides of the fill extrusion.

[brunoabinader]

Please review @mollymerp @mourner @jfirebaugh

[brunoabinader]

Benchmark results:

[ansis]

We apply counter-clockwise winding order for culling back-facing triangles when rendering in the main buffer, and clockwise winding order for culling front-facing triangles when rendering offscreen.

This is a great improvement. Is there an advantage to using a different winding for offscreen and main buffer rendering? It seems like it would be simpler to have one consistent winding order that is always applied.

Also, the only layer type where we would expect to cull faces is the fill-extrusion layer. Does it make sense to enable culling and enforce winding order for all these layers even if it results in no triangles being culled?

[brunoabinader]

This is a great improvement. Is there an advantage to using a different winding for offscreen and main buffer rendering? It seems like it would be simpler to have one consistent winding order that is always applied.

No advantage, just semantics it seems - in my first attempt, I applied the same winding order for all buckets, no matter if it was rendered offscreen or not, but couldn't get offscreen rendering done properly.

It turns out that some stuff we render is on the back-facing side by default e.g. when hillshade prepares the texture in the framebuffer, it uses a projection matrix that is inverted on the Y-axis, which also inverts the winding order, the same happens for some line vertices that seems to be inverting the Z-axis when transforming to NDC. These axis inversions cause the winding order to invert as well. So instead of applying a workaround for hillshade and line buckets only, I came up with the idea of inverting the winding order by default when rendering offscreen. But I should probably only apply the inverted winding order when needed, good point 👍

Also, the only layer type where we would expect to cull faces is the fill-extrusion layer.

Yes, fill extrusion and line, to be exact.

Does it make sense to enable culling and enforce winding order for all these layers even if it results in no triangles being culled?

I do think it is a good idea - enabling face culling by default imposes no performance penalty, and enforces us to adopt a strict check on the winding order :-)

[brunoabinader]

Does it make sense to enable culling and enforce winding order for all these layers even if it results in no triangles being culled?

I do think it is a good idea - enabling face culling by default imposes no performance penalty, and enforces us to adopt a strict check on the winding order :-)

The alternative solution is to have an extra parameter in program.draw() for setting the face culling mode for each program, in order to disable it for layers that doesn't need it.

[mollymerp]

looks good pending a 👍 from @ansis

[tmpsantos]

Can we get this reviewed? It is blocking a PR on Mapbox GL Native.

[ansis]

@brunoabinader sorry about the delay reviewing.

I'm a weak 👎 on enforcing winding order for layers that don't need it. It adds another small thing that people working with the code need to keep in mind but doesn't provide any benefit for most layers/draws.

The alternative solution is to have an extra parameter in program.draw() for setting the face culling mode for each program, in order to disable it for layers that doesn't need it.

This would be my personally preferred solution. Keeping the culling state closer to the draw call might make it a bit less of a trap than global culling

Yes, fill extrusion and line, to be exact.

While it makes the artifacts for translucent line layers slightly better it doesn't really fix them so I'd be fine with not culling for line layers either.

Overall, I'd prefer your alternative that changes program.draw() but I also think it might be reasonable to disagree on this. Do you have a strong preference for enforcing it everywhere?

[ansis]

@mollymerp can you review 63e8081? is this approach preferable to global culling?

(note: 63e8081 reverts some changes from previous commits so it might be easier to review by looking at the pr as a whole)

[mollymerp]

yep I think this is preferable to enforcing ccw winding for all layers! LGTM.

[anderco]

@brunoabinader is away and I agreed with @nagineni that I would follow up on this.

I find the alternative of enabling face culling only for the fill extrusion layer reasonable so I went ahead and implemented it: https://github.com/mapbox/mapbox-gl-js/tree/anderco-gl-face-culling (on a separate branch so I don't overwrite Bruno's pull request).

[mollymerp]

oh @anderco did you miss that @ansis implemented those changes in 63e8081 ? This PR is approved to merge pending a rebase to resolve conflicts.

[anderco]

oh @anderco did you miss that @ansis implemented those changes in 63e8081 ? This PR is approved to merge pending a rebase to resolve conflicts.

Should have hit F5 🤦‍♂️ I had a tab open with this PR open and really didn't notice the new patch and comments. But well, at least I got to know the code better.

[ansis]

Should have hit F5 🤦‍♂️ I had a tab open with this PR open and really didn't notice the new patch and comments. But well, at least I got to know the code better.

Yep, your changes look pretty much identical to mine. I just squashed and rebased on master. @anderco did you want to review this before merging?

[anderco]

Yep, your changes look pretty much identical to mine. I just squashed and rebased on master. @anderco did you want to review this before merging?

Nope, that's fine. I had looked it over earlier and I agree that my changes were pretty much identical.