Skip to content

Commit

Permalink
Fix marker flying off near horizon (#3704)
Browse files Browse the repository at this point in the history
* Pass flat Marker/Popup position to smartWrap

* Shift marker/popup by one globe only if such point is below horizon

* Fix popup test

* Clone original lngLat in smartWrap

* Increase timout in marker tests

* Add changelog entry

* Add test for smartWrap
  • Loading branch information
sbachinin authored Feb 17, 2024
1 parent 3bea9a1 commit b9023bc
Show file tree
Hide file tree
Showing 7 changed files with 132 additions and 15 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions src/ui/marker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand All @@ -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();
});
Expand Down Expand Up @@ -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');

Expand All @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand Down
9 changes: 7 additions & 2 deletions src/ui/marker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export class Marker extends Evented {
_popup: Popup;
_lngLat: LngLat;
_pos: Point;
_flatPos: Point;
_color: string;
_scale: number;
_defaultMarker: boolean;
Expand Down Expand Up @@ -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') {
Expand Down
6 changes: 4 additions & 2 deletions src/ui/popup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
9 changes: 7 additions & 2 deletions src/ui/popup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ export class Popup extends Evented {
_lngLat: LngLat;
_trackPointer: boolean;
_pos: Point;
_flatPos: Point;

constructor(options?: PopupOptions) {
super();
Expand Down Expand Up @@ -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);
Expand Down
97 changes: 97 additions & 0 deletions src/util/smart_wrap.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
9 changes: 8 additions & 1 deletion src/util/smart_wrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

0 comments on commit b9023bc

Please sign in to comment.