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

Custom WebGL layer #6124

Closed
wants to merge 26 commits into from
Closed

Custom WebGL layer #6124

wants to merge 26 commits into from

Conversation

davidmanzanares
Copy link

This PR adds a new layer type 'custom-webgl' as requested in #281

With this new layer type the WebGL context is exposed to external renderers to allow for custom additional functionality.

This new feature is useful because:

  • It allows to put third party renderers between native MapboxGL layers
  • It removes the performance cost of using two different canvas one in top of the other. We've seen a 2x performance improvement.

The approach taken creates a new Map method setCustomWebGLDrawCallback to bind rendering callback functions to custom-webgl layers:

map.setCustomWebGLDrawCallback('externallayer0', (gl, invalidate) => {
   gl.clearColor(0, 0.5, 0, 1);
   gl.clear(gl.COLOR_BUFFER_BIT);
});

map.on('load', function () {
   map.addLayer({
       "id": "externallayer0",
       "type": "custom-webgl"
   }, "water");
});

The callback function receives the WebGL context and a WebGLStateTrackerInvalidation function that can be used by the external renderer to call WebGL functions asynchronously in a safe way.

External renderers should call invalidateCurrentWebGLState when they modify the WebGL context state asynchronously.

For example, if the external renderer receives some data from an AJAX request,
it can upload data to the WebGL context asynchronously (regarding Mapbox GL rendering,
it would synchronous respect WebGL) and call invalidateCurrentWebGLState afterwards
to ensure that in the next Mapbox GL rendering pass the WebGL tracked is not stale (which would produce rendering artifacts).

Thank you!

Note: this PR includes the bugfix of #6123

@mourner
Copy link
Member

mourner commented Feb 9, 2018

Hey, this is great — thank you so much for taking on this! The approach looks very promising. Let us think this through and follow up next week — this is an API where we want to be very careful because incompatibly breaking it later could be painful for users, and increase support efforts significantly.

@davidmanzanares
Copy link
Author

Hi! Awesome! Yes, we share your concerns regarding compatibility, and yes, this PR needs more work to ensure that, but we would like to discuss it a little bit more before that.

To avoid breaking compatibility in future Mapbox GL versions, particularly without knowing that some change is a breaking change we need to establish some safety mechanism since the current approach works, but it is fragile regarding changes in Mapbox GL.

We've considered some automatic-programmatic solution to the problem of sharing the WebGL context, but, to be honest, we haven't found a good performant way of doing that.

However, I think that it would be ok if we tackled this issue from a documentation API point of view. Basically, my idea is that if Mapbox GL declared how all WebGL status can be shared, and if Mapbox GL keeps that promise, then it would be up to the external renderer comply with Mapbox GL requirements.

To be more specific, I would divide the WebGL state into two parts: state tracked by the Mapbox GL context tracker (we should enumerate all of it) and state not tracked.

External renderers should be able to modify the first state with just one condition, if they modify it asynchronously then they should call the invalidate callback.

External renderers should be able to assume that the second state is set to the WebGL default values and if they modify it, then they should set it to the original value afterward.

With these two rules, it should be safe to share the context and future Mapbox GL versions shouldn't break renderers who originally targeted previous Mapbox GL versions.

Of course, this is not the easiest API, but we think that it is easy enough for the target of this feature: programmers with at least a medium understanding of WebGL.

In any case, we really want to see this happen and we'd love to hear about your point of views.

@rochoa
Copy link

rochoa commented Feb 26, 2018

Hey, @mourner,

Did you have the opportunity to discuss this one? I'm pretty interested in seeing this happening, how could I help?

Thanks.

@mourner
Copy link
Member

mourner commented Feb 26, 2018

@rochoa hey, sorry for not getting back to you — there's been a lot on the team's plate for the last
two weeks, but we're now scheduling an internal talk mid-week to discuss custom WebGL layers API and this PR in particular. Let me follow up with how this goes.

@mourner
Copy link
Member

mourner commented Mar 15, 2018

Hey, sorry again for the delay — we did indeed have a team call about custom layers and this PR two weeks ago, and decided that we'd love to move this forward in general. Let me describe our specific concerns / open questions in a new ticket.

@areknawo
Copy link

So, is it indeed moving forward? I'd love to see this functionality added.

@mourner mourner mentioned this pull request Apr 5, 2018
@mourner
Copy link
Member

mourner commented Apr 5, 2018

Again, apologies for the delay. Here's a summary of our current thinking on the API: #6456

@ryanbaumann
Copy link
Contributor

An alternative implementation of custom layers by @ansis -> #7039

@mourner
Copy link
Member

mourner commented Aug 9, 2018

Closing in favor of #7039

@mourner mourner closed this Aug 9, 2018
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.

6 participants