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 atmosphere + Style specification #11590

Merged
merged 41 commits into from
Apr 13, 2022
Merged

Globe atmosphere + Style specification #11590

merged 41 commits into from
Apr 13, 2022

Conversation

karimnaaji
Copy link
Contributor

@karimnaaji karimnaaji commented Mar 11, 2022

Atmosphere style specification

This PR extends the fog system to allow design of an atmosphere effect supported on both globe and mercator projections. It extends the fog style specification to account for three new values: high-color, space-color and star-intensity. The star-intensity allows for a parallax effect while rotating and panning the globe to increase the effect of 3d.

Webp net-gifmaker

Changelist

  • Add an atmosphere debug page with different preset to test the feature
  • Add support for fog on g lobe with a fixed range, the fixed range value is chosen arbitrarily to give plausible rendering of a thin atmospheric layer over the globe.
  • Consolidate frustum corner calculation and share it between atmosphere rendering and fog uniform upload
  • Split atmosphere buffer from GlobeSharedBuffer so that it's not tied to globe rendering
  • Remove clear color potentially using fog color, as this shader now covers the entire screen, it is unnecessary to dispatch an expensive clear
  • Make sure that globe fixed fog range does not affect rendering of symbols and fog tile culling
  • Make sure that heatmaps are not affected by globe fog fixed range, so that this layer can be rendered without being affected by fog opacity, as we get flickering artifacts otherwise
  • Update render test baselines to account for the new changes
  • Add style specification defaults using complex expressions so that map.setFog({}) results in a nice default for the user
  • Add a fake cubemap in the shader in order to create the parallax effect for stars
fake-cubemap.mov

Style specification entries (including old ones):

  • color alpha channel

color-alpha

  • color rgb channels

color

  • horizon-blend

horizon-blend

  • high-color alpha channel

sky-color-alpha

  • high-color rgb channels

sky-color

  • space-color alpha channel

space-color-alpha

  • space-color rgb channels

space-color

  • star-intensity

star-intensity

Follow up to address after this PR

  • Reduce frequency of globe specific uniforms when updating fog uniform, these values can be 1) calculated once per frame and 2) only be done when used along with globe
  • Fix horizon angle line visual artifact, refer https://github.com/mapbox/gl-js-team/issues/410
  • Add render tests specific to globe atmosphere
  • Fix fill-extrusion fog contribution during transition

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

Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox left a comment

Choose a reason for hiding this comment

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

Looks really good! Are we supposed to have a default preset available when switching to globe view? Rest of the debug pages (e.g. terrain-debug.html) have no atmosphere set.

debug/atmosphere.html Show resolved Hide resolved
src/style/fog.js Outdated Show resolved Hide resolved
src/geo/projection/globe.js Outdated Show resolved Hide resolved
src/util/primitives.js Outdated Show resolved Hide resolved
src/util/primitives.js Show resolved Hide resolved
src/render/painter.js Show resolved Hide resolved
src/style/properties.js Show resolved Hide resolved
src/shaders/atmosphere.fragment.glsl Show resolved Hide resolved
@karimnaaji
Copy link
Contributor Author

karimnaaji commented Apr 12, 2022

Are we supposed to have a default preset available when switching to globe view?

No, there's no dependency between globe-view and atmosphere at the spec level. e.g. when setting globe-view we don't automatically enable the atmosphere. To improve UX, this is something to consider fixing at a higher level in Studio/when setting default styling, so that we're not adding dependencies between different style specification entries.

Rest of the debug pages (e.g. terrain-debug.html) have no atmosphere set.

I've updated most of the pages in latest to use the default atmosphere.

There are two follow-ups to this PR that I've captured in other tickets:

globe-fill-extrusion.mov

Otherwise please take another look as this is ready for another review, I think I've addressed all of the other comments.

// exponential curve
// [0.0, 1.0] == inside the globe, > 1.0 == outside of the globe
// https://www.desmos.com/calculator/l5v8lw9zby
float t = exp(-horizon_angle * pow(u_fadeout_range, -1.0));
Copy link
Contributor

Choose a reason for hiding this comment

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

is this an optimization to write pow(x, -1) instead of 1/x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an optimization, that's an overlook from converting the function from desmos, thanks for pointing that out I'll update it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I switched that function to prefer exp over an sqrt based function, so that the effect is smoother, you can notice that the sqrt based function that we used in main (in red) results in an abrupt change when crossing y=0 due to clamping:

Screenshot from 2022-04-13 11-29-45

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I noticed! That is a better option 👍

Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox left a comment

Choose a reason for hiding this comment

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

LGTM!

@karimnaaji karimnaaji merged commit 744367a into main Apr 13, 2022
@karimnaaji karimnaaji deleted the karim/globe-fog branch April 13, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3d 📐 feature 🍏 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