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

Fix displaced markers when toggling projections #12242

Merged
merged 14 commits into from Sep 27, 2022

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Sep 16, 2022

This PR fixes #12129, updating marker position correctly when toggling between a map that renders world copies and a map that doesn't by wrapping the lng of the marker when updating for projections that wrap.

BEFORE:

Screen.Recording.2022-09-16.at.3.04.36.PM.mov

AFTER:

Screen.Recording.2022-09-16.at.3.06.16.PM.mov

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
  • 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>Fix updating marker position when toggling between world copied projections and projections without</changelog>

@avpeery avpeery marked this pull request as ready for review September 16, 2022 22:10
@avpeery avpeery requested a review from a team as a code owner September 16, 2022 22:10
Copy link
Contributor

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@SnailBones SnailBones left a comment

Choose a reason for hiding this comment

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

Nice fix! This should also be applied to popups to keep consistency with markers.

Also, this changes behavior of dragging markers off the edge of the map when renderWorldCopies is false, resulting in markers remaining on the map area and looping instead of being dragged off the edge. I'm not 100% sure what the correct behavior is here, but should we consider snapping markers to the map edge?

marker-albers-before.mp4
markers-albers-after.mp4

@avpeery
Copy link
Contributor Author

avpeery commented Sep 20, 2022

Good points @SnailBones. I think it's acceptable/reasonable to not allow setting popups/markers off of the map, but also wouldn't want to change the behavior if there is a potential customers are using this (but I think that would be unlikely). Do you have opinions on this?

@SnailBones
Copy link
Contributor

SnailBones commented Sep 20, 2022

Good points @SnailBones. I think it's acceptable/reasonable to not allow setting popups/markers off of the map, but also wouldn't want to change the behavior if there is a potential customers are using this (but I think that would be unlikely). Do you have opinions on this?

Agreed, I think it's unlikely that many customers rely on markers outside of the map area, so in my opinion snapping would be the ideal behavior. But I'd also be happy with keeping the current behavior (i.e. only wrap on projection change).

@avpeery
Copy link
Contributor Author

avpeery commented Sep 20, 2022

@SnailBones Updated popups to wrap as well, and found that popups not attached to markers did not get positions updated when toggling projections, and found that popups not attached to markers were also not taking into account terrain exaggeration. I used the same implementation for markers (see: #10985) for popup updating.

Copy link
Contributor

@SnailBones SnailBones left a comment

Choose a reason for hiding this comment

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

Nit: Thoughts on naming the property options.wrap instead of options.setToWrap? Or just passing a boolean instead of an object? Or might we need to add more properties in the future?

Otherwise looks great to me and works great, nice work!

@avpeery
Copy link
Contributor Author

avpeery commented Sep 26, 2022

Nit: Thoughts on naming the property options.wrap instead of options.setToWrap? Or just passing a boolean instead of an object? Or might we need to add more properties in the future?

Naming suggestion noted, and will do! I don't think we'd probably expand on the object, but just thought passing through true would read confusing since it'll still be updating the markers/popups when not passing true in.

@avpeery avpeery merged commit d92c62e into main Sep 27, 2022
@avpeery avpeery deleted the avpeery/update-marker-position branch September 27, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marker position is not updated when changing from a projection with world copies to one without
3 participants