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

Create unique MaskEffect per EffectManager #6595

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Conversation

felixpalmer
Copy link
Collaborator

For #6552

Background

When rendering into multiple WebGL contexts, the MaskEffect.maskMap texture created by the DEFAULT_MASK_EFFECT isn't valid in all contexts. This is an issue in Kepler when rendering in split screen mode. To repro in test/apps/mask.js add:

<div style={{position: 'relative', height: '450px'}}>
  <DeckGL
    layers={showLayers ? getLayers() : []}
    initialViewState={INITIAL_VIEW_STATE}
    controller={true}
  >
    <StaticMap reuseMaps mapStyle={mapStyle} preventStyleDiffing={true} />
  </DeckGL>
</div>
<div style={{position: 'relative', height: '450px'}}>
  <DeckGL
    layers={showLayers ? getLayers() : []}
    initialViewState={INITIAL_VIEW_STATE}
    controller={true}
  >
    <StaticMap reuseMaps mapStyle={mapStyle} preventStyleDiffing={true} />
  </DeckGL>
</div>

Change List

  • Create a new MaskEffect for every EffectManager instance

@coveralls
Copy link

coveralls commented Jan 28, 2022

Coverage Status

Coverage remained the same at 80.432% when pulling c8e5cb0 on felix/mask-multi-gl into 8291e86 on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

When rendering into multiple WebGL contexts.... This is an issue in Kepler when rendering in split screen mode.

In a perfect world, kepler.gl would obviously not be using two contexts but rather adopt the deck.gl View system.

Nit: I see a couple of lint warnings, at least some of them clearly introduced by the mask work, would you care to make a pass and squash them?

modules/core/src/lib/effect-manager.ts Show resolved Hide resolved
@felixpalmer
Copy link
Collaborator Author

In a perfect world, kepler.gl would obviously not be using two contexts but rather adopt the deck.gl View system.

There are other cases where we have two contexts, like when using multiple 3rd party basemaps, so this will always be an issue. @ibgreen I will take a look at the lint warnings

@felixpalmer felixpalmer merged commit e5e9557 into master Jan 31, 2022
@ibgreen
Copy link
Collaborator

ibgreen commented Jan 31, 2022

There are other cases where we have two contexts, like when using multiple 3rd party basemaps, so this will always be an issue.

Yes... if the user does wants deck.gl to use the WebGL contexts of multiple basemaps (as opposed to the "default" case of simply rendering deck.gl's views "on top of" multiple base map canvases), then multiple contexts are indeed needed.

It is in a way the most advanced use case for deck.gl, and it is great that you are working through the corner cases.

And yes, of course, we should absolutely support multiple contexts if we can. Was just pointing out that for performance and memory pressure reasons we have been investigating moving away from the dual context approach in kepler.gl, but sounds like you may still need the option? Though that is probably a discussion for the kepler.gl repo.

@felixpalmer
Copy link
Collaborator Author

Our use case is with kepler, but also with 2 basemaps - so we need 2 context support either way

@Pessimistress Pessimistress deleted the felix/mask-multi-gl branch February 3, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants