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

Match GlobeView projection parameters with Maplibre v5 #9201

Open
wants to merge 3 commits into
base: x/globe-test-app
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion modules/core/src/controllers/globe-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm seeing strange things when moving near the poles

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

class GlobeState extends MapState {
// Apply any constraints (mathematical or defined by _viewportProps) to map state
applyConstraints(props: Required<MapStateProps>): Required<MapStateProps> {
Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


return props;
}
Expand Down
36 changes: 21 additions & 15 deletions modules/core/src/viewports/globe-viewport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import {Matrix4} from '@math.gl/core';
import Viewport from './viewport';
import {PROJECTION_MODE} from '../lib/constants';
import {altitudeToFovy, fovyToAltitude} from '@math.gl/web-mercator';

import {vec3, vec4} from '@math.gl/core';

Expand Down Expand Up @@ -50,6 +51,8 @@ export type GlobeViewportOptions = {
zoom?: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

/** Use orthographic projection */
orthographic?: boolean;
/** Camera fovy in degrees. If provided, overrides `altitude` */
fovy?: number;
/** Scaler for the near plane, 1 unit equals to the height of the viewport. Default `0.1` */
nearZMultiplier?: number;
/** Scaler for the far plane, 1 unit equals to the distance from the camera to the edge of the screen. Default `2` */
Expand All @@ -59,37 +62,39 @@ export type GlobeViewportOptions = {
};

export default class GlobeViewport extends Viewport {
// @ts-ignore
longitude: number;
// @ts-ignore
latitude: number;
resolution: number;
longitude!: number;
latitude!: number;
resolution!: number;

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

Choose a reason for hiding this comment

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

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

farZMultiplier = 2,
nearZMultiplier = 0.5,
farZMultiplier = 1,
resolution = 10
} = opts;

let {height, altitude = 1.5} = opts;
let {height, altitude = 1.5, fovy} = opts;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


height = height || 1;
altitude = Math.max(0.75, altitude);
if (fovy) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

altitude = fovyToAltitude(fovy);
} else {
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

const scale = Math.pow(2, zoom) * scaleAdjust;
const farZ = altitude + (GLOBE_RADIUS * 2 * scale) / height;

// Calculate view matrix
const viewMatrix = new Matrix4().lookAt({eye: [0, -altitude, 0], up: [0, 0, 1]});
const scale = Math.pow(2, zoom);
viewMatrix.rotateX(latitude * DEGREES_TO_RADIANS);
viewMatrix.rotateZ(-longitude * DEGREES_TO_RADIANS);
viewMatrix.scale(scale / height);

const halfFov = Math.atan(0.5 / altitude);
const relativeScale = (GLOBE_RADIUS * 2 * scale) / height;

super({
...opts,
// x, y, width,
Expand All @@ -103,12 +108,13 @@ export default class GlobeViewport extends Viewport {

// projection matrix parameters
distanceScales: getDistanceScales(),
fovyRadians: halfFov * 2,
fovy,
focalDistance: altitude,
near: nearZMultiplier,
far: Math.min(2, 1 / relativeScale + 1) * altitude * farZMultiplier
far: farZ * farZMultiplier
});

this.scale = scale;
this.latitude = latitude;
this.longitude = longitude;
this.resolution = resolution;
Expand Down
2 changes: 1 addition & 1 deletion modules/core/src/views/first-person-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export default class FirstPersonView extends View<FirstPersonViewState, FirstPer
super(props);
}

get ViewportType() {
getViewportType() {
return FirstPersonViewport;
}

Expand Down
5 changes: 3 additions & 2 deletions modules/core/src/views/globe-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import View, {CommonViewState, CommonViewProps} from './view';
import GlobeViewport from '../viewports/globe-viewport';
import WebMercatorViewport from '../viewports/web-mercator-viewport';
import GlobeController from '../controllers/globe-controller';

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

get ViewportType() {
return GlobeViewport;
getViewportType(viewState: GlobeViewState) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 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}) {

return viewState.zoom > 12 ? WebMercatorViewport : GlobeViewport;
}

get ControllerType() {
Expand Down
2 changes: 1 addition & 1 deletion modules/core/src/views/map-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export default class MapView extends View<MapViewState, MapViewProps> {
super(props);
}

get ViewportType() {
getViewportType() {
return WebMercatorViewport;
}

Expand Down
2 changes: 1 addition & 1 deletion modules/core/src/views/orbit-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export default class OrbitView extends View<OrbitViewState, OrbitViewProps> {
this.props.orbitAxis = props.orbitAxis || 'Z';
}

get ViewportType() {
getViewportType() {
return OrbitViewport;
}

Expand Down
2 changes: 1 addition & 1 deletion modules/core/src/views/orthographic-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export default class OrthographicView extends View<OrthographicViewState, Orthog
super(props);
}

get ViewportType() {
getViewportType() {
return OrthographicViewport;
}

Expand Down
7 changes: 4 additions & 3 deletions modules/core/src/views/view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export default abstract class View<
ViewProps extends CommonViewProps<ViewState> = CommonViewProps<ViewState>
> {
id: string;
abstract get ViewportType(): ConstructorOf<Viewport>;
abstract getViewportType(viewState: ViewState): ConstructorOf<Viewport>;
protected abstract get ControllerType(): ConstructorOf<Controller<any>>;

private _x: Position;
Expand Down Expand Up @@ -102,7 +102,7 @@ export default abstract class View<
}

// To correctly compare padding use depth=2
return this.ViewportType === view.ViewportType && deepEqual(this.props, view.props, 2);
return this.constructor === view.constructor && deepEqual(this.props, view.props, 2);
}

/** Make viewport from canvas dimensions and view state */
Expand All @@ -114,7 +114,8 @@ export default abstract class View<
if (!viewportDimensions.height || !viewportDimensions.width) {
return null;
}
return new this.ViewportType({...viewState, ...this.props, ...viewportDimensions});
const ViewportType = this.getViewportType(viewState);
return new ViewportType({...viewState, ...this.props, ...viewportDimensions});
}

getViewStateId(): string {
Expand Down
2 changes: 1 addition & 1 deletion test/modules/core/viewports/globe-viewport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const TEST_VIEWPORTS = [
{
width: 800,
height: 600,
latitude: 90,
latitude: 80,
longitude: 0,
zoom: 1
}
Expand Down
2 changes: 1 addition & 1 deletion test/modules/geo-layers/tileset-2d/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
}),
tileSize: 512,
Expand Down
Binary file modified test/render/golden-images/globe-mvt.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/render/golden-images/path-globe.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/render/golden-images/polygon-globe-extruded.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/render/golden-images/polygon-globe.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion test/render/test-cases/path-layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ export default [
viewState: {
latitude: 0,
longitude: 0,
zoom: 0
zoom: 1.5
},
layers: [
new PathLayer({
Expand Down
4 changes: 2 additions & 2 deletions test/render/test-cases/polygon-layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export default [
viewState: {
latitude: 0,
longitude: 50,
zoom: 0
zoom: 1.5
},
layers: [
new PolygonLayer({
Expand Down Expand Up @@ -184,7 +184,7 @@ export default [
viewState: {
latitude: 0,
longitude: 50,
zoom: 0
zoom: 1.5
},
layers: [
new PolygonLayer({
Expand Down
4 changes: 1 addition & 3 deletions test/render/test-cases/views.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,7 @@ export default [
viewState: {
longitude: -100,
latitude: 80,
zoom: 0,
pitch: 0,
bearing: 0
zoom: -1
Copy link
Collaborator

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?

},
layers: [
new SimpleMeshLayer({
Expand Down
Loading