feat: add isHighAccurate property to Marker

[malekeym]

Add isHighAccurate property to Marker. for disable rounding marker._pos
close #11135

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

[CLAassistant]

All committers have signed the CLA.

[SnailBones]

Thanks for the contribution @malekym!

One small piece of feedback so far: I don't think isHighAccurate is the most descriptive name for this property. Perhaps snapToPixel would give a more clear description of the behavior?

[SnailBones]

I think this should close #8614, but this will need to be verified.

[malekeym]

Thanks for the contribution @malekym!

One small piece of feedback so far: I don't think isHighAccurate is the most descriptive name for this property. Perhaps snapToPixel would give a more clear description of the behavior?

@SnailBones
You are right isHighAccurate isn't clear, i changed that to snapToPixel. Sometimes choosing a correct name is more difficult than writing codes. :))

[behnammodi]

@SnailBones Also can we have it on 1.x.x and 2.x.x? I mean release on two major version

[ryanhamley]

Can you say what your use case is for this? It seems like a very small, subtle change and I'm not sure it justifies expanding the API with a new option and two new methods

Nevermind. I see that this is in response to #11135

[SnailBones]

@ryanhamley As @malekeym describes in #11135, the use case is to create smoother animated markers. The current jitter is demonstrated in this demo.

I wonder if there's a way to address this without expanding the API - perhaps specifically disabling snapping on recently-moved markers?

[ryanhamley]

Thanks for taking this on @malekeym. Besides the comments I left, this also needs to add snapToPixel (or whatever it ends up being called) to the documentation by adding it as a @param in the options documentation in lines 33-47. We should also be able to add some unit tests that compare the marker's position with and without this new property being set to ensure we're getting the expected outcome.

[ryanhamley]

We want to call this._update() after updating snapToPixel as in all the other setter methods. We should also handle the undefined condition here: this._snapToPixel = !!shouldSnapToPixel;.

[malekeym]

Done!

[malekeym]

@ryanbaumann
I think calling _update when changing snapToPixel is redundant because _pos doesn't change so if call _update nothing changed. Am I right? let me know to remove calling _update on setSnapToPixel.

[SnailBones]

I think that setSnapToPixel(true) should update the position so that the snap can be immediately perceived by the user.

[ryanhamley]

I wonder if there's a way to address this without expanding the API - perhaps specifically disabling snapping on recently-moved markers?

In line 536, we can adjust it from if (!e || e.type === "moveend") to if (e && e.type === "moveend"). I'm trying to figure out if there's a use case where that would be an issue, but I'm not really coming up with much.

[SnailBones]

In line 536, we can adjust it from if (!e || e.type === "moveend") to if (e && e.type === "moveend"). I'm trying to figure out if there's a use case where that would be an issue, but I'm not really coming up with much.

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

Ideally, I think we want the Marker to snap on discrete position changes or when it finishes moving, but not during ongoing animations. I don't see any easy way to do that automatically without adding an extra _update() call.

@malekeym @ryanhamley As an alternative to a class property, what about an optional argument to Marker.setLngLat()? I think that would provide the same functionality while avoiding extra update calls and user complexity required by setSnapToPixel().

[malekeym]

In line 536, we can adjust it from if (!e || e.type === "moveend") to if (e && e.type === "moveend"). I'm trying to figure out if there's a use case where that would be an issue, but I'm not really coming up with much.

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

Ideally, I think we want the Marker to snap on discrete position changes or when it finishes moving, but not during ongoing animations. I don't see any easy way to do that automatically without adding an extra _update() call.

@malekeym @ryanhamley As an alternative to a class property, what about an optional argument to Marker.setLngLat()? I think that would provide the same functionality while avoiding extra update calls and user complexity required by setSnapToPixel().

Maybe use an optional parameter in setLngLat is better but we have some issues. the current implementation that round position is in line 541 that is _update method but your suggestion is to change setLngLat. consider this case:
someone use setLngLat with snapToPixel set to true so we don't round _pos, then use setRotation in the result the marker _pos is rounded.
we can solve this problem in this way:

• get snapToPixel as optional in all methods that call _update and pass that to _update

Is it clear? Do you think it's a good idea?

[SnailBones]

Maybe use an optional parameter in setLngLat is better but we have some issues. the current implementation that round position is in line 541 that is _update method but your suggestion is to change setLngLat. consider this case:
someone use setLngLat with snapToPixel set to true so we don't round _pos, then use setRotation in the result the marker _pos is rounded.
we can solve this problem in this way:

get snapToPixel as optional in all methods that call _update and pass that to _update

Is it clear? Do you think it's a good idea?

Good point @malekeym!

One option would be to avoid updating _pos on setRotation, maybe by moving the modifying code into a separate function: updatePos(snapToPixel: boolean). This function would be called on map updates and changes to LngLat, rotationAlignment and pitchAlignment. You could include an extra argument insetPitchAlignment and setRotationAligment but these functions are unlikely to be called frequently and cause a sudden change in popup appearance anyway, so I think that they could always snap.

Does that make sense? What do you think?

[malekeym]

Maybe use an optional parameter in setLngLat is better but we have some issues. the current implementation that round position is in line 541 that is _update method but your suggestion is to change setLngLat. consider this case:
someone use setLngLat with snapToPixel set to true so we don't round _pos, then use setRotation in the result the marker _pos is rounded.
we can solve this problem in this way:

get snapToPixel as optional in all methods that call _update and pass that to _update

Is it clear? Do you think it's a good idea?

Good point @malekeym!

One option would be to avoid updating _pos on setRotation, maybe by moving the modifying code into a separate function: updatePos(snapToPixel: boolean). This function would be called on map updates and changes to LngLat, rotationAlignment and pitchAlignment. You could include an extra argument insetPitchAlignment and setRotationAligment but these functions are unlikely to be called frequently and cause a sudden change in popup appearance anyway, so I think that they could always snap.

Does that make sense? What do you think?

I catch you, you mean to split _update method into two methods, one update _pos named updatePos and call updatePos in _update. and give snapToPixel option for some methods like setLngLat or setPitchAlignment or setRotationAlignment so we call updatePos(snapToPixel:boolean = true) with defatult value true but it can change by calling and pass snapToPixel (correct me if I'm wrong).
_update is triggered when the 'move' event is fired. the result is the _pos is rounded. I don't know it's good or not.
If you think it's the better approach I can change the code.

[malekeym]

@SnailBones with merging this #11167 I close this PR, Thank you!