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 layer immediate and draped rendering on globe and terrain #12182

Merged
merged 25 commits into from
Jan 24, 2023

Conversation

akoylasar
Copy link
Contributor

@akoylasar akoylasar commented Aug 23, 2022

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:

  • Fix globe to mercator transition in custom-layer-globe.html

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 Experimental API.
    - [ ] post benchmark scores N/A
  • manually test the debug page
    - [ ] tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • 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></changelog>

@mourner mourner mentioned this pull request Aug 31, 2022
@sienki-jenki
Copy link

Hey @akoylasar , could you give us a small update on when/if this is going to be merged? I would be thankful 🙏

@beifeizhou
Copy link

beifeizhou commented Dec 2, 2022

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?

@mpiannucci
Copy link

This is awesome, cant wait to see this merged

@akoylasar
Copy link
Contributor Author

Hey @akoylasar , could you give us a small update on when/if this is going to be merged? I would be thankful 🙏

Hi @sienki-jenki for now there aren't any plans to merge this but if that changes we will keep you updated.

@akoylasar
Copy link
Contributor Author

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?

yes it should be possible.

@zakjan
Copy link

zakjan commented Jan 5, 2023

Hi @akoylasar, Is this going to allow upgrading Mapbox integration with deck.gl to support globe, or is there more to that?

@akoylasar akoylasar force-pushed the fouad/custom-layer-immediate-and-draped branch from 38c4621 to f1e47f7 Compare January 16, 2023 09:58
@akoylasar akoylasar added the skip changelog Used for PRs that do not need a changelog entry label Jan 16, 2023
@akoylasar akoylasar marked this pull request as ready for review January 17, 2023 13:01
@akoylasar akoylasar requested a review from a team as a code owner January 17, 2023 13:01
@akoylasar akoylasar changed the title Custom layer immediate and draped on globe Custom layer immediate and draped rendering on globe Jan 17, 2023
@akoylasar akoylasar added feature 🍏 and removed skip changelog Used for PRs that do not need a changelog entry labels Jan 17, 2023
@akoylasar
Copy link
Contributor Author

akoylasar commented Jan 17, 2023

noticed an issue where for some of the tiles the custom layer seems to not get draped into the corresponding offscreen drape texture:

Screenshot 2023-01-17 at 17 22 33

initially I thought that for the said tiles the renderToTile callback might have been left out uninvoked. But as seen from the screenshot above this wasn't the case. Further investigating this.

UPDATE: Some debugging revealed that for the "invisible" tiles above when renderToTiles is invoked stencil test is enabled and is failing causing the fragments rendered by client code to be dropped. It's still not clear why for some of the tiles stencil test is enabled while for other it's not. Further investigating.

Copy link
Contributor

@karimnaaji karimnaaji left a 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

src/geo/lng_lat.js Outdated Show resolved Hide resolved
src/geo/lng_lat.js Outdated Show resolved Hide resolved
src/geo/lng_lat.js Outdated Show resolved Hide resolved
src/geo/lng_lat.js Outdated Show resolved Hide resolved
this.updateBuffers();

gl.enable(gl.DEPTH_TEST);
if (globeMatrix !== null) { // globe projection
Copy link
Contributor

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,

globeToMercatorMatrix(): ?Array<number> {
we would break such an API and also client-side code.

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@astojilj astojilj Jan 23, 2023

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).

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.

src/geo/lng_lat.js Show resolved Hide resolved
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

LGTM % CI

Comment on lines 18 to 26
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;
}
Copy link
Contributor

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:

Suggested change
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;
}

Copy link
Contributor

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.

@astojilj astojilj changed the title Custom layer immediate and draped rendering on globe Custom layer immediate and draped rendering on globe and terrain Jan 20, 2023
@astojilj
Copy link
Contributor

Transition mercator to globe and back in the demo:

Untitled.mov

Copy link
Contributor

@karimnaaji karimnaaji left a 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;
Copy link
Contributor

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
};

Copy link
Contributor Author

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)

@astojilj astojilj added the skip changelog Used for PRs that do not need a changelog entry label Jan 23, 2023
@@ -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';
Copy link
Contributor

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.

@akoylasar
Copy link
Contributor Author

there is one issue where a discontinuity occurs during the transition from globe to mercator as seen in the video below:

discontin.mov

this 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

astojilj and others added 5 commits January 24, 2023 11:15
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
@akoylasar akoylasar force-pushed the fouad/custom-layer-immediate-and-draped branch from 93868a1 to 0979259 Compare January 24, 2023 10:15
@akoylasar akoylasar merged commit 5aed4f6 into main Jan 24, 2023
@akoylasar akoylasar deleted the fouad/custom-layer-immediate-and-draped branch January 24, 2023 10:44
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.

Sorry for a late review, just a few nits we can address later.

Comment on lines +133 to +134
const ecef = latLngToECEF(this.lat, this.lng, radius);
return [ecef[0], ecef[1], ecef[2]];
Copy link
Member

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;
Copy link
Member

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());
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Xin-jj
Copy link

Xin-jj commented Jun 15, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🍏 skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add terrain rendering support for customlayer
9 participants