Skip to content

Commit

Permalink
Avoid constraining with Mercator transform when Globe is used (#5186)
Browse files Browse the repository at this point in the history
* Lazily constrain hash values on map constructor

* Add tests

* Add Changelog entry

* Fix tests

* Simplify condition

* Remove duplicated code

* Deconstruct array

---------

Co-authored-by: Harel M <[email protected]>
  • Loading branch information
ibesora and HarelM authored Dec 18, 2024
1 parent 35ea568 commit 5d5a2cd
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 15 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

- Fix holes at the poles when terrain is used with globe ([#5232](https://github.com/maplibre/maplibre-gl-js/pull/5232))
- Fix geometry artifacts when globe terrain is zoomed out too much ([#5232](https://github.com/maplibre/maplibre-gl-js/pull/5232))
- Fix center being incorrectly constrained when using globe ([#5186](https://github.com/maplibre/maplibre-gl-js/pull/5186))
- Fix atmosphere improperly blending into the background ([#5235](https://github.com/maplibre/maplibre-gl-js/pull/5235))
- _...Add new stuff here..._

Expand Down
4 changes: 2 additions & 2 deletions src/geo/projection/globe_transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ export class GlobeTransform implements ITransform {
isPaddingEqual(padding: PaddingOptions): boolean {
return this._helper.isPaddingEqual(padding);
}
resize(width: number, height: number): void {
this._helper.resize(width, height);
resize(width: number, height: number, constrainTransform: boolean = true): void {
this._helper.resize(width, height, constrainTransform);
}
getMaxBounds(): LngLatBounds {
return this._helper.getMaxBounds();
Expand Down
4 changes: 2 additions & 2 deletions src/geo/projection/mercator_transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ export class MercatorTransform implements ITransform {
isPaddingEqual(padding: PaddingOptions): boolean {
return this._helper.isPaddingEqual(padding);
}
resize(width: number, height: number): void {
this._helper.resize(width, height);
resize(width: number, height: number, constrain: boolean = true): void {
this._helper.resize(width, height, constrain);
}
getMaxBounds(): LngLatBounds {
return this._helper.getMaxBounds();
Expand Down
4 changes: 2 additions & 2 deletions src/geo/transform_helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,10 +425,10 @@ export class TransformHelper implements ITransformGetters {
this._calcMatrices();
}

resize(width: number, height: number) {
resize(width: number, height: number, constrain: boolean = true): void {
this._width = width;
this._height = height;
this._constrain();
if (constrain) this._constrain();
this._calcMatrices();
}

Expand Down
2 changes: 1 addition & 1 deletion src/geo/transform_interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ interface ITransformMutators {
/**
* Sets the transform's width and height and recomputes internal matrices.
*/
resize(width: number, height: number): void;
resize(width: number, height: number, constrainTransform: boolean): void;
/**
* Helper method to update edge-insets in place
*
Expand Down
24 changes: 17 additions & 7 deletions src/ui/map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,11 @@ export class Map extends Camera {
}
}

this.resize();
// When no style is set or it's using something other than the globe projection, we can constrain the camera.
// When a style is set with other projections though, we can't constrain the camera until the style is loaded
// and the correct transform is used. Otherwise, valid points in the desired projection could be rejected
const shouldConstrainUsingMercatorTransform = typeof resolvedOptions.style === 'string' || !(resolvedOptions.style?.projection?.type === 'globe');
this.resize(null, shouldConstrainUsingMercatorTransform);

this._localIdeographFontFamily = resolvedOptions.localIdeographFontFamily;
this._validateStyle = resolvedOptions.validateStyle;
Expand All @@ -734,6 +738,8 @@ export class Map extends Camera {
this.addControl(new LogoControl(), resolvedOptions.logoPosition);

this.on('style.load', () => {
// If we didn't constrain the camera before, we do it now
if (!shouldConstrainUsingMercatorTransform) this._resizeTransform();
if (this.transform.unmodified) {
const coercedOptions = pick(this.style.stylesheet, ['center', 'zoom', 'bearing', 'pitch', 'roll']) as CameraOptions;
this.jumpTo(coercedOptions);
Expand Down Expand Up @@ -873,10 +879,8 @@ export class Map extends Camera {
* if (mapDiv.style.visibility === true) map.resize();
* ```
*/
resize(eventData?: any): Map {
const dimensions = this._containerDimensions();
const width = dimensions[0];
const height = dimensions[1];
resize(eventData?: any, constrainTransform = true): Map {
const [width, height] = this._containerDimensions();

const clampedPixelRatio = this._getClampedPixelRatio(width, height);
this._resizeCanvas(width, height, clampedPixelRatio);
Expand All @@ -892,8 +896,7 @@ export class Map extends Camera {
this.painter.resize(width, height, clampedPixelRatio);
}

this.transform.resize(width, height);
this._requestedCameraState?.resize(width, height);
this._resizeTransform(constrainTransform);

const fireMoving = !this._moving;
if (fireMoving) {
Expand All @@ -909,6 +912,13 @@ export class Map extends Camera {
return this;
}

_resizeTransform(constrainTransform = true) {
const [width, height] = this._containerDimensions();

this.transform.resize(width, height, constrainTransform);
this._requestedCameraState?.resize(width, height, constrainTransform);
}

/**
* @internal
* Return the map's pixel ratio eventually scaled down to respect maxCanvasSize.
Expand Down
2 changes: 1 addition & 1 deletion src/ui/map_tests/map_style.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('#setStyle', () => {
test('style transform overrides unmodified map transform', () => new Promise<void>(done => {
const map = new Map({container: window.document.createElement('div')} as any as MapOptions);
map.transform.setMaxBounds(new LngLatBounds([-120, -60], [140, 80]));
map.transform.resize(600, 400);
map.transform.resize(600, 400, true);
expect(map.transform.zoom).toBe(0.6983039737971013);
expect(map.transform.unmodified).toBeTruthy();
map.setStyle(createStyle());
Expand Down
85 changes: 85 additions & 0 deletions src/ui/map_tests/map_transform_constrains.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import {beforeEach, test, expect} from 'vitest';
import {createMap, beforeMapTest} from '../../util/test/util';
import {fixedLngLat, fixedNum} from '../../../test/unit/lib/fixed';

beforeEach(() => {
beforeMapTest();
global.fetch = null;
});

test('Creating a map without style constrains invalid hash values to valid Mercator transform values', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [-180, -90], zoom: -2});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 180, lat: 0});
expect(fixedNum(map.getZoom(), 3)).toBe(-0.771);
});

test('Creating a map without style keeps valid hash values as Mercator transform values', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [15, 30], zoom: 3});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 15, lat: 30});
expect(fixedNum(map.getZoom(), 3)).toBe(3);
});

test('Creating a map with style but no projection uses Mercator transform constrains', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [-180, -90], zoom: -2, style: {version: 8, sources: {}, layers: []}});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 180, lat: 0});
expect(fixedNum(map.getZoom(), 3)).toBe(-0.771);
});

test('Creating a map with style but no projection does not constrain valid values', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [15, 30], zoom: 3, style: {version: 8, sources: {}, layers: []}});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 15, lat: 30});
expect(fixedNum(map.getZoom(), 3)).toBe(3);
});

test('Creating a map with style defining mercator projection uses Mercator transform constrains', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [-180, -90], zoom: -2, style: {version: 8, sources: {}, layers: [], projection: {type: 'mercator'}}});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 180, lat: 0});
expect(fixedNum(map.getZoom(), 3)).toBe(-0.771);
});

test('Creating a map with style defining mercator projection does not constrain valid values', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [15, 30], zoom: 3, style: {version: 8, sources: {}, layers: [], projection: {type: 'mercator'}}});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 15, lat: 30});
expect(fixedNum(map.getZoom(), 3)).toBe(3);
});

test('Creating a map with style defining globe projection uses Globe transform constrains', () => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const map = createMap({container, center: [65.7, -38.2], zoom: -2, style: {version: 8, sources: {}, layers: [], projection: {type: 'globe'}}});

expect(fixedLngLat(map.getCenter(), 4)).toEqual({lng: 65.7, lat: -38.2});
expect(fixedNum(map.getZoom(), 3)).toBe(-2);
});

0 comments on commit 5d5a2cd

Please sign in to comment.