Skip to content
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

Add additional resets for culling state #10412

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented Feb 25, 2021

Resolves: #10372

Sky rendering may change the culling state. @astojilj pointed out that this code which ought to put WebGL in a predictable state before handing rendering off to custom layers doesn't apply consistent frontFace and cullFaceSide state, depending on whether the sky is rendered or not.

// The default values for this state is meaningful and often expected.
// Leaving this state dirty could cause a lot of confusion for users.
this.context.cullFace.setDefault();

This PR additionally resets context.frontFace and context.cullFaceSide to their defaults in context.reset(). It wasn't obvious to me how to test this, though I did check that it solves the codepen from #10372.

I'm a bit ambivalent about this change. It makes things more predictable, but it feels it risks snowballing into setting all of the WebGL state before handing off to a custom layer. Three.js has a new method called resetState() (as a result of buggy Mapbox interactions!) which fixes this problem by performing a more complete reset. I've updated the Mapbox GL-JS/Three.js example to use that function since it seems like the most robust way moving forward to minimize problems.

There are two options, I guess:

  1. Merge this fix since it's still preferable to put GL in a more predictable state. This may still leave gaps in the GL state reset.
  2. Omit this fix and make weaker guarantees about the state when handing off to a custom layer, putting the burden on third party libraries to enforce the state they need.

I think my preference is for 1 for this particular issue, but with an eye on this getting out of hand when third party libraries can perfectly well enforce the state they require.

cc @astojilj @karimnaaji @arindam1993 @mourner
(cc's probably not spot on; still trying to figure out how best to identify the most interested parties)

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Reset WebGL culling state before drawing custom layers</changelog>

Copy link
Contributor

@astojilj astojilj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@@ -755,6 +755,8 @@ class Painter {
// The default values for this state is meaningful and often expected.
// Leaving this state dirty could cause a lot of confusion for users.
this.context.cullFace.setDefault();
this.context.frontFace.setDefault();
this.context.cullFaceSide.setDefault();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good, this exhausts the missing one and I don't think it is necessary to add any other states to reset here. Just to confirm, the one not defaulted here are blending, clearColor, depthTest, depthMask, depthFunc, stencilTest, stencilMask, current program & fbo.

color modes are overridden here:

context.setColorMode(painter.colorModeForRenderPass());

stencil here:
context.setStencilMode(StencilMode.disabled);

depth here:
context.setDepthMode(depthMode);

And custom layer should override the program.

@karimnaaji karimnaaji merged commit 35b2652 into main Feb 25, 2021
@karimnaaji karimnaaji deleted the rreusser/reset-cull-state branch February 25, 2021 21:35
@karimnaaji karimnaaji added this to the v2.2 milestone Feb 25, 2021
@karta-solutions
Copy link

This, perhaps inadvertently, seems to have fixed an issue I was having where custom layer elements added through Three.js were simply disappearing along with a WebGL warning. Thanks for fixing that (even on accident)!

@rreusser
Copy link
Contributor Author

@karta-solutions Solving unexpected custom layer interactions was exactly the intent! 🎉 But I'm intrigued that it resulted in a warning rather than simply undesirable/unexpected state. Do you happen to have the warning handy? Though don't bother if you don't have it handy—that it's solved is the main thing 😄

@karta-solutions
Copy link

@rreusser Yes, the error in the console read "ERROR: GL_INVALID_OPERATION: glDrawElements: range out of bounds for buffer". Not very descriptive, not sure if that helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objects in custom layer flicker when using atmosphere sky layer
4 participants