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

Globe occlusion for popups and improving marker performance on globe #11658

Merged
merged 14 commits into from
Apr 26, 2022

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Mar 31, 2022

Launch Checklist

#11556 added support for marker and popup position on globe and marker occlusion behind globe. This PR adds occlusion for markers that are not attached to a popup.

Also improves the occlusion algorithm as described here: #11556 (review)

Also fixes a minor bug where a marker behind terrain and in fog would have its fog opacity calculated at the point of the intersecting terrain, not the marker itself. i.e. in some cases marker opacity would increase when the marker was hidden behind terrain.

Other changes are refactoring for code readability and removing repetition.

A future optimization could include caching the result to reduce calls to latLngToECEF, though this may not be worth the added complexity.

  • Globe occlusion for markeless popups.
  • Fog support + terrain occlusion for markerless popups.
  • 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
  • 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 markers in fog sometimes becoming more visible when behind terrain. </changelog>

@SnailBones SnailBones assigned SnailBones and unassigned SnailBones Apr 19, 2022
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 good @SnailBones ! I've left a few nits, but the one missing thing before merging is adding a small test for popup occlusion, otherwise good to merge at your convenience.

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 SnailBones merged commit 6b5ff72 into main Apr 26, 2022
@SnailBones SnailBones deleted the aidan/globe-marker-popup branch April 26, 2022 22:19
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.

2 participants