Match GlobeView projection parameters with Maplibre v5 #9201

Pessimistress · 2024-10-04T05:13:56Z

For #9199

Grid lines are rendered by Maplibre, points are rendered by deck.gl.

Change List

Match GlobeViewport's scale, nearZ, farZ with maplibre's GlobeTransform
GlobeView switch to WebMercatorViewport at 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.
Some golden images are updated because the zoom -> scale mapping has changed. The new implementation uses an adaptive scale that depends on the latitude of the viewport center.

felixpalmer

Looks promising, but I get the page freezing after playing with the controls when viewing the full globe

felixpalmer · 2024-10-04T08:32:31Z

modules/core/src/controllers/globe-controller.ts

@@ -9,6 +9,9 @@ import {MapState, MapStateProps} from './map-controller';
 import {mod} from '../utils/math-utils';
 import LinearInterpolator from '../transitions/linear-interpolator';

+// matches Web Mercator projection limit
+const MAX_VALID_LATITUDE = 85.051129;


I wonder if math.gl shouldn't export this - it is used in a number of places

+1 - we have a web-mercator module after all, and this is one of the primary web-mercator constants.

felixpalmer · 2024-10-04T08:41:15Z

modules/core/src/controllers/globe-controller.ts

@@ -9,6 +9,9 @@ import {MapState, MapStateProps} from './map-controller';
 import {mod} from '../utils/math-utils';
 import LinearInterpolator from '../transitions/linear-interpolator';

+// matches Web Mercator projection limit
+const MAX_VALID_LATITUDE = 85.051129;
+


I'm seeing strange things when moving near the poles

Screen.Recording.2024-10-04.at.10.40.03.mov

felixpalmer · 2024-10-04T08:45:18Z

modules/core/src/viewports/globe-viewport.ts


  constructor(opts: GlobeViewportOptions = {}) {
    const {
      latitude = 0,
      longitude = 0,
      zoom = 0,
-      nearZMultiplier = 0.1,


Can you document somewhere how you updated these defaults? Is there some magic formula?

felixpalmer · 2024-10-04T08:48:23Z

test/modules/geo-layers/tileset-2d/utils.spec.ts

@@ -275,7 +275,7 @@ const TEST_CASES = [
      viewState: {
        longitude: -6,
        latitude: 58,
-        zoom: 1.5
+        zoom: 1.5 + 0.7353406094252244 // Math.log2(1 / Math.PI / Math.cos(58 / 180 * Math.PI))


I feel it is better to update the image than having some weird constant here. There is a significant change in output anyway

Does this constant help see us abnormalities in the images, or have some other benefit for testing?

Nice if comment explained what the formula is doing in a few words.

felixpalmer · 2024-10-04T09:05:38Z

modules/core/src/viewports/globe-viewport.ts

+      fovy = altitudeToFovy(altitude);
+    }
+    // Used to match globe and web mercator projection at high zoom
+    const scaleAdjust = 1 / Math.PI / Math.cos((latitude * Math.PI) / 180);


Worth guarding against division by 0? Despite this, the viewport could still be manually constructed with the pole value

We have had a LOT of problems over the years in deck and kepler with mercator viewports throwing due to bad inputs somehow being generated - every time it arises, it confuses a couple of new engineers and PMs, tickets get created, discussions in sprint reviews etc - ultimately hours of time are lost and little is done.

So I agree that avoiding the issue would be great. The question is what to do instead. Force the value to the closest value that will work?

chrisgervang · 2024-11-22T20:17:21Z

test/render/test-cases/views.js

-        zoom: 0,
-        pitch: 0,
-        bearing: 0
+        zoom: -1


What's a negative zoom mean for globe?

chrisgervang · 2024-11-22T20:18:17Z

modules/core/src/viewports/globe-viewport.ts

@@ -50,6 +51,8 @@ export type GlobeViewportOptions = {
  zoom?: number;


Worth adding a comment about zoom's range in a globe view?

ibgreen

This looks quite clean, I don't see any immediate reason why we wouldn't want to merge this as experimental in 9.1.

ibgreen · 2024-11-25T15:16:48Z

modules/core/src/controllers/globe-controller.ts

@@ -9,6 +9,9 @@ import {MapState, MapStateProps} from './map-controller';
 import {mod} from '../utils/math-utils';
 import LinearInterpolator from '../transitions/linear-interpolator';

+// matches Web Mercator projection limit
+const MAX_VALID_LATITUDE = 85.051129;


+1 - we have a web-mercator module after all, and this is one of the primary web-mercator constants.

ibgreen · 2024-11-25T15:17:49Z

modules/core/src/controllers/globe-controller.ts

@@ -20,7 +23,7 @@ class GlobeState extends MapState {
    if (longitude < -180 || longitude > 180) {
      props.longitude = mod(longitude + 180, 360) - 180;
    }
-    props.latitude = clamp(latitude, -89, 89);
+    props.latitude = clamp(latitude, -MAX_VALID_LATITUDE, MAX_VALID_LATITUDE);


Can we go all the way? or do we need some offset to avoid issues?

ibgreen · 2024-11-25T15:22:27Z

modules/core/src/viewports/globe-viewport.ts

      resolution = 10
    } = opts;

-    let {height, altitude = 1.5} = opts;
+    let {height, altitude = 1.5, fovy} = opts;


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};

ibgreen · 2024-11-25T15:24:58Z

modules/core/src/viewports/globe-viewport.ts

+      fovy = altitudeToFovy(altitude);
+    }
+    // Used to match globe and web mercator projection at high zoom
+    const scaleAdjust = 1 / Math.PI / Math.cos((latitude * Math.PI) / 180);


We have had a LOT of problems over the years in deck and kepler with mercator viewports throwing due to bad inputs somehow being generated - every time it arises, it confuses a couple of new engineers and PMs, tickets get created, discussions in sprint reviews etc - ultimately hours of time are lost and little is done.

So I agree that avoiding the issue would be great. The question is what to do instead. Force the value to the closest value that will work?

ibgreen · 2024-11-25T15:59:41Z

modules/core/src/views/globe-view.ts

@@ -37,8 +38,8 @@ export default class GlobeView extends View<GlobeViewState, GlobeViewProps> {
    super(props);
  }

-  get ViewportType() {
-    return GlobeViewport;
+  getViewportType(viewState: GlobeViewState) {


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

Suggested change

getViewportType(viewState: GlobeViewState) {

getViewportType(viewState: {zoom: number}) {

ibgreen · 2024-11-25T16:04:18Z

test/modules/geo-layers/tileset-2d/utils.spec.ts

@@ -275,7 +275,7 @@ const TEST_CASES = [
      viewState: {
        longitude: -6,
        latitude: 58,
-        zoom: 1.5
+        zoom: 1.5 + 0.7353406094252244 // Math.log2(1 / Math.PI / Math.cos(58 / 180 * Math.PI))


Nice if comment explained what the formula is doing in a few words.

ibgreen · 2024-11-25T16:05:26Z

modules/core/src/viewports/globe-viewport.ts


    height = height || 1;
-    altitude = Math.max(0.75, altitude);
+    if (fovy) {


This aligns us more with other views, right?
We always support fovy and altitude is a backup for geospatial views?

ibgreen · 2024-11-25T16:07:09Z

modules/core/src/viewports/globe-viewport.ts

+      fovy = altitudeToFovy(altitude);
+    }
+    // Used to match globe and web mercator projection at high zoom
+    const scaleAdjust = 1 / Math.PI / Math.cos((latitude * Math.PI) / 180);


Is this the new math logic? Almost missed it. Not sure we can put this in a separate method or something to make it stand out mode?
Commenting generously here would be appreciated.

felixpalmer

@Pessimistress I believe the page freeze is unrelated to this PR #9265

Pessimistress added 2 commits October 3, 2024 18:30

align globe projection with maplibre

8e2141d

Update golden images

f33a217

Pessimistress requested review from felixpalmer and ibgreen October 4, 2024 05:13

fix unit test

3fe3bc3

felixpalmer requested changes Oct 4, 2024

View reviewed changes

birkskyum mentioned this pull request Nov 2, 2024

Rename adaptive globe to globe-to-mercator maplibre/maplibre-style-spec#878

Merged

1 task

chrisgervang reviewed Nov 22, 2024

View reviewed changes

test/render/test-cases/views.js

zoom: 0,

pitch: 0,

bearing: 0

zoom: -1

Copy link

Collaborator

chrisgervang Nov 22, 2024

Choose a reason for hiding this comment

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

What's a negative zoom mean for globe?

chrisgervang reviewed Nov 22, 2024

View reviewed changes

ibgreen approved these changes Nov 25, 2024

View reviewed changes

felixpalmer approved these changes Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match GlobeView projection parameters with Maplibre v5 #9201

Match GlobeView projection parameters with Maplibre v5 #9201

Pessimistress commented Oct 4, 2024

felixpalmer left a comment

felixpalmer Oct 4, 2024

ibgreen Nov 25, 2024

felixpalmer Oct 4, 2024

felixpalmer Oct 4, 2024

felixpalmer Oct 4, 2024

chrisgervang Nov 22, 2024

ibgreen Nov 25, 2024

felixpalmer Oct 4, 2024

ibgreen Nov 25, 2024

chrisgervang Nov 22, 2024

chrisgervang Nov 22, 2024

ibgreen left a comment

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

ibgreen Nov 25, 2024

felixpalmer left a comment

		@@ -50,6 +51,8 @@ export type GlobeViewportOptions = {
		zoom?: number;

	getViewportType(viewState: GlobeViewState) {
	getViewportType(viewState: {zoom: number}) {

Match GlobeView projection parameters with Maplibre v5 #9201

Are you sure you want to change the base?

Match GlobeView projection parameters with Maplibre v5 #9201

Conversation

Pessimistress commented Oct 4, 2024

Change List

felixpalmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixpalmer left a comment

Choose a reason for hiding this comment