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

fix discontinuity when transitioning from globe to mercator in custom layer example #12544

Merged

Conversation

akoylasar
Copy link
Contributor

@akoylasar akoylasar commented Jan 27, 2023

This PR extends the prerender and render methods for the custom layer when globe projection is in use. The new API exposes variables that can be used to also fix the issue described in #12182 (comment).

after:

after-.mov

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
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • 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>

…custom layer immediate mode example -expose more variables for this purpose in prerender and render methods
@akoylasar akoylasar added the skip changelog Used for PRs that do not need a changelog entry label Jan 27, 2023
@akoylasar akoylasar requested a review from a team as a code owner January 27, 2023 01:47
@akoylasar akoylasar changed the title fix discontinuity when transitioning from globe to mercator in globe … fix discontinuity when transitioning from globe to mercator in custom layer example Jan 27, 2023
Copy link
Contributor

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

Looks good to me, but worth another look from @mapbox/gl-js 👍

@@ -7,7 +7,7 @@ import assert from 'assert';
import type {ValidationErrors} from '../validate_style.js';
import type {ProjectionSpecification} from '../../style-spec/types.js';

type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>, projection: ?ProjectionSpecification, projectionToMercatorMatrix: ?Array<number>, projectionToMercatorTransition: ?number) => void;
type CustomRenderMethod = (gl: WebGLRenderingContext, matrix: Array<number>, projection: ?ProjectionSpecification, projectionToMercatorMatrix: ?Array<number>, projectionToMercatorTransition: ?number, centerInMercator: ?Array<number>, pixelsPerMeterRatio: ?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.

Adding more parameter gives me another reason to point out this suggestion: #12182 (comment) to clean this interface before we publish it publicly.

Or, if we could eliminate most of these parameters per below comment altogether that would be even more ideal to reduce a bit of the complexity for the user; e.g. remove the need to pass along globeToMercMatrix, transition, centerInMercator, and pixelsPerMeterRatio.

Comment on lines 19 to +28
void main() {
vec4 p = u_projection * u_globeToMercMatrix * vec4(a_pos_ecef, 1.);
p /= p.w;
if (u_globeToMercatorTransition > 0.) {
vec4 merc = u_projection * vec4(a_pos_merc, 1.);

vec4 merc = vec4(a_pos_merc, 1.);
merc.xy = (merc.xy - u_centerInMerc) * u_pixelsPerMeterRatio + u_centerInMerc;
merc.z *= u_pixelsPerMeterRatio;

merc = u_projection * merc;
Copy link
Contributor

Choose a reason for hiding this comment

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

@astojilj @akoylasar Are there mechanisms that we could adopt to include related code/functions to facilitate these conversions for the user? I imagine that this isn't trivial knowledge for them to successfully implement the conversions. As a user I would only want to care about my custom layer implementation (creating data in each of these world coordinates + projecting it).

My main concern is that this requires quite a bit of understanding of the internal workings of mapbox-gl. What I would want to care as a developer using globe custom layers is the following:

  • creating ECEF coordinates for globe
  • creating mercator coordinates for mercator
  • projecting each of these without thinking about internal details

The most minimal custom layer code I'd imagine someone would write:

gl_Position = custom_layer_project(a_pos_ecef, a_pos_merc);

Then, our internal boilerplate code would be injected and have the following:

uniform mat4 u_projection;
uniform mat4 u_globeToMercMatrix;
uniform float u_globeToMercatorTransition;
uniform vec2 u_centerInMerc;
uniform float u_pixelsPerMeterRatio;

vec4 project_custom_layer(vec3 pos_ecef, vec3 pos_merc) {
    vec4 projected_position = u_projection * u_globeToMercMatrix * vec4(a_pos_ecef, 1.);
    projected_position /= projected_position.w;
    if (u_globeToMercatorTransition > 0.) {
        vec4 merc = u_projection * vec4(a_pos_merc, 1.);
        vec4 merc = vec4(a_pos_merc, 1.);
        merc.xy = (merc.xy - u_centerInMerc) * u_pixelsPerMeterRatio + u_centerInMerc;
        merc.z *= u_pixelsPerMeterRatio;
        merc = u_projection * merc;
        merc /= merc.w;
        projected_position = mix(projected_position, merc, u_globeToMercatorTransition);
    }
    return projected_position;
}

The above shader code would be injected by default, and uniforms could be setup on our side prior to call onAdd and updated prior to the render function.

By hiding some of that complexity, we would also not have have to carry so many parameters to the render function.

Copy link
Contributor

Choose a reason for hiding this comment

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

More thoughts on the shader code injection point, the header shader code could be simply added as

this.globeProgram = createProgram(gl, mapboxgl.getGlobeVertexHeader().concat(globeVertCode), fragCode); 

And uniforms uploaded with a specific function taking in the program id

mapboxgl.updateCustomGlobeShader(this.globeProgram)

and I think that would cover it?

Then, render interface becomes:

render (gl, projectionMatrix, projection)

instead of:

render (gl, projectionMatrix, projection, globeToMercMatrix, transition, centerInMercator, pixelsPerMeterRatio)

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried those suggestions in #12545.

@karimnaaji
Copy link
Contributor

Discussed with @akoylasar , let's merge this PR and continue on the discussion from #12544 (comment) in the other PR.

@akoylasar akoylasar merged commit f5250ce into main Jan 30, 2023
@akoylasar akoylasar deleted the fouad/fix_discont_globe_merc_transition_custom_layer_example branch January 30, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release blocker ⛔ 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.

None yet

3 participants