-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support GL face culling #7178
Support GL face culling #7178
Conversation
1774ec9
to
ca3ee2a
Compare
280281d
to
6b60f98
Compare
@jfirebaugh as predicted, the updated results in the |
Please review @mollymerp @mourner @jfirebaugh |
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? |
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 👍
Yes, fill extrusion and line, to be exact.
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 :-) |
6b60f98
to
5dbb24e
Compare
The alternative solution is to have an extra parameter in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good pending a 👍 from @ansis
8b1e393
to
105c69c
Compare
Can we get this reviewed? It is blocking a PR on Mapbox GL Native. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
35c64ef
to
63e8081
Compare
@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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep I think this is preferable to enforcing ccw winding for all layers! LGTM.
@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). |
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. |
63e8081
to
650c58b
Compare
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. |
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
fill-extrusion-pattern/function
fill-extrusion-pattern/literal
fill-extrusion-pattern/opacity
line-opacity/property-function
GL native implementation: mapbox/mapbox-gl-native#12725