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

Adding rotationAlignment: 'horizon' for markers on globe #11894

Merged
merged 55 commits into from May 18, 2022

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented May 13, 2022

Takeaway from the discussion in #11642 is that a globe-oriented rotation alignment property is useful (but not a pitch alignment), and the clearest name for the property is "horizon." This PR adds that feature accordingly.

const marker = new mapboxgl.Marker({
      rotationAlignment: 'horizon',
      pitchAlignment: 'viewport' // This line can also be left out.
  })

image

As the map zooms in and transitions from globe to Mercator, markers smoothly transition into rotationAlignment: viewport behavior.

marker-transition.mov

With custom pin markers:

pin-demoo.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
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Adding new marker styling option: rotationAlignment: 'horizon' allowing marker rotation to match the curvature of the horizon in globe view</changelog>

cc @mapbox/map-design-team

SnailBones and others added 30 commits February 25, 2022 09:32
… pitch-alignment:map in globe without change to 2d behavior
Co-authored-by: Karim Naaji <karim.naaji@gmail.com>
@SnailBones SnailBones changed the title Marker pitchAlignment: 'horizon' Adding rotationAlignment: 'horizon' for markers on globe May 13, 2022
@SnailBones SnailBones force-pushed the aidan/marker-horizon-alignment branch from 0d4af8e to ab55eca Compare May 13, 2022 23:16
@SnailBones SnailBones marked this pull request as ready for review May 13, 2022 23:16
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Nice addition! This will be a good feature to land for 2.9.1 🎉

I have a few stylistic comments regarding the newly added code. Otherwise, I haven't been able to locally test markers-custom, it seems like the page doesn't show the markers (only popups):

Screen Shot 2022-05-16 at 12 53 31 PM

But it worked great and the other page markers

src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/geo/projection/globe_util.js Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
@SnailBones
Copy link
Contributor Author

I have a few stylistic comments regarding the newly added code.

@karimnaaji I realized there was still a bit of vestigial logic accounting for the experimental horizon pitch alignment in #1642, so I've been able to simplify the code in a few places. It now seems that adding athis._enableHorizonAlignment() function wouldn't need to be used more than once, otherwise I've addressed your concerns.

Otherwise, I haven't been able to locally test markers-custom, it seems like the page doesn't show the markers (only popups):

Whoops, forgot to commit the custom icon! 🤦 That's now fixed.

src/ui/map.js Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
Comment on lines -123 to -124
const defaultHeight = 41;
const defaultWidth = 27;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why move these out? I'd prefer locality, so the values aren't few hundreds of code lines away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was that this makes them less magic-numbery and more consistent with the other constants defined at the start of the file, but that's a good point. Do you think that ALIGN_TO_HORIZON_BELOW_ZOOM and ALIGN_TO_HORIZON_ABOVE_ZOOM should also be defined closer to where they are used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think that ALIGN_TO_HORIZON_BELOW_ZOOM and ALIGN_TO_HORIZON_ABOVE_ZOOM should also be defined closer to where they are used?

I think so, since they're not really shared by anything else and not exported for use outside of this file, it's probably good to keep it local to the block they're used in as well instead of in the global scope. Being consistent in naming would also make sense if you keep here, e.g. making these two variables all caps similar to the other ones.

src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Looks great! One last nit to address in #11894 (comment) either by making the naming consistent or moving it back to local scope, but otherwise good to merge.

It would be great once 2.9.1 gets released to update the public documentation with the example from markers-custom, so that we have better visibility for that option. Could you open a ticket so we follow up on that?

@SnailBones SnailBones merged commit 2ad1134 into main May 18, 2022
@SnailBones SnailBones deleted the aidan/marker-horizon-alignment branch May 18, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants