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.

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

Add isHighAccurate property to Marker #11135

Closed
malekeym opened this issue Oct 19, 2021 · 10 comments · Fixed by #11167
Closed

Add isHighAccurate property to Marker #11135

malekeym opened this issue Oct 19, 2021 · 10 comments · Fixed by #11167

Comments

@malekeym
Copy link
Contributor

Motivation

For animating a point the example that Mapbox provided is animate point along a route. This convention has bad performance and triggers a repaint map on every update source.
image
as you can see The CPU is involved throughout the airplane movement.

If we use Marker instead of symbol layer the result is very different, because it works with CSS and doesn't trigger a repaint.
image
As you can see this convention uses less CPU and memory, but if you check this you can see Codesandbox link that movement is not smooth and sometimes the marker shakes. This behavior happens because round marker._pos property of Marker(maybe for performance) but if we want to implement animation movement, we should have highAccuracy.

Implementation

We can add isHighAccuracy property and check if we want high accurate marker don't round marker._pos.

@SnailBones
Copy link
Contributor

SnailBones commented Oct 19, 2021

Thanks for the issue @malekeym!

This is happening because markers automatically snap to the pixel grid. This creates a crisp appearance when they're stationary, but when moving markers (or when moving the map as in #8614) this jittering behavior appears.

We have a stale PR that should provide a fix for this issue.

@malekeym
Copy link
Contributor Author

Thanks for your attention @SnailBones
but I think we have a misunderstanding here :)
We can animate a Marker on Map in two ways,

  • add Marker using Symbol layer and use setData method...(like Mapbox Example)
  • add Marker using Marker class and use setLngLat from Marker class(Example)

the first implementation involves CPU all the time when Animation running, so it's not a good idea to use this. (check the first profiling picture)

the second implementation uses CSS to set Marker on the right lngLat and less more CPU and memory(check the second profiling)

but if we use Marker as you can see in the example the Marker shakes, because in _update method of Marker class we round position
image
in this #11136 I add the flag that check that the marker should be high accurate and if true don't round position.
If something is not clear, tell me to clarify that.

@SnailBones
Copy link
Contributor

You're right, I now realize the PR I linked is for the Symbol layer "Markers" and not for HTML Markers (which as you demonstrate are much more performant option for this use case)!

Thank you for your contribution! Leaving some more comments on your PR.

@SnailBones
Copy link
Contributor

@malekeym I just noticed that we do have an example of animating a HTML Marker. The jittering behavior is clear in it, especially at lower zoom levels.

@mourner
Copy link
Member

mourner commented Oct 22, 2021

An alternative solution to introducing an option would be to always NOT round on an update, but set a debounce-like timeout that rounds the position after the marker is settled (not changing position for X ms). This might be a compromise between both use cases (keeping the icon sharp and no jitter), with the possible drawback of noticeable movement on the timeout.

@malekeym
Copy link
Contributor Author

malekeym commented Oct 22, 2021

An alternative solution to introducing an option would be to always NOT round on an update, but set a debounce-like timeout that rounds the position after the marker is settled (not changing position for X ms). This might be a compromise between both use cases (keeping the icon sharp and no jitter), with the possible drawback of noticeable movement on the timeout.

Thank you @mourner. I think with your solution we call _update one more time, but a good idea. @SnailBones do you think if we debounce rounding _pos and _update, issues that are talked about in #11136 are fixed?

@malekeym
Copy link
Contributor Author

malekeym commented Oct 22, 2021

@SnailBones @ryanhamley @mourner
We have 3 ways to fix this issue.

I think the first one can fix the issue but @SnailBones said:

That would disable snapping on Marker.setLngLat, which would mean we lose the advantages of snapping in cases where the marker is moved on user interaction (geocoder and draggable marker).

I think this isn't really an issue because snapToPixel is Marker behavior, if someone sets snapToPixel to false the result that we expect is The Marker should be never snap to pixel. and if someone needs snapToPixel set this property to true. and we have setter and getter for this property that completely cover cases that we want to change this behavior.

The second suggestion can be good, but we should investigate all cases that _update call, for example, if we call Marker.setLngLat(coords, {snapToPixel:false}) then moveend event fired the result is that _pos rounded.

The third suggestion is a good idea, but if we want to debounce some actions maybe we should use setTimeout and something like that, I'm not really sure, but I think this action takes the process in the background, and for solving this tiny issue isn't the best way. ( If I'm wrong let me know ).

I want to know your idea, which one do you prefer?

@SnailBones
Copy link
Contributor

Could we use requestAnimationFrame to update the marker exactly one frame after it moves and only if the position hasn't changed in that time?

@mourner
Copy link
Member

mourner commented Oct 22, 2021

I think this action takes the process in the background, and for solving this tiny issue isn't the best way. ( If I'm wrong let me know ).

It will still be a minor change without performance impact, especially with @SnailBones's suggestion to use requestAnimationFrame — one frame might be enough to catch all the animating use cases.

My main reason for proposing this option is to avoid expanding the API surface for something that can be an internal detail users don't have to think about. If we can make it to just work without any user-side changes for both cases, that's ideal.

@malekeym
Copy link
Contributor Author

@SnailBones @mourner
Ok, I submit a new PR that debounce rounding _pos with requestAnimationFrame. If something is wrong let me know to fix that. This is new PR #11167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants