-
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
Custom layer immediate and draped rendering on globe and terrain #12182
Conversation
Hey @akoylasar , could you give us a small update on when/if this is going to be merged? I would be thankful 🙏 |
This feature looks great! May I ask you whether it is possible to load a 3d model around the earth (e.g. ISS station) by setting up the coordinate and the altitude? |
This is awesome, cant wait to see this merged |
Hi @sienki-jenki for now there aren't any plans to merge this but if that changes we will keep you updated. |
yes it should be possible. |
Hi @akoylasar, Is this going to allow upgrading Mapbox integration with deck.gl to support globe, or is there more to that? |
38c4621
to
f1e47f7
Compare
noticed an issue where for some of the tiles the custom layer seems to not get draped into the corresponding offscreen drape texture: initially I thought that for the said tiles the UPDATE: Some debugging revealed that for the "invisible" tiles above when |
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.
@akoylasar a first pass at reviewing this, but it's looking great already
debug/satellites-custom-layer.js
Outdated
this.updateBuffers(); | ||
|
||
gl.enable(gl.DEPTH_TEST); | ||
if (globeMatrix !== null) { // globe projection |
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.
For discussion: I wonder if there's a better way on the API of custom layer to split between projections. I could imagine two more options:
- add a specific value to express which projection we're in (e.g.
render(gl, projection, matrices, ...)
- have another call for this projection so related code is contained within, e.g.
renderGlobe
I find it a bit counter-intuitive to rely on one of the input parameter not being set, but also a bit fragile, if we internally change the interface of this function to not return null
,
mapbox-gl-js/src/geo/transform.js
Line 1652 in ad9c713
globeToMercatorMatrix(): ?Array<number> { |
If you still prefer that design, I'd only suggest to ensure that function always return null
under these circumstances with some small tests covering that.
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.
@karimnaaji that's a very good point! There was an implicit agreement with @astojilj that we could always get back to these (i.e. immediate mode related APIs) later as they're experimental anyways. I think we could go with the approach you suggested.
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.
renderGlobe
seems better. There is a need to pass additional information, globe matrix, mercatorMatrix (projection matrix passed during globe projection has different z scale from mercator projection matrix used in mercator projection, due to pixelsPerMeter usage in worldToCamera).
mapbox-gl-js/src/geo/transform.js
Lines 1849 to 1875 in 55eab7c
const zUnit = this.projection.zAxisUnit === "meters" ? pixelsPerMeter : 1.0; | |
const worldToCamera = this._camera.getWorldToCamera(this.worldSize, zUnit); | |
const cameraToClip = this._camera.getCameraToClipPerspective(this._fov, this.width / this.height, this._nearZ, this._farZ); | |
// Apply center of perspective offset | |
cameraToClip[8] = -offset.x * 2 / this.width; | |
cameraToClip[9] = offset.y * 2 / this.height; | |
let m = mat4.mul([], cameraToClip, worldToCamera); | |
if (this.projection.isReprojectedInTileSpace) { | |
// Projections undistort as you zoom in (shear, scale, rotate). | |
// Apply the undistortion around the center of the map. | |
const mc = this.locationCoordinate(this.center); | |
const adjustments = mat4.identity([]); | |
mat4.translate(adjustments, adjustments, [mc.x * this.worldSize, mc.y * this.worldSize, 0]); | |
mat4.multiply(adjustments, adjustments, getProjectionAdjustments(this)); | |
mat4.translate(adjustments, adjustments, [-mc.x * this.worldSize, -mc.y * this.worldSize, 0]); | |
mat4.multiply(m, m, adjustments); | |
this.inverseAdjustmentMatrix = getProjectionAdjustmentInverted(this); | |
} else { | |
this.inverseAdjustmentMatrix = [1, 0, 0, 1]; | |
} | |
// The mercatorMatrix can be used to transform points from mercator coordinates | |
// ([0, 0] nw, [1, 1] se) to GL coordinates. | |
this.mercatorMatrix = mat4.scale([], m, [this.worldSize, this.worldSize, this.worldSize / pixelsPerMeter, 1.0]); |
Edit: prerender
needs the same and prerenderGlobe doesn't sound correct. Refactoring to support other projections and updating mercator and globeToMercator matrix to handle difference in z scaling.
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.
LGTM % CI
src/render/draw_custom.js
Outdated
if (painter.transform.projection.unsupportedLayers && painter.transform.projection.unsupportedLayers.includes("custom")) { | ||
if (painter.transform.projection.unsupportedLayers && painter.transform.projection.unsupportedLayers.includes("custom") && | ||
!(painter.terrain && (painter.terrain.renderingToTexture || painter.renderPass === 'offscreen') && layer.isLayerDraped())) { | ||
warnOnce('Custom layers are not yet supported with non-mercator projections. Use mercator to enable custom layers.'); | ||
return; | ||
} |
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.
Should this error message be updated to note that globe support? I also think the second condition should only early-return, not output an error message:
if (painter.transform.projection.unsupportedLayers && painter.transform.projection.unsupportedLayers.includes("custom")) { | |
if (painter.transform.projection.unsupportedLayers && painter.transform.projection.unsupportedLayers.includes("custom") && | |
!(painter.terrain && (painter.terrain.renderingToTexture || painter.renderPass === 'offscreen') && layer.isLayerDraped())) { | |
warnOnce('Custom layers are not yet supported with non-mercator projections. Use mercator to enable custom layers.'); | |
return; | |
} | |
if (painter.terrain && !painter.terrain.renderingToTexture && painter.renderPass !== 'offscreen' && layer.isLayerDraped()) { | |
// When terrain is enabled, don't process custom draped layers unless we're in the offscreen pass | |
return; | |
} | |
if (painter.transform.projection.unsupportedLayers && painter.transform.projection.unsupportedLayers.includes("custom")) { | |
warnOnce('Custom layers are not yet supported with this projection. Use mercator or globe to enable usage of custom layers.'); | |
return; | |
} |
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.
Thanks. Updated comment. Custom layer draping follows the order of other layers and draping happens within the translucent pass.
Transition mercator to globe and back in the demo: Untitled.mov |
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.
One suggestion up for discussion, but nothing blocking. LGTM
|
||
type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>) => void; | ||
type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>, projection?: ProjectionSpecification, projectionToMercatorMatrix?: Array<number>, projectionToMercatorTransition?: number) => void; |
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.
Following a bit the conversation from #12182 (comment), and since we're adding a few new parameters in that interface, it might make sense to wrap it under a single variable projectionParams
so changes don't break client-side code if we need to remove, add or re-order new parameters to that interface. It would become:
type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>, projectionParams: ProjectionParams) => void;
export type ProjectionParams = {
projection?: ProjectionSpecification,
projectionToMercatorMatrix?: Array<number>,
projectionToMercatorTransition?: number
};
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.
this can be implemented in a follow-up PR along with the fix for #12182 (comment)
@@ -2,6 +2,7 @@ | |||
|
|||
import {wrap} from '../util/util.js'; | |||
import LngLatBounds from './lng_lat_bounds.js'; | |||
import {GLOBE_RADIUS, globeMetersToEcef, latLngToECEF} from '../geo/projection/globe_util.js'; |
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.
Unit test fail on circular dependency here globe_utils -> mercator_coordinate -> lng_lat -> globe_utils.
there is one issue where a discontinuity occurs during the transition from globe to mercator as seen in the video below: discontin.movthis issue is particularly noticeable closer to the poles. However to speed things up the fix for this issue will be delivered in a follow-up PR. cc @astojilj @karimnaaji |
CustomStyleLayer API extended to support draping with new methods: export type CustomLayerInterface = { ... // misusing MercatorCoordiate's xyz for tile id (x, y, z) where wrap is baked into x. renderToTile: ?(gl: WebGLRenderingContext, tileId: MercatorCoordinate) => void, // return true only for frame when content has changed - otherwise, all the terrain // render cache would be invalidated and redrawn causing huge drop in performance. shouldRerenderTiles: ?() => boolean, } Using WebGL Wind demo code, for simplicity of use, instead of adding submodule, copied dist code. Full code is available at https://github.com/mapbox/webgl-wind/tree/astojilj-draping-support
In globe projection, mercator matrix z scale was incorrect, disabling possibility for transition. Transform.mercatorMatrix changed in order to produce the expected mercator matrix, even in globe (zoom 5->6) transition, and then the same compensated in globe to mercator matrix. updated satellites_custom_layer.js to show transition and satellites in mercator projection.
93868a1
to
0979259
Compare
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.
Sorry for a late review, just a few nits we can address later.
const ecef = latLngToECEF(this.lat, this.lng, radius); | ||
return [ecef[0], ecef[1], ecef[2]]; |
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.
Nit: latLngToECEF
already returns a new 3-item array, there seems to be no purpose in making another one.
@@ -68,6 +67,10 @@ const GLOBE_LOW_ZOOM_TILE_AABBS = [ | |||
new Aabb([0, 0, GLOBE_MIN], [GLOBE_MAX, GLOBE_MAX, GLOBE_MAX]) // x=1, y=1 | |||
]; | |||
|
|||
export function globeMetersToEcef(d: number): number { | |||
return d * mercatorZfromAltitude(1, 0.0) * 2.0 * GLOBE_RADIUS * Math.PI; |
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.
Nit: we could simplify this. mercatorZfromAltitude(1, 0) = 1 / earthCircumference = 1 / 2 * Math.PI * earthRadius
, so we would have:
return d * GLOBE_RADIUS / earthRadius;
if (painter.transform.projection.name === "globe") { | ||
prerender.call(implementation, context.gl, painter.transform.customLayerMatrix(), painter.transform.getProjection(), painter.transform.globeToMercatorMatrix(), globeToMercatorTransition(painter.transform.zoom)); | ||
} else { | ||
prerender.call(implementation, context.gl, painter.transform.customLayerMatrix()); |
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.
If I'm reading this right, for non-globe projections, this line will be called twice... A mistake probably?
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.
Yes, this one would be a regression. We need to address this before the release.
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.
Sorry for a late review, just a few nits we can address later.
I will create a bugfix PR for this immediately.
I have attempted to create a custom layer that uses a PNG image as a data source, but this layer does not drop on the terrain. Is there any optimization available for such custom layers in this project? |
This PR consolidates the changes from #12143 and #11996 to showcase both immediate and draped rendering modes on globe and proposes new necessary APIs.
Note that it might also be necessary to expose an API to allow clients to adjust near and far distances for the camera.
custom-layer-globe.mov
Fixes #11177. This PR requires API required to implement animated wind demo presented in #11996 (comment), but it doesn't include wind demo files.
TODO:
Launch Checklist
- [ ] document any changes to public APIsExperimental API.- [ ] post benchmark scoresN/A- [ ] tagged@mapbox/map-design-team
@mapbox/static-apis
if this PR includes style spec API or visual changes- [ ] add an entry inside this element for inclusion in themapbox-gl-js
changelog:<changelog></changelog>