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

Do not render atmosphere when fog isn't supported by the map projection #11947

Merged
merged 1 commit into from
May 31, 2022

Conversation

karimnaaji
Copy link
Contributor

Small fix for issue reported by map-design, when fog isn't supported by the projection we should not render the atmosphere part of it in the background.

cc @tristen

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
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@karimnaaji karimnaaji requested a review from avpeery May 31, 2022 21:30
@karimnaaji karimnaaji added the skip changelog Used for PRs that do not need a changelog entry label May 31, 2022
Copy link
Contributor

@avpeery avpeery 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! One thing I noticed that when I call map.getFog(); on a map with albers projections and made a map.setFog({}); call, I get values for the fog properties instead of null. When I don't call map.setFog({});, null is returned. I think I'd expect the value to still be null if the projection doesn't support fog?

@karimnaaji
Copy link
Contributor Author

@avpeery That's a good point. This behavior was adopted during the projection implementation which behaves the same as with other unsupported root properties such as terrain at the moment (e.g. terrain isn't supported by projections, but setting terrain with non-mercator projection as no effect and the map will still return it with a call to map.getTerrain).

We could decide to return null for unsupported root properties even when they have been set, as long as we have consistency across root properties in that regard, but that hasn't been an issue or suggested behavior. So in my opinion, it's ok to keep it this way and not introduce a breaking change until we have an external request on that.

@karimnaaji karimnaaji merged commit b573850 into main May 31, 2022
@karimnaaji karimnaaji deleted the karim/unsupported-fog-projection branch May 31, 2022 22:46
karimnaaji added a commit that referenced this pull request Jun 8, 2022
* Add fog render test for empty update parameters (#11900)

* Add precision qualifiers to globe_raster fragment shader (#11903)

* Add filter features within map view with globe release test (#11888)

* add filter features within map view with globe release test

* Fix incorrect circle bucket up direction (#11904)

* Fix for https://github.com/mapbox/mapbox-gl-native-internal/issues/3426

* Update baselines

* ScaleControl: add i18n support (#11850)

* ScaleControl: add i18n support

* hardcode nautical miles as `nm` for all locales

* Add minimum/maximum fields for center/parallels (#11912)

* Add new render test for globe antialiasing (#11933)

* Fix potential flickering on image source updates (#11928)

* Fix flickering of image source on reload

* Add unit tests

* Fix setStyle breaking with globe view (#11913)

* set draping for terrain

* use map option for terrain draping check

* move to one line

* wip: add render test

* fixed error with render test

* demonstrate issue with mercator

* fix background id to update in second setStyle call

* bring zoom out to show sphere

* Updating expected.png

* better readability

* fix conditional

* fix lint

Co-authored-by: Aidan H <[email protected]>

* Fix tiles not rendering across antimeridian in globe (#11943)

* Add video to globe release tests (#11929)

* add ozone video on globe to release tests

* formatting

* formatting

* fix lint issues

* simplify play/pause button

* added atmosphere

* Do not render atmosphere when fog isn't supported by the projection (#11947)

* Clone stylesheet to allow toggling different styles with `setStyle` (#11942)

* clone stylesheet instead of manipulating it directly

* style should not contain terrain since it is no longer being overwritten

* add unit test

* add in default values for terrain when updating

* Add test for projections

* unit test for terrain exaggeration does not need sources

* should have two calls to setStyle to fully test toggling

* default value does not need to be added if previous terrain style also used the default value

* hasOwnProperty is already a boolean

* no need to check currSpec because checks for deep equals and 1 is already set for default

* fix unit tests

* fix style setting in unit test

* accidental deletion

* switch to deep clone from extend

* better way to find default value

* use hasOwnProperty instead

* convert to false from undefined

* create reusable function for setting transition parameters

* update _setTransitionParameters

* mix up between duraton/delay values

* reduce repetitive code to update style spec and transitions between terrain and fog

* revert change

* minimize scale changes when panning in globe view (#11951)

* adjust scale at low zoom levels in globe view

* improve

* Adding render test catching missing tiles with minzoom

* Allowing lower zoom level tiles toward the poles

* Fix lint

* Updating render tests to pass

* Updating tolerances to fix failing tests

* Updating one more tolerance

* Fixing query tests

* use GLOBE_ZOOM_THRESHOLD_MIN

* rename to mercatorScaleRatio

* add comment about matching scale

* avoid recentering on terrain if rendering globe

* update unit tests

* lint

Co-authored-by: Aidan H <[email protected]>

* Do not set the map language to the user's preferred language by default (#11952)

* Do not set the map language to the user's preferred language by default

* fix unit test

* cleanup

* cleanup

* fix worldview setter

Co-authored-by: Mikko Pulkki <[email protected]>
Co-authored-by: Anna Peery <[email protected]>
Co-authored-by: Stepan Kuzmin <[email protected]>
Co-authored-by: Tristen Brown <[email protected]>
Co-authored-by: Aidan H <[email protected]>
Co-authored-by: Ansis Brammanis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants