diff --git a/CHANGELOG.md b/CHANGELOG.md index b606cdc780..b392d82c44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - _...Add new stuff here..._ ### 🐞 Bug fixes +- Fix marker flying off near horizon ([3704](https://github.com/maplibre/maplibre-gl-js/pull/3704)) - _...Add new stuff here..._ ## 4.0.1 diff --git a/src/ui/marker.test.ts b/src/ui/marker.test.ts index a583ddb9cd..ea450720da 100644 --- a/src/ui/marker.test.ts +++ b/src/ui/marker.test.ts @@ -845,7 +845,7 @@ describe('marker', () => { const marker = new Marker() .setLngLat([0, 0]) .addTo(map); - await sleep(100); + await sleep(500); expect(marker.getElement().style.opacity).toMatch('1'); map.remove(); }); @@ -855,7 +855,7 @@ describe('marker', () => { const marker = new Marker({opacity: '0.7'}) .setLngLat([0, 0]) .addTo(map); - await sleep(100); + await sleep(500); expect(marker.getElement().style.opacity).toMatch('.7'); map.remove(); }); @@ -894,21 +894,21 @@ describe('marker', () => { getElevationForLngLatZoom: () => 0, depthAtPoint: () => .95 // Mocking distance to terrain } as any as Terrain; - await sleep(100); + await sleep(500); map.fire('terrain'); expect(marker.getElement().style.opacity).toMatch('1'); // Terrain blocks marker map.terrain.depthAtPoint = () => .92; // Mocking terrain blocking marker - await sleep(100); + await sleep(500); map.fire('moveend'); expect(marker.getElement().style.opacity).toMatch('.2'); // Remove terrain map.terrain = null; - await sleep(100); + await sleep(500); map.fire('terrain'); expect(marker.getElement().style.opacity).toMatch('1'); @@ -926,7 +926,7 @@ describe('marker', () => { getElevationForLngLatZoom: () => 0, depthAtPoint: () => .95 } as any as Terrain; - await sleep(100); + await sleep(500); map.fire('terrain'); expect(marker.getElement().style.opacity).toMatch('.7'); @@ -944,7 +944,7 @@ describe('marker', () => { getElevationForLngLatZoom: () => 0, depthAtPoint: (p) => p.y === 256 ? .95 : .92 // return "far" given the marker's center coord; return "near" otherwise } as any as Terrain; - await sleep(100); + await sleep(500); map.fire('terrain'); expect(marker.getElement().style.opacity).toMatch('.7'); @@ -962,7 +962,7 @@ describe('marker', () => { getElevationForLngLatZoom: () => 0, depthAtPoint: () => .92 } as any as Terrain; - await sleep(100); + await sleep(500); map.fire('terrain'); expect(marker.getElement().style.opacity).toMatch('0.3'); diff --git a/src/ui/marker.ts b/src/ui/marker.ts index 278349f7e4..83d041b431 100644 --- a/src/ui/marker.ts +++ b/src/ui/marker.ts @@ -126,6 +126,7 @@ export class Marker extends Evented { _popup: Popup; _lngLat: LngLat; _pos: Point; + _flatPos: Point; _color: string; _scale: number; _defaultMarker: boolean; @@ -568,10 +569,14 @@ export class Marker extends Evented { } if (this._map.transform.renderWorldCopies) { - this._lngLat = smartWrap(this._lngLat, this._pos, this._map.transform); + this._lngLat = smartWrap(this._lngLat, this._flatPos, this._map.transform); } - this._pos = this._map.project(this._lngLat)._add(this._offset); + this._flatPos = this._pos = this._map.project(this._lngLat)._add(this._offset); + if (this._map.terrain) { + // flat position is saved because smartWrap needs non-elevated points + this._flatPos = this._map.transform.locationPoint(this._lngLat)._add(this._offset); + } let rotation = ''; if (this._rotationAlignment === 'viewport' || this._rotationAlignment === 'auto') { diff --git a/src/ui/popup.test.ts b/src/ui/popup.test.ts index 70f3015bfd..0ca077573e 100644 --- a/src/ui/popup.test.ts +++ b/src/ui/popup.test.ts @@ -334,9 +334,11 @@ describe('popup', () => { test('Popup is repositioned at the specified LngLat', () => { const map = createMap({width: 1024}); // longitude bounds: [-360, 360] - + map.terrain = { + getElevationForLngLatZoom: () => 0 + } as any; const popup = new Popup() - .setLngLat([270, 0]) + .setLngLat([70, 0]) .setText('Test') .addTo(map) .setLngLat([0, 0]); diff --git a/src/ui/popup.ts b/src/ui/popup.ts index a9f6aeef90..f1898d8501 100644 --- a/src/ui/popup.ts +++ b/src/ui/popup.ts @@ -158,6 +158,7 @@ export class Popup extends Evented { _lngLat: LngLat; _trackPointer: boolean; _pos: Point; + _flatPos: Point; constructor(options?: PopupOptions) { super(); @@ -570,12 +571,16 @@ export class Popup extends Evented { } if (this._map.transform.renderWorldCopies && !this._trackPointer) { - this._lngLat = smartWrap(this._lngLat, this._pos, this._map.transform); + this._lngLat = smartWrap(this._lngLat, this._flatPos, this._map.transform); } if (this._trackPointer && !cursor) return; - const pos = this._pos = this._trackPointer && cursor ? cursor : this._map.project(this._lngLat); + const pos = this._flatPos = this._pos = this._trackPointer && cursor ? cursor : this._map.project(this._lngLat); + if (this._map.terrain) { + // flat position is saved because smartWrap needs non-elevated points + this._flatPos = this._trackPointer && cursor ? cursor : this._map.transform.locationPoint(this._lngLat); + } let anchor = this.options.anchor; const offset = normalizeOffset(this.options.offset); diff --git a/src/util/smart_wrap.test.ts b/src/util/smart_wrap.test.ts new file mode 100644 index 0000000000..44c8ef230d --- /dev/null +++ b/src/util/smart_wrap.test.ts @@ -0,0 +1,97 @@ +import Point from '@mapbox/point-geometry'; +import {LngLat} from '../geo/lng_lat'; +import {Transform} from '../geo/transform'; +import {smartWrap} from './smart_wrap'; + +const transform = new Transform(); +transform.width = 100; +transform.height = 100; +transform.getHorizon = () => 0; // map center + +describe('smartWrap', () => { + test('Shifts lng to -360 deg when such point is closer to the priorPos', () => { + transform.locationPoint = ((ll: LngLat) => { + if (ll.lng === -360) { + return new Point(90, 51); // close to priorPos & below horizon + } else { + return new Point(120, 51); + } + }); + + const result = smartWrap( + new LngLat(0, 0), + new Point(100, 51), + transform); + expect(result.lng).toBe(-360); + }); + + test('Shifts lng to +360 deg when such point is closer to the priorPos', () => { + transform.locationPoint = ((ll: LngLat) => { + if (ll.lng === 360) { + return new Point(90, 51); // close to priorPos & below horizon + } else { + return new Point(120, 51); + } + }); + + const result = smartWrap( + new LngLat(0, 0), + new Point(100, 51), + transform); + expect(result.lng).toBe(360); + }); + + test('Does not change lng when there are no closer points at -360 and +360 deg', () => { + transform.locationPoint = ((ll: LngLat) => { + if (ll.lng === 15) { + return new Point(90, 51); // close to priorPos & below horizon + } else { + return new Point(12000, 51); + } + }); + + const result = smartWrap( + new LngLat(15, 0), + new Point(100, 51), + transform); + expect(result.lng).toBe(15); + }); + + test('Does not change lng to -360 deg when such point is above horizon', () => { + transform.locationPoint = ((ll: LngLat) => { + if (ll.lng === -360) { + return new Point(90, 49); // close to priorPos BUT above horizon + } else { + return new Point(120, 51); + } + }); + + const result = smartWrap( + new LngLat(0, 0), + new Point(100, 51), + transform); + expect(result.lng).toBe(0); + }); + + test('Shifts lng to -360 if lng is outside viewport on the right and at least 180° from map center', () => { + transform.center.lng = 50; + transform.locationPoint = (() => { return new Point(110, 51); }); // outside viewport + + const result = smartWrap( + new LngLat(250, 0), // 200 from map center + new Point(0, 0), // priorPos doesn't matter in this case + transform); + expect(result.lng).toBe(-110); + }); + + test('Shifts lng to +360 if lng is outside viewport on the left and at least 180° from map center', () => { + transform.center.lng = 50; + transform.locationPoint = (() => { return new Point(-10, 51); }); // outside viewport + + const result = smartWrap( + new LngLat(-150, 0), // 200 from map center + new Point(0, 0), // priorPos doesn't matter in this case + transform); + expect(result.lng).toBe(210); + }); +}); diff --git a/src/util/smart_wrap.ts b/src/util/smart_wrap.ts index 71f9e2b93a..2ef487e193 100644 --- a/src/util/smart_wrap.ts +++ b/src/util/smart_wrap.ts @@ -17,6 +17,7 @@ import type {Transform} from '../geo/transform'; * should wrap just enough to avoid doing so. */ export function smartWrap(lngLat: LngLat, priorPos: Point, transform: Transform): LngLat { + const originalLngLat = new LngLat(lngLat.lng, lngLat.lat); lngLat = new LngLat(lngLat.lng, lngLat.lat); // First, try shifting one world in either direction, and see if either is closer to the @@ -47,5 +48,11 @@ export function smartWrap(lngLat: LngLat, priorPos: Point, transform: Transform) } } - return lngLat; + // Apply the change only if new coord is below horizon + if (lngLat.lng !== originalLngLat.lng && + transform.locationPoint(lngLat).y > (transform.height / 2 - transform.getHorizon())) { + return lngLat; + } + + return originalLngLat; }