debounce rounding position on Marker #11167

malekeym · 2021-10-22T16:30:58Z

Launch Checklist

In this PR we debounce rounding position for cases that we need to animate Marker
Close #11135

src/ui/marker.js

SnailBones

The unit tests look great to me!

I just tested this out in browser, and unfortunately the bug still isn't fixed.
https://user-images.githubusercontent.com/14878684/141167735-a2739059-b954-409e-9082-e4cd513fb15a.mov

I think this is because of the map._forceMarkerUpdate(), which I added to fix another marker bug here. The intention was to have markers update in response to map.setTerrain(), but it seems to be calling excess updates as well. I think that fixing this will require checking if terrain actually changes on the call to painter.updateTerrain() and only updating the markers if so.
EDIT: I'm wrong, this isn't the cause of the bug.

malekeym · 2021-11-10T20:10:33Z

The unit tests look great to me!

I just tested this out in browser, and unfortunately the bug still isn't fixed. https://user-images.githubusercontent.com/14878684/141167735-a2739059-b954-409e-9082-e4cd513fb15a.mov

I think this is because of the map._forceMarkerUpdate(), which I added to fix another marker bug here. The intention was to have markers update in response to map.setTerrain(), but it seems to be calling excess updates as well. I think that fixing this will require checking if terrain actually changes on the call to painter.updateTerrain() and only updating the markers if so.

2021-11-10.23-27-53.mov

@SnailBones good point, I think I catch it. but when i test it in my browser all thing is fine.
could you provide a code that demonstrates this bug.

SnailBones · 2021-11-10T20:38:51Z

@SnailBones good point, I think I catch it. but when i test it in my browser all thing is fine. could you provide a code that demonstrates this bug.

I've copied the code from this example into a local html file, and tested in both Firefox and Chrome.

Your video unfortunately doesn't seem to be working for me.

malekeym · 2021-11-10T20:48:44Z

@SnailBones good point, I think I catch it. but when i test it in my browser all thing is fine. could you provide a code that demonstrates this bug.

I've copied the code from this example into a local html file, and tested in both Firefox and Chrome.

Your video unfortunately doesn't seem to be working for me.

actually i copied that code...
https://user-images.githubusercontent.com/70053533/141190959-299cad85-3bcc-4622-9787-6ef5c25e95e9.mov
Could you check this link?

SnailBones · 2021-11-10T20:54:33Z

actually i copied that code... https://user-images.githubusercontent.com/70053533/141190959-299cad85-3bcc-4622-9787-6ef5c25e95e9.mov Could you check this link?

Still not working for me. I see this:

mourner · 2021-11-10T20:59:13Z

@SnailBones here's a better format:

marker.mp4

SnailBones · 2021-11-10T21:04:48Z

Thanks for the conversion @mourner!
@malekeym It's hard for me to tell if the marker is snapping in that video. I've found that the bug is most visible when a marker is moving very slowly, like this:

Screen.Recording.2021-11-10.at.1.59.58.PM.mov

Here's the code I used to create this:

<!DOCTYPE html>
<html>
<head>
    <title>Marker snapping test</title>
    <meta charset='utf-8'>
    <link rel='stylesheet' href='../dist/mapbox-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>
<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
<script>

var map = new mapboxgl.Map({
    container: 'map',
    style: 'mapbox://styles/mapbox/streets-v11',
    zoom: 0
});

const marker = new mapboxgl.Marker({color: '#F84C4C'});

function animateMarker(timestamp) {
    const radius = 5;
    marker.setLngLat([
        Math.cos(timestamp / 1000) * radius,
        Math.sin(timestamp / 1000) * radius
    ]);
    marker.addTo(map);
    requestAnimationFrame(animateMarker);
}

requestAnimationFrame(animateMarker);

</script>
</body>
</html>

mourner · 2021-11-10T21:04:48Z

src/ui/marker.js

@@ -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));


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).

malekeym · 2021-11-10T21:27:52Z

Thanks for the conversion @mourner! @malekeym It's hard for me to tell if the marker is snapping in that video. I've found that the bug is most visible when a marker is moving very slowly, like this:

Screen.Recording.2021-11-10.at.1.59.58.PM.mov
Here's the code I used to create this:

<!DOCTYPE html>
<html>
<head>
    <title>Marker snapping test</title>
    <meta charset='utf-8'>
    <link rel='stylesheet' href='../dist/mapbox-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>
<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
<script>

var map = new mapboxgl.Map({
    container: 'map',
    style: 'mapbox://styles/mapbox/streets-v11',
    zoom: 0
});

const marker = new mapboxgl.Marker({color: '#F84C4C'});

function animateMarker(timestamp) {
    const radius = 5;
    marker.setLngLat([
        Math.cos(timestamp / 1000) * radius,
        Math.sin(timestamp / 1000) * radius
    ]);
    marker.addTo(map);
    requestAnimationFrame(animateMarker);
}

requestAnimationFrame(animateMarker);

</script>
</body>
</html>

Thank you @SnailBones , I check the code. if you see in function animateMarker call addTo after setLngLat so the result is that _pos is rounded, as we discussed before we decided to round position immediately in addTo and moveend. am i right?
please check this code

<!DOCTYPE html>
<html>
<head>
    <title>Marker snapping test</title>
    <meta charset='utf-8'>
    <link
      href="https://api.mapbox.com/mapbox-gl-js/v2.4.1/mapbox-gl.css"
      rel="stylesheet"
    />    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>
<script src='./mapbox-gl-unminified.js'></script>
<script>
      mapboxgl.accessToken =
        "ACCESS_TOKEN";
var map = new mapboxgl.Map({
    container: 'map',
    style: 'mapbox://styles/mapbox/streets-v11',
    zoom: 0
});

const marker = new mapboxgl.Marker({color: '#F84C4C'}).setLngLat([0,0]).addTo(map);

function animateMarker(timestamp) {
    const radius = 5;
    marker.setLngLat([
        Math.cos(timestamp / 10000) * radius,
        Math.sin(timestamp / 10000) * radius
    ]);
    requestAnimationFrame(animateMarker);
}

requestAnimationFrame(animateMarker);

</script>
</body>
</html>

SnailBones · 2021-11-11T02:01:33Z

Thank you @SnailBones , I check the code. if you see in function animateMarker call addTo after setLngLat so the result is that _pos is rounded, as we discussed before we decided to round position immediately in addTo and moveend. am i right?

Good catch @malekeym! That works for me, you're correct that the issue results from the repeated calls to marker.addTo. One last suggestion before I approve this: as a way to fix this specific case, can we add an early exit condition to the addTo()? Maybe something like:

        if (map === this._map) { return this; }

malekeym · 2021-11-11T16:44:49Z

Thank you @SnailBones , I check the code. if you see in function animateMarker call addTo after setLngLat so the result is that _pos is rounded, as we discussed before we decided to round position immediately in addTo and moveend. am i right?

Good catch @malekeym! That works for me, you're correct that the issue results from the repeated calls to marker.addTo. One last suggestion before I approve this: as a way to fix this specific case, can we add an early exit condition to the addTo()? Maybe something like:
        if (map === this._map) { return this; }

@SnailBones very good idea, i changed the code, but I find a bug that is related to _forceUpdateMarker :)) if you want to reproduce it, load terrain Map and just add a Marker then move the map you can see the marker is jagging. (this is not happening in street Map), could you help me to fix this?
I found the bug that is related to @mourner Review, I don't know why but when using _updateMoving the behaviour is changed on the move event. To demonstrate the issue first create a map then just add a Marker and move the map. Is it clear?

SnailBones · 2021-11-11T21:59:50Z

I found the bug that is related to @mourner Review, I don't know why but when using _updateMoving the behaviour is changed on the move event. To demonstrate the issue first create a map then just add a Marker and move the map. Is it clear?

I'm glad you noticed that issue @malekeym! The cause is that this is bound to the map in updateMoving, so it's actually calling map._update If you can find a way to correctly call marker._update the bug should be fixed.

add Unit test to check this

malekeym · 2021-11-12T06:49:59Z

I found the bug that is related to @mourner Review, I don't know why but when using _updateMoving the behaviour is changed on the move event. To demonstrate the issue first create a map then just add a Marker and move the map. Is it clear?

I'm glad you noticed that issue @malekeym! The cause is that this is bound to the map in updateMoving, so it's actually calling map._update If you can find a way to correctly call marker._update the bug should be fixed.

Thank you @SnailBones for finding the reason that bug happened I made changes and add a unit test to check move events and the issue is fixed but I have a problem with flow test, I can fix it with define property _updateMove instead of method and use Arrow function, or I should use wild card for disabling flow on reassigning Method. I think the first solution is better what do you think?

SnailBones · 2021-11-12T16:02:12Z

I think you have the right idea with assigning a property with an arrow function! Because besides the flow error, I think the current approach runs into the same issue that @mourner mentioned above because .bind(); creates a new method.

malekeym · 2021-11-12T17:00:08Z

I think you have the right idea with assigning a property with an arrow function! Because besides the flow error, I think the current approach runs into the same issue that @mourner mentioned above because .bind(); creates a new method.

I use the arrow function, but just one question, if we use bind and then assign the new function to a method, so the references are the same, so the @mourner issue is fixed, am I right?

SnailBones

Looks good to me!

malekeym · 2021-12-14T06:07:37Z

@SnailBones any blocker for this merge request? should I change anything?

mourner

This looks great to me now!

mourner · 2021-12-14T11:59:55Z

src/ui/marker.js

+            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}`);


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

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

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.

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

CLAassistant · 2021-12-14T12:52:48Z

All committers have signed the CLA.

malekeym · 2021-12-14T12:58:01Z

@SnailBones and @mourner , when I merge the main branch to my branch, I see that DOM is removed and used js for add style so I changed my code, too.

SnailBones · 2021-12-15T17:18:29Z

src/ui/marker.js

@@ -446,41 +452,59 @@ export default class Marker extends Evented {
            position.y >= 0 && position.y < tr.height;
    }

-    _update(e?: {type: 'move' | 'moveend'}) {
-        if (!this._map) return;
+    _updateDOM(pos: Point) {


Great work @malekeym!
One more tiny nit: Why does this function take in the position as an argument? Given it's directly accessing class variables like this._anchor, I think that it would be more consistent and prevent potential confusion to either remove this argument, or else pass in the other variables as arguments.

@SnailBones Done!

Looks great!

SnailBones · 2021-12-17T17:04:47Z

Merging it in, thanks for your work on this @malekeym! 🙏

debounce rounding position on Marker

0ae8f5e

malekeym mentioned this pull request Oct 22, 2021

Add isHighAccurate property to Marker #11135

Closed

SnailBones suggested changes Oct 22, 2021

View reviewed changes

src/ui/marker.js Outdated Show resolved Hide resolved

malekeym requested a review from SnailBones November 2, 2021 20:35

malekeym added 3 commits November 5, 2021 01:01

change _update Marker parameters to accepts delaySnap as a boolean

309e14a

test(Marker): add Marker test for rounding _pos

f0dd607

test(Marker): add unit test for move and setLngLat

dfb4113

SnailBones suggested changes Nov 10, 2021

View reviewed changes

mourner requested changes Nov 10, 2021

View reviewed changes

fix: function Refs in move events

a74e3a9

malekeym added 2 commits November 11, 2021 20:05

fix(Marker): ignore _update when addTo called when map not changed

b2c7c41

fix(Marker): check test for moveend

1d0dab6

fix(Marker): moveend should happend Immediatly

4c2b83c

fix(Marker): call Marker _update method on move event

1fc674a

add Unit test to check this

fix(Marker): use arrow function to access to Marker this

4c08403

SnailBones approved these changes Nov 12, 2021

View reviewed changes

SnailBones added the feature 🍏 label Nov 12, 2021

SnailBones requested a review from mourner November 15, 2021 19:13

mourner approved these changes Dec 14, 2021

View reviewed changes

Merge branch 'mapbox:main' into debounce-round-position

16fa227

refactor: remove DOM.setTransform

dc792c3

malekeym force-pushed the debounce-round-position branch from 4434ebf to dc792c3 Compare December 14, 2021 12:55

malekeym added 2 commits December 15, 2021 11:17

refactor(marker): split updateTransform to method

9fefbd3

refactor: seperate methods

1c1281c

SnailBones reviewed Dec 15, 2021

View reviewed changes

refactor(Marker): remove pos argument

7895068

SnailBones merged commit 5058720 into mapbox:main Dec 17, 2021

malekeym mentioned this pull request Dec 17, 2021

feat: add isHighAccurate property to Marker #11136

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debounce rounding position on Marker #11167

debounce rounding position on Marker #11167

malekeym commented Oct 22, 2021 •

edited by SnailBones

SnailBones left a comment •

edited

malekeym commented Nov 10, 2021

SnailBones commented Nov 10, 2021

malekeym commented Nov 10, 2021

SnailBones commented Nov 10, 2021

mourner commented Nov 10, 2021

SnailBones commented Nov 10, 2021 •

edited

mourner Nov 10, 2021

malekeym Nov 10, 2021

malekeym commented Nov 10, 2021 •

edited

SnailBones commented Nov 11, 2021

malekeym commented Nov 11, 2021 •

edited

SnailBones commented Nov 11, 2021

malekeym commented Nov 12, 2021

SnailBones commented Nov 12, 2021

malekeym commented Nov 12, 2021 •

edited

SnailBones left a comment

malekeym commented Dec 14, 2021

mourner left a comment

mourner Dec 14, 2021

malekeym Dec 14, 2021

SnailBones Dec 14, 2021 •

edited

malekeym Dec 15, 2021 •

edited

CLAassistant commented Dec 14, 2021 •

edited

malekeym commented Dec 14, 2021

SnailBones Dec 15, 2021

malekeym Dec 17, 2021

SnailBones Dec 17, 2021

SnailBones commented Dec 17, 2021

debounce rounding position on Marker #11167

debounce rounding position on Marker #11167

Conversation

malekeym commented Oct 22, 2021 • edited by SnailBones

Launch Checklist

SnailBones left a comment • edited

Choose a reason for hiding this comment

malekeym commented Nov 10, 2021

SnailBones commented Nov 10, 2021

malekeym commented Nov 10, 2021

SnailBones commented Nov 10, 2021

mourner commented Nov 10, 2021

SnailBones commented Nov 10, 2021 • edited

mourner Nov 10, 2021

Choose a reason for hiding this comment

malekeym Nov 10, 2021

Choose a reason for hiding this comment

malekeym commented Nov 10, 2021 • edited

SnailBones commented Nov 11, 2021

malekeym commented Nov 11, 2021 • edited

SnailBones commented Nov 11, 2021

malekeym commented Nov 12, 2021

SnailBones commented Nov 12, 2021

malekeym commented Nov 12, 2021 • edited

SnailBones left a comment

Choose a reason for hiding this comment

malekeym commented Dec 14, 2021

mourner left a comment

Choose a reason for hiding this comment

mourner Dec 14, 2021

Choose a reason for hiding this comment

malekeym Dec 14, 2021

Choose a reason for hiding this comment

SnailBones Dec 14, 2021 • edited

Choose a reason for hiding this comment

malekeym Dec 15, 2021 • edited

Choose a reason for hiding this comment

CLAassistant commented Dec 14, 2021 • edited

malekeym commented Dec 14, 2021

SnailBones Dec 15, 2021

Choose a reason for hiding this comment

malekeym Dec 17, 2021

Choose a reason for hiding this comment

SnailBones Dec 17, 2021

Choose a reason for hiding this comment

SnailBones commented Dec 17, 2021

malekeym commented Oct 22, 2021 •

edited by SnailBones

SnailBones left a comment •

edited

SnailBones commented Nov 10, 2021 •

edited

malekeym commented Nov 10, 2021 •

edited

malekeym commented Nov 11, 2021 •

edited

malekeym commented Nov 12, 2021 •

edited

SnailBones Dec 14, 2021 •

edited

malekeym Dec 15, 2021 •

edited

CLAassistant commented Dec 14, 2021 •

edited