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.

Sign up for GitHub

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

debounce rounding position on Marker #11167

Merged
merged 15 commits into from
Dec 17, 2021
Merged

debounce rounding position on Marker #11167

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0ae8f5e
debounce rounding position on Marker
malekeym Oct 22, 2021
309e14a
change _update Marker parameters to accepts delaySnap as a boolean
malekeym Nov 4, 2021
f0dd607
test(Marker): add Marker test for rounding _pos
malekeym Nov 6, 2021
dfb4113
test(Marker): add unit test for move and setLngLat
malekeym Nov 10, 2021
a74e3a9
fix: function Refs in move events
malekeym Nov 10, 2021
b2c7c41
fix(Marker): ignore _update when addTo called when map not changed
malekeym Nov 11, 2021
1d0dab6
fix(Marker): check test for moveend
malekeym Nov 11, 2021
4c2b83c
fix(Marker): moveend should happend Immediatly
malekeym Nov 11, 2021
1fc674a
fix(Marker): call Marker _update method on move event
malekeym Nov 12, 2021
4c08403
fix(Marker): use arrow function to access to Marker this
malekeym Nov 12, 2021
16fa227
Merge branch 'mapbox:main' into debounce-round-position
malekeym Dec 14, 2021
dc792c3
refactor: remove DOM.setTransform
malekeym Dec 14, 2021
9fefbd3
refactor(marker): split updateTransform to method
malekeym Dec 15, 2021
1c1281c
refactor: seperate methods
malekeym Dec 15, 2021
7895068
refactor(Marker): remove pos argument
malekeym Dec 17, 2021
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
19 changes: 14 additions & 5 deletions src/ui/marker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export default class Marker extends Evented {
_rotationAlignment: string;
_originalTabIndex: ?string; // original tabindex of _element
_fadeTimer: ?TimeoutID;
_updateFrameId: number;

constructor(options?: Options, legacyOptions?: Options) {
super();
Expand Down Expand Up @@ -256,7 +257,7 @@ export default class Marker extends Evented {
this.remove();
this._map = map;
map.getCanvasContainer().appendChild(this._element);
map.on('move', this._update);
map.on('move', () => this._update(true));
map.on('moveend', this._update);
map.on('remove', this._clearFadeTimer);
map._addMarker(this);
Expand All @@ -282,7 +283,7 @@ export default class Marker extends Evented {
remove() {
if (this._map) {
this._map.off('click', this._onMapClick);
this._map.off('move', this._update);
this._map.off('move', () => this._update(true));
Copy link
Member

Choose a reason for hiding this comment

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

This won't actually remove the listener that was added previously, because this is a different function. You need to pass the exact reference — e.g. by creating a separate _updateMoving that internally calls _update(true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

this._map.off('moveend', this._update);
this._map.off('mousedown', this._addDragHandler);
this._map.off('touchstart', this._addDragHandler);
Expand Down Expand Up @@ -337,7 +338,7 @@ export default class Marker extends Evented {
this._lngLat = LngLat.convert(lnglat);
this._pos = null;
if (this._popup) this._popup.setLngLat(this._lngLat);
this._update();
this._update(true);
return this;
}

Expand Down Expand Up @@ -507,7 +508,8 @@ export default class Marker extends Evented {
position.y >= 0 && position.y < tr.height;
}

_update(e?: {type: 'move' | 'moveend'}) {
_update(delaySnap?: boolean) {
window.cancelAnimationFrame(this._updateFrameId);
if (!this._map) return;

if (this._map.transform.renderWorldCopies) {
Expand All @@ -533,7 +535,14 @@ export default class Marker extends Evented {
// because rounding the coordinates at every `move` event causes stuttered zooming
// we only round them when _update is called with `moveend` or when its called with
// no arguments (when the Marker is initialized or Marker#setLngLat is invoked).
if (!e || e.type === "moveend") {
if (delaySnap) {
this._updateFrameId = window.requestAnimationFrame(() => {
if (this._element && this._pos && this._anchor) {
this._pos = this._pos.round();
DOM.setTransform(this._element, `${anchorTranslate[this._anchor]} translate(${this._pos.x}px, ${this._pos.y}px) ${pitch} ${rotation}`);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I wonder if we could deduplicate this long line of code that's duplicated below, but not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't have any idea, do you have?

Copy link
Contributor

@SnailBones SnailBones Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe seperate that line into something like:

function _updateDOM(){   DOM.setTransform(this._element, `${anchorTranslate[this._anchor]} translate(${this._pos.x}px, ${this._pos.y}px) ${pitch} ${rotation}`);}

If you move the pitch and location calculating code into this function, then it can live in the marker's scope.

Copy link
Contributor Author

@malekeym malekeym Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I separate some methods, I appreciate reviewing changes and giving your feedback

}
});
} else {
this._pos = this._pos.round();
}

Expand Down
41 changes: 41 additions & 0 deletions test/unit/ui/marker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {createMap as globalCreateMap} from '../../util/index.js';
import Marker from '../../../src/ui/marker.js';
import Popup from '../../../src/ui/popup.js';
import LngLat from '../../../src/geo/lng_lat.js';
import {Event} from '../../../src/util/evented.js';
import Point from '@mapbox/point-geometry';
import simulate from '../../util/simulate_interaction.js';

Expand Down Expand Up @@ -925,3 +926,43 @@ test('Marker and fog', (t) => {
});
});
});

test('Snap To Pixel', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([1, 2])
.addTo(map);
t.test("Snap To Pixel immediately after initializing marker", (t) => {
t.same(marker._pos, marker._pos.round());
t.end();
});
t.test("Not Immediately Snap To Pixel After setLngLat", (t) => {
marker.setLngLat([2, 1]);
const pos = marker._pos;
setTimeout(() => {
t.notSame(marker._pos, pos);
t.same(marker._pos, pos.round());
t.end();
}, 100);
});
t.test("Not Immediately Snap To Pixel when Map move and Snap To Pixel on moveend", (t) => {
map.fire(new Event("move"));
t.notSame(marker._pos, marker._pos.round());
map.fire(new Event("moveend"));
window.requestAnimationFrame(() => {
t.same(marker._pos, marker._pos.round());
t.end();
});
});
t.test("Not Immediately Snap To Pixel when Map move and setLngLat", (t) => {
marker.setLngLat([1, 2]);
map.fire(new Event("move"));
t.notSame(marker._pos, marker._pos.round());
setTimeout(() => {
t.same(marker._pos, marker._pos.round());
t.end();
}, 100);
});
map.remove();
t.end();
});