feat(mapbox): support Maplibre globe projection #9296
Conversation
Pessimistress
force-pushed
the
x/mapbox-overlay-globe
branch
from
f9e58ad to
faa9aed
Compare
Pessimistress
force-pushed
the
x/mapbox-overlay-globe
branch
from
faa9aed to
90eff32
Compare
| @@ -58,6 +58,10 @@ export type GlobeViewportOptions = { | |||
| nearZMultiplier?: number; | |||
| /** Scaler for the far plane, 1 unit equals to the distance from the camera to the edge of the screen. Default `1` */ | |||
| farZMultiplier?: number; | |||
| /** Optionally override the near plane position. `nearZMultiplier` is ignored if `nearZ` is supplied. */ | |||
There was a problem hiding this comment.
A bit nervous about having these overrides, especially as nearZ and nearZMultiplier do the same thing. Should we issue a warning if both are supplied?
There was a problem hiding this comment.
I agree this can be confusing (and wasn't an easy thing to catch when I was first learning it). FWIW, web-mercator-viewport has had these same overrides for a long time.
A warning would be nice.. the multipliers have defaults assigned, so we'd just need to assign those after checking the user-supplied values (or else, they'd always print).
There was a problem hiding this comment.
This does look confusing and somewhat intimidating. I think these props were originally intended as a semi-undocumented escape hatch but now I seem to see these being "touched" in every PR...
If not a coincidence, might be good to audit this API with an eye towards making it as fail proof as possible for new (and old) users.
| if (type === 'globe') { | ||
| return 'globe'; | ||
| } | ||
| return 'mercator'; |
There was a problem hiding this comment.
warn when type is not mercator?
|
|
||
| export function getDefaultView(map: Map): GlobeView | MapView { | ||
| if (getProjection(map) === 'globe') { | ||
| return new GlobeView({id: 'mapbox'}); |
There was a problem hiding this comment.
Detect maplibre and instead call view 'maplibre'? Or choose a common name like 'map-overlay'?
There was a problem hiding this comment.
We don't have a reliable way to detect mapbox vs maplibre, given that we also support a wide range of versions of each library.
There was a problem hiding this comment.
I agree it's not the best name. Unfortunately, this is used in some edge cases... @chrisgervang
There was a problem hiding this comment.
Is it a question of whether this would be a breaking change for the multi-view edge cases?
| // @ts-ignore (2345) map is always defined if deck is | ||
| deck.setProps({viewState: getViewState(this._map)}); | ||
| const map = this._map; | ||
| if (deck && map) { |
There was a problem hiding this comment.
I think it is actually better to crash if our assumption is that map is defined is invalid
| // SPDX-License-Identifier: MIT | ||
| // Copyright (c) vis.gl contributors | ||
|
|
||
| import {MapboxOverlay as DeckOverlay} from '@deck.gl/mapbox'; |
There was a problem hiding this comment.
Why rename/alias things? If we are not happy with the name, rename it at export? If trying to make code clearer a longer name might be needed? Any name that mentions either just Deck or Mapbox is going to be confusing
| @@ -58,6 +58,10 @@ export type GlobeViewportOptions = { | |||
| nearZMultiplier?: number; | |||
| /** Scaler for the far plane, 1 unit equals to the distance from the camera to the edge of the screen. Default `1` */ | |||
| farZMultiplier?: number; | |||
| /** Optionally override the near plane position. `nearZMultiplier` is ignored if `nearZ` is supplied. */ | |||
There was a problem hiding this comment.
This does look confusing and somewhat intimidating. I think these props were originally intended as a semi-undocumented escape hatch but now I seem to see these being "touched" in every PR...
If not a coincidence, might be good to audit this API with an eye towards making it as fail proof as possible for new (and old) users.
For #9199
Note: requires #9295
Tested for overlaid and interleaved modes
Change List
MapboxOverlayswitches views based on map projection mode