-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix discontinuity when transitioning from globe to mercator in custom layer example #12544
Conversation
…custom layer immediate mode example -expose more variables for this purpose in prerender and render methods
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.
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; |
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.
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
.
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; |
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.
@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.
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.
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)
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.
Tried those suggestions in #12545.
Discussed with @akoylasar , let's merge this PR and continue on the discussion from #12544 (comment) in the other PR. |
This PR extends the
prerender
andrender
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
@mapbox/map-design-team
@mapbox/static-apis
if this PR includes style spec API or visual changes@mapbox/gl-native
if this PR includes shader changes or needs a native portmapbox-gl-js
changelog:<changelog></changelog>