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

Simplify globe custom layer interface #12545

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

karimnaaji
Copy link
Contributor

@karimnaaji karimnaaji commented Jan 27, 2023

Simplifications suggested in #12544 (comment) to hide the complexity/internal details from the user and simplify interface.

  • Remove internal shader boilerplate code as it requires quite a bit of understanding of the internal workings of mapbox-gl. The new API customLayerVertexHeader is now used to include as shader prelude for boilerplate code. For globe custom layers, users only have to about the following:
    • creating ECEF coordinates for globe
    • creating mercator coordinates for mercator
    • projecting each of these without thinking about internal details with project_custom_layer
  • Remove the need for split shader in user implementation, implementation can now be used in a single shader unit
  • Remove now unnecessary interface parameters:
- type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>, projection: ?ProjectionSpecification, projectionToMercatorMatrix: ?Array<number>, projectionToMercatorTransition: ?number, centerInMercator: ?Array<number>, pixelsPerMeterRatio: ?number) => void;
+ type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>) => void;
  • Add new requirement from user to implement getShaderProgram, allowing us to upload necessary uniforms
  • Encode center transformation used during the globe-mercator transition on a matrix (moving some calculations to CPU instead of GPU)

if (painter.transform.projection.name === "globe") {
implementation.render(context.gl, painter.transform.customLayerMatrix(), painter.transform.getProjection(), painter.transform.globeToMercatorMatrix(), globeToMercatorTransition(painter.transform.zoom));
if (shaderProgram) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're ok with the approach, I'll clean up this section for better error reporting as well as safeguard (ensuring that the user properly injected user code snippet helper).

void main() {
gl_PointSize = 30.;
gl_Position = u_projection * vec4(a_pos_merc, 1.);
gl_Position = project_custom_layer(a_pos_merc, a_pos_ecef);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could simplify this one step further on the globe custom layer, since it's now consolidated in the same shader program the user may not need to provide two buffers (ecef + merc) and we could ask for them to only provide the merc while doing the projection to ecef for them in project_custom_layer(a_pos_merc).

It would mean moving some computations from CPU to GPU but I think that's an ok tradeoff for the sake of simplified use. This also has the advantage that translating custom layer code to the new system becomes fairly trivial.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering though if this approach won't limit users. In the debug example where only a few points are rendered what's proposed above works quite ok. But what if a client decides to use a heavy model (e.g. a satellite model). In that case all the mercator points need to be transformed into ECEF and this could be quite expensive since the operation is not a linear one that could be done like before using a standard transformation matrix. What are your thoughts on 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.

Discussed offline: let's hold on this suggestion, e.g. transform ECEF on the GPU on our side for performance concern. We'll still keep API conservative and simple as in this PR and introduce other hooks as needed. The idea being that we don't over-commit by giving too much through the API, since it's always easy to give more later than give too much now and introduce breaking changes by removing it.

const EARTH_CIRCUMFERENCE_METERS = 2 * Math.PI * EARTH_RADIUS_METERS;
const GLOBE_CIRCUMFERENCE_ECEF = 8192;
const METERS_TO_ECEF = GLOBE_CIRCUMFERENCE_ECEF / EARTH_CIRCUMFERENCE_METERS;

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for removing this as well it was leftovers which I had forgotten to clean up.

void main() {
gl_PointSize = 30.;
gl_Position = u_projection * vec4(a_pos_merc, 1.);
gl_Position = project_custom_layer(a_pos_merc, a_pos_ecef);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering though if this approach won't limit users. In the debug example where only a few points are rendered what's proposed above works quite ok. But what if a client decides to use a heavy model (e.g. a satellite model). In that case all the mercator points need to be transformed into ECEF and this could be quite expensive since the operation is not a linear one that could be done like before using a standard transformation matrix. What are your thoughts on that?

@karimnaaji karimnaaji force-pushed the karim/simplify-globe-custom-layer-interface branch from 677fdec to 1b54482 Compare January 30, 2023 20:46
@karimnaaji karimnaaji marked this pull request as ready for review January 30, 2023 23:09
@karimnaaji karimnaaji requested a review from a team as a code owner January 30, 2023 23:09
@karimnaaji
Copy link
Contributor Author

@akoylasar the changes suggested as part of PR review were implemented here, please take it from there to add extra requirement on 3d models example and adapt the API as needed.

@karimnaaji karimnaaji changed the title PoC: Simplify globe custom layer interface Simplify globe custom layer interface Jan 30, 2023
@sienki-jenki
Copy link

Hey, for past few days I'm following progress on custom layers in globe view and would like to discuss about usage of CustomLayerInterface. After @akoylasar merged #12182 I started exploring what can I do with custom layers in globe using Three.js (I'm not an expert in webgl, so writing shaders from scratch is painful).
Quickly, I managed to render some stuff on the surface of globe, as following:

camera.projectionMatrix = new Matrix4().fromArray(projectionMatrix).multiply(new Matrix4().fromArray(globeToMercMatrix));

and also adjusting coordinates to ECEF from mercator.

Problem appeared during transitioning from globe to mercator projection at the edges of camera frustum, objects further from the center of camera were "sliding" towards middle - probably that's why this PR is created.

My concern is that (AFAIK) if this PR will get merged every object in globe view would have to use custom shaders to adjust for both projections and transitioning.
Wouldn't it be easier for the end user to just adjust projectionMatrix in render method using arguments that render method receives?
@karimnaaji

@karimnaaji
Copy link
Contributor Author

karimnaaji commented Feb 1, 2023

Hey @sienki-jenki !

Problem appeared during transitioning from globe to mercator projection at the edges of camera frustum, objects further from the center of camera were "sliding" towards middle - probably that's why this PR is created.

Exactly, this was fixed in #12544, but it requires quite some extra code for the user to add, which we'd like to prevent. threejs may be a bit more challenging, as the threejs API can be used without writing shader code. In our case, we need extra GPU code to interpolate between two vertex arrays to support the transition properly, and it isn't a transformation we can encode in a single matrix at the moment.

Do you have the threejs example that has the issue you mentioned? That would be helpful to clarify this use case, and a potential solution might be going through writing a custom shader for your threejs object.

@karimnaaji
Copy link
Contributor Author

Removing release-blocker as this needs more examples to validate the immediate mode API. Immediate mode support will be excluded from changelog for now.

@sienki-jenki
Copy link

sienki-jenki commented Feb 3, 2023

Hey @karimnaaji , sorry for late response, I had some troubles making it work from this branch 😅
Here is your PR with my additional commit containing my use case for custom layers with Three.js (very rough proof of concept, but correctly demonstrates my use case)
sienki-jenki#1
And also quick video demonstrating current problem.
https://www.loom.com/share/a16d99adc12f4fcbb533f25d8bcba846

I can already tell that the way I implemented render method seems wrong since I'm not using matrix that render method supplies but I just couldn't make it work with matrix that this PR changed.
Other than that, my use case is more complicated regarding custom shaders because I'm using InstancedMesh and I just couldn't grasp adjustment of custom shaders to support smooth transition from globe to mercator in such case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants