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

implement custom style layers #7039

Merged
merged 1 commit into from
Sep 6, 2018
Merged

implement custom style layers #7039

merged 1 commit into from
Sep 6, 2018

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jul 27, 2018

This implements #6456. It follows the approach taken in -native and keeps the scope fairly tight. It's an alternative to #6124.

Custom layers can be created by implementing CustomLayerInterface and can be added to the map with map.addLayer(customLayer). An example. Another example using ThreeJS.

The interface

type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>) => void;

type CustomLayerInterface = {
    id: string,
    type: "custom",
    renderingMode: "2d" | "3d",
    render: CustomRenderMethod, // for rendering directly into the texture
    prerender: ?CustomRenderMethod, // for rendering into a texture
    onAdd: ?(map: Map, gl: WebGLRenderingContext) => void,
    onRemove: ?(map: Map) => void
}

Custom layers trigger repaints with the newly exposed map.triggerRepaint().

The matrix

The matrix projects spherical mercator coordinates to gl coordinates. The spherical mercator coordinate [0, 0] represents the top left corner of the mercator world and [1, 1] represents the bottom right corner. The z coordinate is conformal. A box with identical x, y, and z lengths in mercator units would be rendered as a cube.

Shared depth

Custom 3D layers share depth space with our 3D layers. /debug/threejs.html contains an example of ThreeJS sharing depth space with fill extrusions:

screen shot 2018-07-27 at 12 51 43 pm

API design questions

Currently a layer implements render(...) if it wants to render a 2D layer and render3D(...) if it wants to render in 3D. Layers can implement both. Would it be better to only have render(...) and differentiate based on something else? For example, having two types: "type": "custom-3d" and "type": "custom"

When a custom layer takes over the gl context the state is in a mostly undefined state. Should we reset all (or a subset) of the state to default values?

Things left for later

  • an api for projecting to/from mercator and custom coordinate spaces
  • convenience methods for producing matrices for custom defined coordinate spaces
  • exposing support or helpers for tiled rendering

Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@davidmanzanares @ibgreen does this meet your basic needs?

@andrewharvey
Copy link
Collaborator

This is huge in what it will unlock! ❤️ Thanks @ansis

As someone writing a custom layer do I need to worry about if I'm drawing off screen or not? Is that my responsibility or does GL JS worry about that for me?

Surely there's performance considerations with your THREE example if I'm making boxes which are halfway across the world and not in the current view.

an api for projecting to/from mercator and custom coordinate spaces

I think the ultimate convenience is working with WGS84 longitude, latitude. For example from your https://github.com/mapbox/mapbox-gl-js/blob/custom-layers/debug/threejs.html example many use cases would be I have the long,lat coordinates and want to use THREE to draw something here.

@mourner
Copy link
Member

mourner commented Jul 30, 2018

@ansis the API and the implementation look really great! Exploring it in custom-layers-wind, having some troubles...

  • No layers draw after the wind layer. E.g. if I place it before building, no layers that come after building will be drawn. So the wind layer somehow corrupts the gl state I suppose.
  • I still need to do gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, false); in onAdd. I guess we should reset the context before onAdd too, and also expose a method to reset it, e.g. if the GL context is initialized asynchronously.
  • The map doesn't draw properly if I don't do gl.viewport(0, 0, gl.canvas.width, gl.canvas.height) at the end of the render call — I guess viewport isn't being properly reset at the beginning of GL rendering?
  • The particles now exhibit some weird striping artifacts that I haven't noticed before (especially noticeable when zooming to a certain zoom), and I have no idea what might be causing them and why we haven't seen them with the previous implementation.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

This is great!

I wonder how we can best include tests for this API. The render tests don't make sense, since custom layers can't be specified in a style document... maybe a (much simplified) analogue to those, but as a unit test using pixelmatch?

Would it be better to only have render(...) and differentiate based on something else? For example, having two types: "type": "custom-3d" and "type": "custom"

My gut feeling would be to only have render(), leave type: "custom", and differentiate with a separate property renderingMode: '2d' | '3d'. By the way: from the implementor's point of view, is the only difference that 2d must not use the depth buffer, while 3d can?

drawOffscreenTexture(painter, layer, 1);
}

} else if (painter.renderPass === 'offscreen' && layer.implementation.render3D) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for consistency with fill_extrusion (and because offscreen pass happens first), would it be clearer to swap the order of the translucent/offscreen cases here?


setBaseState() {
this.context.cullFace.set(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that this is the only property we have to set here -- why is that?

@@ -53,38 +55,47 @@ class StyleLayer extends Evented {
pixelsToTileUnits: number,
posMatrix: Float32Array) => boolean;

constructor(layer: LayerSpecification, properties: {layout?: Properties<*>, paint: Properties<*>}) {
+onAdd: (map: Map) => void;
+onRemove: (map: Map) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be ?(map: Map) => void; to express that they might be absent (i.e. so that Flow enforces the if (layer.onAdd), etc. checks we're doing in style.js)

this.sourceLayer = layer['source-layer'];
this.filter = layer.filter;
}
if (layer.type !== 'custom') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe invert this by doing if (layer.type === 'custom') return;?

render3D: CustomRenderMethod,
onAdd: (map: Map, gl: WebGLRenderingContext) => void,
onRemove(map: Map): void
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This type definition says that render and render3D are both defined. Should they both be typed ?CustomRenderMethod instead?

}
let layer;
if (layerObject.type === 'custom') {
layer = createStyleLayer(layerObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this isn't going through the style spec validation path, the provided implementation needs to be validated 1) for layer id uniqueness, and 2) conformance to the CustomLayerInterface. Probably 1) makes sense to do here, and maybe 2) in the CustomStyleLayer constructor?

@ryanhamley
Copy link
Contributor

I think the ultimate convenience is working with WGS84 longitude, latitude. For example from your https://github.com/mapbox/mapbox-gl-js/blob/custom-layers/debug/threejs.html example many use cases would be I have the long,lat coordinates and want to use THREE to draw something here.

Backing up @andrewharvey's point here with a specific example, this is exactly the kind of thing that would have been a game changer at my previous company. We had all these meshes and point clouds of real-world sites that we rendered with Three.js but we weren't able to geolocate them on a map so we simply rendered them in their own separate viewer. Being able to place them on a map would have given the users a ton of added value.

@markusjohnsson
Copy link
Contributor

This looks very promising!

It looks as if it will allow us to move our custom canvas-based layers to this API instead. I think we will need to keep some of our scene handling logic though, like rendering two world widths to allow graphics crossing the +/-180 meridian. It would be nice if that could be handled by mapbox in the future, though I don't know how it would work.

@markusjohnsson
Copy link
Contributor

markusjohnsson commented Aug 3, 2018

I tested this branch to feel it out, but I got problems with label layers:

image

It only happened after I added a texture (using THREE.TextureLoader) and subsequently used it in a shader (THREE.ShaderMaterial). Prior to using textures it worked fine.

Gist with example source:
https://gist.github.com/markusjohnsson/b6a2d8750cd15f593010c4dfa68ce2e3

@andrewharvey
Copy link
Collaborator

andrewharvey commented Aug 6, 2018

Backing up @andrewharvey's point here with a specific example, this is exactly the kind of thing that would have been a game changer at my previous company. We had all these meshes and point clouds of real-world sites that we rendered with Three.js but we weren't able to geolocate them on a map so we simply rendered them in their own separate viewer. Being able to place them on a map would have given the users a ton of added value.

This function works for me to transform from WGS84 long, lat into a unit vector, anchored at the top left, as needed by this PR.

 var fromLL = function (lon,lat) {
     // derived from https://gist.github.com/springmeyer/871897
     var extent = 20037508.34;
 
     var x = lon * extent / 180;
     var y = Math.log(Math.tan((90 + lat) * Math.PI / 360)) / (Math.PI / 180);
     y = y * extent / 180;
 
     return [(x + extent) / (2 * extent), 1 - ((y + extent) / (2 * extent))];
 }

Providing a helper function for this would make it much easier for people to do #3996 (custom 3d models).

Here's an example built on top of @ansis's code, but placing a model with long, lat coords.

https://bl.ocks.org/andrewharvey/7b61e9bdb4165e8832b7495c2a4f17f7

selection_999 017

@davidmanzanares
Copy link

Hi,

First of all, thank you so much for pushing this, this is an amazing feature.

Regarding your question, we've only made one small PoC of integrating this with CARTO VL, but it looked quite good. We found two small issues regarding WebGL state sharing( #7080 and #7081) but apart from that, we haven't seen any other rendering issue.

However, we do have seen a performance regression compared to our previous approach. After some profiling, I saw that the actor.js:58 receive() function took much more time when using this integration: from a total time of 9% to a total time of 19%. We also saw an important increase in execution time in web_worker_transfer.js:197 deserialize(). To be honest, I don't know if this makes any sense, I don't see any WebGL related calls in those functions. I could be seeing another performance regression not related to this branch since our integration is not aligned with master. If you think this is the case, we could update our integration with master and see if the regression is there.

We also have thought of another way to share the WebGL state in a safe way without changing this new API. I think that monkey-patching the WebGL context to provide a patched version to custom layers could be a possible way of knowing when you need to mark some state as dirty. This will remove the need of calling Context.setDirty in every frame and state would be marked as dirty as needed. The number of WebGL drawing calls could be decreased that way, but I don't know if the performance advantage is worthwhile.

Thanks!

@mourner
Copy link
Member

mourner commented Aug 6, 2018

@davidmanzanares yes, the performance regression you're seeing is likely in master too — see #7011 for more context. There's also #7015 as an attempt to win some perf back, but we're not sure it's the right approach yet.

@andrewharvey
Copy link
Collaborator

I ran into an issue where a THREE.js scene wasn't showing up with an empty map style:

style: {
    version: 8,
    sources: {},
    layers: []
}

I needed to add a background layer first. It might have been a problem on my end, but I'm not sure.

@TripleMap
Copy link

@andrewharvey,

Your example inspired me to use models instead of circles. But the question arises. I can not find the input point in src mapbox, to work with the current source. For example, I have about 20 point (lat, lng, height). How can I get data from source? (I belive, that it is possible).

@ansis
Copy link
Contributor Author

ansis commented Aug 13, 2018

I've pushed a bunch of fixes including some api changes:

  • therender3D method was removed and a "renderingMode": "2d" | "3d" property was added
  • an optional prerender method was added so that users can separate rendering into textures from rendering into the main framebuffer. This was added to work around cases where the user changes the blend and depth settings when drawing into the texture and would then need to reset these settings when drawing onto the map

Two render tests were added.

I think the ultimate convenience is working with WGS84 longitude, latitude. For example from your https://github.com/mapbox/mapbox-gl-js/blob/custom-layers/debug/threejs.html example many use cases would be I have the long,lat coordinates and want to use THREE to draw something here.

Yep! I think we should add this but outside of this pr so that we can get the api right for that.

I think we will need to keep some of our scene handling logic though, like rendering two world widths to allow graphics crossing the +/-180 meridian. It would be nice if that could be handled by mapbox in the future, though I don't know how it would work.

Yeah, this is tricky to implement but it's outside the scope of the current custom layers work. I'm not sure how this would work on the mapbox-gl side.

I tested this branch to feel it out, but I got problems with label layers:
@markusjohnsson thanks for the test case. It should be fixed!

We also have thought of another way to share the WebGL state in a safe way without changing this new API. I think that monkey-patching the WebGL context to provide a patched version to custom layers could be a possible way of knowing when you need to mark some state as dirty.

@davidmanzanares yep, that could be a good approach! If the setDirty() approach turns out to be a performance or safety problem we should try this.

I ran into an issue where a THREE.js scene wasn't showing up with an empty map style:

@andrewharvey Thanks for catching this. It should now be fixed!

@Jesus89
Copy link
Contributor

Jesus89 commented Aug 23, 2018

Hi @ansis,

I have tested the integration of this branch with CARTO VL, adding the new interface (attributes and functions) to our carto.Layer object to match the API.

I'm a bit worried about exposing these attributes and functions in the Layer because a user can be confused using them as part of the public API.

Could you add something to differentiate this internal API from the public API, for example, adding '$' at the beginning?

type CustomLayerInterface = {
    $id: string,
    $type: "custom",
    $render: CustomRenderMethod,
    $render3D: CustomRenderMethod,
    $onAdd: (map: Map, gl: WebGLRenderingContext) => void,
    $onRemove(map: Map): void
}

Thanks for your work!

Regards.

@ansis
Copy link
Contributor Author

ansis commented Aug 23, 2018

I'm a bit worried about exposing these attributes and functions in the Layer because a user can be confused using them as part of the public API.

Could you add something to differentiate this internal API from the public API, for example, adding '$' at the beginning?

@Jesus89 from the mapbox-gl-js perspective this will be a public API. I'm not sure if I'm understanding your issue correctly, but if adding these members to carto.Layer is an issue, could you implement them on a separate object and then add that to the map, instead of the carto.Layer itself?

Also: thanks for testing it out!

@markusjohnsson
Copy link
Contributor

markusjohnsson commented Aug 23, 2018

@ansis I think that what @Jesus89 refers to is that if you get a reference to the layer you would be able to make a call to one of these methods e.g. map.getLayer().render(), in that sense, the methods should not be public (but rather protected, in the TypeScript/C#/Java sense).

Edit: not that I really agree.. I think the API as it is is very nice to use.

src/geo/transform.js Show resolved Hide resolved
x + d, y + d, z,
x, y + d + d, 0,
x + d, y + d + d, 0]);
const indexArray = new window.Uint16Array([
Copy link
Member

Choose a reason for hiding this comment

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

Nit: why do we access those with window? If linter complains, this probably needs a fix on the lint rules side.

src/render/draw_custom.js Show resolved Hide resolved
src/render/draw_custom.js Show resolved Hide resolved
src/render/draw_custom.js Show resolved Hide resolved
layers: this._order.map((id) => this._layers[id].serialize())
layers: this._order.map((id) => this._layers[id])
.filter((layer) => layer.type !== 'custom')
.map((layer) => layer.serialize())
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated with _serializeLayers. Let's DRY up. Maybe also write with a for loop / push for less short-lived allocations?

@sakeenaq
Copy link

sakeenaq commented Aug 30, 2018

@markusjohnsson wrt #7039 (comment) I was facing the same issue with the label layers, did you come across any resolution for it?

@markusjohnsson
Copy link
Contributor

@SakeenaQureshee no, I just tested with the latest changes, and I still have the same problem (at least at certain zoom levels).

@andrewharvey
Copy link
Collaborator

@SakeenaQureshee no, I just tested with the latest changes, and I still have the same problem (at least at certain zoom levels).

I think it affects other textures too including raster sources.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

This looks good to me, pending a green rebase on master. We can follow-up on improvements with separate PRs.

@ansis
Copy link
Contributor Author

ansis commented Sep 6, 2018

@Jesus89 thanks for the feedback but we've decided to leave the interface as is

@measuredweighed the current implementation should be pretty stable. The first production release it will be included in will be released October 10th.

@Jesus89
Copy link
Contributor

Jesus89 commented Sep 6, 2018

OK. Thanks for your work! 👍

@tibotiber
Copy link

tibotiber commented Sep 26, 2018

This feature is huge, thanks for the amazing work. A quick question: if I wanted to add a lot of independent objects (e.g. threejs 3D models), each with separate locations, would adding a custom layer (with its own scene, camera and renderer) for each object pose any performance issue? Or would you recommend I create a single custom layer in which I add the different objects computing location based on camera matrix and intended position?

In the second case, I'd imagine the location computation will be a bit more complex. Any pointer on this? Probably something like gl_Position = projectionMatrix * viewMatrix * modelMatrix * vec4( position, 1.0 ); as seen in mrdoob/three.js#1188 (comment) but not sure where I'd get viewMatrix (camera.matrixWorldInverse) from?

Cc @pviejo

@tibotiber
Copy link

tibotiber commented Sep 27, 2018

Update on my previous comment. I have gone the second way and implemented one single custom layer centered in (lat,lng)=(0,0) in which I add multiple objects for which the position is computed using the fromLL method shared by @andrewharvey. Works really well and I believe more efficient than having a separate renderer for each object. Thanks again for the great work 👍.

@tibotiber
Copy link

Re-update FWIW. threejs would have rendering artifacts on large scenes, so it's better locating the custom layer scene near your area of interest and offsetting objects in the scene accordingly.

@wandergis
Copy link
Contributor

Update on my previous comment. I have gone the second way and implemented one single custom layer centered in (lat,lng)=(0,0) in which I add multiple objects for which the position is computed using the fromLL method shared by @andrewharvey. Works really well and I believe more efficient than having a separate renderer for each object. Thanks again for the great work 👍.

Does Raycaster works also works well ?

@tibotiber
Copy link

@wandergis no, completely out of sync 😬

@rveciana
Copy link

Hi, does somebody know how to calculate the positions? The initial comment says:

The spherical mercator coordinate [0, 0] represents the top left corner of the mercator world and [1, 1] represents the bottom right corner.

Taking the Three.js example, for instance:

  • The horizontal coordinate is easy: (lon +180)/360. In the example, would be (-79.390307 + 180) / 360 = 0.279471369, which is the position in the code
  • The vertical position doesn't seem to work the same way. It should be something like (90 - lat)/180, but applying it to the example, 0.257450244, very far away from the correct 0.364935
  • I know that the upper limit seems 85 degrees, not 90, but changing the values doesn't give a good result
  • 0.5 is the equator, as expected, but 45 degrees is 0.33, instead of the expected 0.25. Does the position depends on the cos(lat)? I don't find the formula neither...

@andrewharvey
Copy link
Collaborator

@rveciana See the code snippet in my comment at #7039 (comment)

@lileialg
Copy link

For the first,this feature is very great,I can upload my shader to mapbox-gl,So I can display unlimited visualization; but I have a question, when I increase the zoom to 17,18,19,20,21,22....., the position of point is not stable, the position of points are shaking with the zoom increasing or map rotating, the attachment is my code file,I am looking for your response, thanks very much
custom3d-4.txt

@measuredweighed
Copy link

@lileialg This is related to a lack of floating point precision. I opened an issue about this (#7268) and also provided one possible workaround.

@lileialg
Copy link

@lileialg This is related to a lack of floating point precision. I opened an issue about this (#7268) and also provided one possible workaround.
Thank you very much, that's really help

@lileialg
Copy link

lileialg commented Nov 1, 2018

@ansis @measuredweighed I covert position to pixel in javascript instead of in shader script,It looks like working fine.
custom3d-13.txt

@bhison
Copy link

bhison commented Jul 26, 2019

Re-update FWIW. threejs would have rendering artifacts on large scenes, so it's better locating the custom layer scene near your area of interest and offsetting objects in the scene accordingly.

@tibotiber Thanks for your above comments, they've really set me on the right path for rendering multiple 3D objects across my map. I've started from the Add a 3D model example, set the center of my map as the translation for the Three projection matrix and then applied an offset transform on the object to have it positioned correctly. Fairly standard stuff

My model is now in the correct position, with the right scale, a slightly off rotation (which I assume is the symptom of floating point issues), but the major issue is for some reason it now looks like the normals are inverted on my model. I was wondering if you came up against anything like this? I've tried fiddling with a lot of options but I'm perplexed, I can't see how this could be happening.

@tibotiber
Copy link

@bhison glad the comments could help :). And no I haven't met any issue concerning normals. Sorry, can't be of much help on this.

@bhison
Copy link

bhison commented Jul 29, 2019

@bhison glad the comments could help :). And no I haven't met any issue concerning normals. Sorry, can't be of much help on this.

Knowing you haven't had the same issue is kind of helpful from a process of elimination perspective. Much appreciated.

@mourner mourner deleted the custom-layers branch August 1, 2019 11:47
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.