Match GlobeView projection parameters with Maplibre v5#9201
Match GlobeView projection parameters with Maplibre v5#9201Pessimistress merged 5 commits intomasterfrom
Conversation
felixpalmer
left a comment
There was a problem hiding this comment.
Looks promising, but I get the page freezing after playing with the controls when viewing the full globe
|
|
||
| // matches Web Mercator projection limit | ||
| const MAX_VALID_LATITUDE = 85.051129; | ||
|
|
There was a problem hiding this comment.
I'm seeing strange things when moving near the poles
Screen.Recording.2024-10-04.at.10.40.03.mov
| zoom: 0, | ||
| pitch: 0, | ||
| bearing: 0 | ||
| zoom: -1 |
There was a problem hiding this comment.
What's a negative zoom mean for globe?
| @@ -50,6 +51,8 @@ export type GlobeViewportOptions = { | |||
| zoom?: number; | |||
There was a problem hiding this comment.
Worth adding a comment about zoom's range in a globe view?
ibgreen
left a comment
There was a problem hiding this comment.
This looks quite clean, I don't see any immediate reason why we wouldn't want to merge this as experimental in 9.1.
| props.longitude = mod(longitude + 180, 360) - 180; | ||
| } | ||
| props.latitude = clamp(latitude, -89, 89); | ||
| props.latitude = clamp(latitude, -MAX_VALID_LATITUDE, MAX_VALID_LATITUDE); |
There was a problem hiding this comment.
Can we go all the way? or do we need some offset to avoid issues?
There was a problem hiding this comment.
If we let latitude=90 then the controller goes into gimbal lock. It is a solvable problem, but irrelevant to this PR.
| } = opts; | ||
|
|
||
| let {height, altitude = 1.5} = opts; | ||
| let {height, altitude = 1.5, fovy} = opts; |
There was a problem hiding this comment.
Nit: FWIW, I have consistently been adding a defaultOptions object to classes in luma.gl so that I can document all default options in one place rather than spreading out the default values in a bunch of descructuring calls like this.
static defaultOptions: Required<GlobeViewportOptions> = {
...
};
this.options ={...GlobeViewport.defaultOptions, ...options};|
|
||
| get ViewportType() { | ||
| return GlobeViewport; | ||
| getViewportType(viewState: GlobeViewState) { |
There was a problem hiding this comment.
- Nit: It just seems a bit weird that one of these takes an argument and the others do not. Should we allow it to be omitted?
- Not sure if making the types less restrictive could be useful? - i.e. only ask for what we need to answer the question
| getViewportType(viewState: GlobeViewState) { | |
| getViewportType(viewState: {zoom: number}) { |
|
|
||
| height = height || 1; | ||
| altitude = Math.max(0.75, altitude); | ||
| if (fovy) { |
There was a problem hiding this comment.
This aligns us more with other views, right?
We always support fovy and altitude is a backup for geospatial views?
There was a problem hiding this comment.
That is correct.
altitude is a parameter we inherited from the mapbox/maplibre. The name is a bit misleading. It refers to the camera's distance to the sea plane.
felixpalmer
left a comment
There was a problem hiding this comment.
@Pessimistress I believe the page freeze is unrelated to this PR #9265
3fe3bc3 to
2f8996f
Compare
For #9199
Grid lines are rendered by Maplibre, points are rendered by deck.gl.
Change List
GlobeViewport's scale, nearZ, farZ with maplibre'sGlobeTransformGlobeViewswitch toWebMercatorViewportat zoom>=12. maplibre performs interpolation between the two projections at z [11, 12]. We may need to do the same, but the difference is honestly very subtle.