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 fog sky blending #10578

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Conversation

rreusser
Copy link
Contributor

This PR restores a small fix which was lost along the way, in which terrain that sits above the horizon at high pitch is still obscured by fog, even though it stands out in stark contrast with the unobscured sky. It does this by multiplying fog by the fog sky blending function. Haze doesn't seem to require this change.

The reason we'd cut this out before was that it didn't play well with culling so that blank terrain showed through the fog. I'm not currently able to find problematic spots, though that doesn't mean we don't still need to consider whether this is valid.

Before: http://localhost:9966/debug/fog.html#15.06/-44.66578/167.90699/-53.6/84

Screen Shot 2021-04-15 at 9 28 53 AM copy

After:

Screen Shot 2021-04-15 at 9 29 06 AM copy

@rreusser rreusser requested a review from karimnaaji April 15, 2021 16:38
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.

Thinking about this more, I'm a bit concerned about going with this approach:

  • Implicitly using sky-blend as a height fog control may prevent us from hosting a dedicated height fog system as they could hardly co-exist and leads to ambiguity. If you could achieve a similar look with the use of sky-blend to approximate this effect, how do you control actual elevation-based height fog: Are they mutually exclusive? Which one overrules the other?
  • What happens to symbols going higher than this blend factor? Should they be shown/clipped?
  • Height fog may have use cases beyond pitch > 65 degrees. For example, I could imagine it being lightly used (with an opacity control) on non-pitched views to allow for better depth perception of top-down terrain.
  • How does haze affect fragments over the range of sky-blend? Should it also dissipate as height increases? e.g. in this sort of situation:

Screen Shot 2021-04-15 at 4 23 08 PM

  • Should the sky blend still affect the terrain when no sky is available?
sky-blend-2.mov

Due to this ambiguity, I think height fog would really deserve more thoughts and not be something tightly coupled with distance fog in any way (even with a minor approximation like here).

Although it's true that we may have a discrepancy between foreground/background, I still consider this acceptable as far fog ranges would not immediately show this type of issue (and very close-up fog should be considered an edge case). It might be best to leave this open-ended to not restrain the possibilities of a dedicated system.

@rreusser
Copy link
Contributor Author

rreusser commented Apr 18, 2021

That makes sense, though it's too bad since it seems moderately visible at high pitch+high zoom. I think map designers would be well advised to either favor haze or significantly increase sky-blend at high zoom (and, once possible, at high pitch).

Tall mountains near sea level seem to be the worst, e.g.: http://localhost:9966/debug/fog.html#15.92/-44.665341/167.914518/-43/85

@rreusser rreusser force-pushed the ricky/fog-sky-blending-fix branch from 2bf347d to 91444e6 Compare April 18, 2021 19:34
@rreusser
Copy link
Contributor Author

rreusser commented Apr 18, 2021

After working with this and trying out height fog a bit, I don't think I agree that it would conflict with dedicated height fog, and I think the visibility of this artifact is actually pretty high even with low amounts of fog. Height fog seems significantly different in its mechanics, so that I don't see it being a simple addition which suddenly falls in conflict with existing fog. The number one argument in my opinion though is that I don't think we should skip a small fix for a high-visibility artifact due to the possibility that it might conflict with a feature (though I don't think it would) we don't currently have any intent of implementing, and which we could trivially disable if height fog addressed it instead.

Here is the image at the top again, with fog range [5, 15], which is quite minimal. It's definitely the worst close to sea level.

Screen Shot 2021-04-18 at 3-1 13 15 PM copy

Here's the same region zoomed out, to illustrate that it's quite a minimal amount of fog:

Screen Shot 2021-04-18 at 3 16 48 PM copy

Regarding the specific points:

Implicitly using sky-blend as a height fog control may prevent us from hosting a dedicated height fog system as they could hardly co-exist and leads to ambiguity. If you could achieve a similar look with the use of sky-blend to approximate this effect, how do you control actual elevation-based height fog: Are they mutually exclusive? Which one overrules the other?

I don't see the ambiguity since if height fog is active and addressed it in its own way, then presumably we would simply disable this fix. I'd also reiterate that I don't think we will implement height fog on any reasonable timeline due to the challenges in estimating the local baseline altitude. I think the sky-blend layer is in a similar situation in the sense that if height fog obviates the need for it, then we'd have to come up with a reasonable solution like layering it under height fog.

What happens to symbols going higher than this blend factor? Should they be shown/clipped?

For a start, I would fade symbols with the strictly-distance-based fog. I don't see a particular reason why symbol fading needs to exactly match fog, though we could certainly try either approach with minimal effort.

Height fog may have use cases beyond pitch > 65 degrees. For example, I could imagine it being lightly used (with an opacity control) on non-pitched views to allow for better depth perception of top-down terrain.

I agree, though I don't think that's related to this fix.

How does haze affect fragments over the range of sky-blend? Should it also dissipate as height increases? e.g. in this sort of situation

I would apply haze just the same. In my experimentation, it looks reasonable and is physically justifiable. To the best of my understanding, haze is the result of light-molecule interactions which presumably vary with air density, so that Mount Everest would still only maybe show a 50% variation in haze from sea level, which is almost certainly negligible compared to other liberties taken with our atmospheric model.

@arindam1993
Copy link
Contributor

With regards to symbols and fog with this, it shouldn't be much of an issue as long as the CPU side fog code can account for sample points height + the fog-sky blend factor.

And regarding this being conflicting with future height fog implementation, honestly we can just rename fog-skt-blend to fog-height with this change, its a much more simper name to understand and in the future we can unlock a larger range for fog-height to allow for below horizon fog heights as well.

@@ -14,6 +15,12 @@ float fog_opacity(float t) {
return u_fog_opacity * min(1.0, 1.00747 * falloff);
}

// This function much match fog_sky_blending defined in _prelude_fog.fragment.glsl
float fog_sky_blending(vec3 camera_dir) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need account for this in CPU side code as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think either direction is defensible, but yeah, I agree, @arindam1993. I've updated the CPU side code to include this.

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.

Thanks for giving it another look. A few more questions:

  • Does sky-blend still make sense as a name? It was originally meant to be a horizon-based value, to clarify more on the meaning should it be named differently to better reflect this latest change? elevation-blend, height-blend, horizon-blend? Better naming would also resolve one question I've previously mentioned, as it removes the implicit dependency:

Should the sky blend still affect the terrain when no sky is available?

If we go ahead, I think we should also update the documentation to be more clear about the elevation detail, as I don't think we're mentioning that yet.

  • Should we leave 0 as a minimal bound? After testing, 0 seems a bit aggressive visually, using 0.05 as a minimum bound seems to prevent that potential horizon band (basically only allowing a minimum smoothing effect on top of terrain).

Screen Shot 2021-04-19 at 3 10 15 PM

@rreusser
Copy link
Contributor Author

rreusser commented Apr 19, 2021

Does sky-blend still make sense as a name?

I wouldn't call it elevation-blend or height-blend since it has no actual dependence on elevation or height, but I think horizon-blend is a pretty good name. 👍

Should the sky blend still affect the terrain when no sky is available?

I think so, yes. You can always set sky/horizon-blend = 1.0 (the max?) Given a fovy of 36 degrees, I think the maximum angle you can see in the sky is something like 31 degrees, so that the blending equation looks like this for sky-blend=1.0:

Screen Shot 2021-04-19 at 4 15 45 PM

The point being, you can more or less disable the effect when sky is off by just setting sky-blend to 1.0. For reference, here's fog in what should be perfectly whiteout conditions, with sky-blend set to 1.0. We could always allow sky-blend to go up to 2 or 3.

Screen Shot 2021-04-19 at 4 39 37 PM copy

  • Should we leave 0 as a minimal bound?

0.05 seems alright. If you really want no sky-blend you can set it to a transparent color, right?

@rreusser rreusser force-pushed the ricky/fog-sky-blending-fix branch from 2ef9db4 to 889caa2 Compare April 20, 2021 18:36
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.

Thanks for following up on that, the few things we discussed to follow up after this PR:

  • Update docs to reflect the new elevation detail introduced by this change
  • Better naming of sky-blend to reflect this change (not tied anymore to sky only)
  • Revise default minimum bound to prevent eventual harsh horizontal band (we could address this one in a note in the documentation for very low values, or prevent them altogether)

@karimnaaji karimnaaji dismissed arindam1993’s stale review April 20, 2021 19:14

Addressed in latest changes.

@karimnaaji karimnaaji merged commit 673559b into fog-implementation Apr 20, 2021
@karimnaaji karimnaaji deleted the ricky/fog-sky-blending-fix branch April 20, 2021 19:14
@rreusser
Copy link
Contributor Author

rreusser commented Apr 20, 2021

Here is horizon-blend = 0.02:

Screen Shot 2021-04-20 at 12 16 38 PM

At low zoom, it seems reasonable to allow a pretty sharp transition, so I'd maybe hesitate to disallow small values. I think best is to simply have zero disable the effect on terrain, and to explain the risks of very small values in the docs.

karimnaaji pushed a commit that referenced this pull request Apr 20, 2021
* Clean up fog sky blending

* Add fog sky blending fix to cpu side
karimnaaji added a commit that referenced this pull request Apr 23, 2021
karimnaaji pushed a commit that referenced this pull request Apr 23, 2021
* Clean up fog sky blending

* Add fog sky blending fix to cpu side
karimnaaji added a commit that referenced this pull request Apr 23, 2021
karimnaaji pushed a commit that referenced this pull request May 4, 2021
* Clean up fog sky blending

* Add fog sky blending fix to cpu side
karimnaaji added a commit that referenced this pull request May 4, 2021
karimnaaji pushed a commit that referenced this pull request May 4, 2021
* Clean up fog sky blending

* Add fog sky blending fix to cpu side
karimnaaji added a commit that referenced this pull request May 4, 2021
karimnaaji pushed a commit that referenced this pull request May 4, 2021
* Clean up fog sky blending

* Add fog sky blending fix to cpu side

Fix post rebase flow
karimnaaji added a commit that referenced this pull request May 4, 2021
karimnaaji pushed a commit that referenced this pull request May 6, 2021
* Clean up fog sky blending

* Add fog sky blending fix to cpu side

Fix post rebase flow
karimnaaji added a commit that referenced this pull request May 6, 2021
karimnaaji added a commit that referenced this pull request May 7, 2021
* Add fog stylespec

* Add fog type specification

* Add fog stylespec validation

Lint

* Add setFog() and getFog() APIs

* Add debug page

* Add note to validate fog

* Add Fog preludes and inject preprocessor defines when enabled

Fix Lint

Fix Flow

drop: Test Fog define

* Inject shader preludes and connect to globally defined fog uniforms

Fixup

* Apply fog to sky layers

Remove unnecessary shader statement

* Apply fog to terrain render

* Fix fog on sky gradient layers

* Apply fog to fill-extrusion layer

* Apply fog to fill layer

* Apply fog to fill-extrusion-pattern layer

Fixup shader statement

* Apply fog to fill-outline, fill-outline-pattern, fill-pattern layers

* Apply fog to background layer

* rename posMatrix to projMatrix

* Add cameraMatrix

* add separate cameraMatrix

* Calculate worldsize for cameraMatrix based on camera distance distance to sealevel

Fix flow and lint errors

* Simplify shader statement, remove extra division by w
u_cam_matrix is not projective and w is always 1 for points

* Add fading in of fog around 60 deg pitch

Fix lint errors

* Improve fog demo example and add GUI controls

* Fog tile culling in tile cover

Fixup comment

* Remove extraneous mult

Remove unused

* Simplify fog tile culling

Fix lint and flow

* Apply correct fog+blending in shaders (#10496)

* Make an attempt to implement consistent fog logic

* Add comments

* Apply premultiplied fix across shaders

* Clean up gamma correction

* Simply GLSL defines and fix shader compile error

* Fix syntax error and remove unused gamma correction

* Add fog to circles

* Apply fog to background pattern and hillshade

* Clean up a small shader optimization

* Add dithering to fog (#10506)

* Add basic dithering to fog

* Add temporal offset for fog

* Make uniform naming consistent

* Code golf the shaders just a little

* Separate dithering from fog function

* Rework the fog falloff function (#10515)

* Rework the fog falloff function

* Rework the fog function

* Add some spec validation

* Support fog on markers (#10516)

* Apply fog to Markers

* Fix lint flow and cleanup

* Fix opacity not updating for terrain and fog after updating the spec properties

* Add map.getFogOpacity API

* Code style

* Evaluate opacity less often

* Update per rebased changes

* Minor tweak

* Update src/style/fog.js

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/ui/marker.js

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/ui/map.js

Co-authored-by: Ricky Reusser <[email protected]>

* Update variable name per review

* Fixup invalid commit from github

* Address review comments

* Update per review

Co-authored-by: Ricky Reusser <[email protected]>

Fix popup tip not being occluded

* Fog-sky blending (#10527)

* Improve fog blending

* Clean up blending

* Remove fog-sky blending fix

Fix tests and low

* Do not cull tiles crossing over the horizon during fog culling phase
This prevents sudden pop in and out for tiles within the fog culling range

* Use fog color as default context clear color when enabled
This prevents unaffected fragments resulting in empty holes in location
where we do not draw fragments

* Cull at 80% of the distance between fog start end fog end
This leaves a non-noticeable 2% fog opacity change threshold
and reduces the distance before we start culling

* Add style diffing support and unit tests fog setFog and getFog

* Cutoff fog-opacity from the style specification

* Fix a bug using stale tile cover result due missing update to fog ranges
This issue was visible when updating fog properties and an incorrect tile
cover run would potentially leave invalid tiles on the viewport

* Switch fog to use window-scaled units (#10533)

* Switch fog to use window-scaled units

* Update style spec for fog range

* Update fog validation tests to match new limits

* Update fog validations

* Add haziness property to fog (#10537)

* Add haziness to fog

* Fix flow types

* Apply PR feedback

* Update marker opacity to use viewport-scaled height (#10542)

* Update marker opacity to use viewport-scaled height

* add units to variable names to make intent clear

* Code cleanup: Simplify getDistanceToSeaLevel + remove extraneous transform variable

* Add unit tests for furthestTileCorner

* Add unit tests for coveringTiles with fog culling

* Add basic symbol clipping when fog opacity is high (#10541)

* Rename cameraMatrix --> fogMatrix (#10544)

* Offload terrain fog to vertex shader (#10549)

* Offload terrain fog to vertex shader

* Don't set haze uniforms unless haze is present

* Remove haze varying unless haze is used

* Disable fog code path on the 2d case

* Remove redundant uniforms

* Simplify fog render logic

Co-authored-by: Karim Naaji <[email protected]>

* Add documentation of the public facing fog APIs + revise defaults (#10545)

* Add documentation of the public facing APIs

* Fixup missing `}`

* Update fog range docs

* Disable haze by default and fix mixed up haze properties

* Update src/style-spec/reference/v8.json

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/style-spec/reference/v8.json

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/style-spec/reference/v8.json

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/style-spec/reference/v8.json

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/ui/map.js

Co-authored-by: Ricky Reusser <[email protected]>

Co-authored-by: Ricky Reusser <[email protected]>

* Extract getAABBPointSquareDist from fog culling and add unit tests

* Use implicit vec3.transformMat4 division by w instead of explicit one

* Add fog as part of the release testing

* Add fog culling render test

* Add sky-blend render tests

* Add more style diffing unit tests and coverage around setFog/getFog public APIs

* Add unit tests for getFogOpacity

* Add basic terrain render tests

* Add fog marker occlusion unit tests

Remove extraneous note

* Add 2D tests for fog (#10566)

* Add 2D render tests for fog

* Test fog shaders

* Swap raster test

* Add half float heatmap render test image

* Increase test allowance for raster test

* Add more fog style spec render test coverage

* Fix shader preprocessor define leading to invalid shaders on production build variants (#10582)

* Prevent shader stripping issue on production builds
Always use same function signature independently of shader variant

* Use const for the unused param

* Directly use inlined parameter

* Fixup

* Fix render tests
Cannot use const as 'out' or 'inout' parameters.

* Fog style spec changes (#10590)

* Clean up fog style properties

* Present a more reasonable example

* Fix fog tests and validations

* Improve haze wording

* Apply review feedback: getFogOpacity -> queryFogOpacity

* Apply review feedback: Keep seperation of DOM-based and non-DOM based map element updates
Defer marker opacity update in DOM task queue

* Fix fog sky blending (#10578)

* Clean up fog sky blending

* Add fog sky blending fix to cpu side

Fix post rebase flow

* Better consistency in naming conventions

* Apply review feedback: Remove extraneous function call

* Apply review feedback: Rename sky-blend -> horizon-blend

* Apply review feedback: Increase range of pitch fading to be less aggressive

* Simplify haze evaluation (#10606)

* Simplify fog evaluation

* Switch to approximate gamma conversion

* Factor out haze application

* Document the haze_apply function

* Remove backticks

* Update src/shaders/_prelude_fog.fragment.glsl

* Apply review feedback: Consolidate shader code that can be shared in shared prelude

* Remove haze (#10613)

* Remove haze

* Remove density

* Catch a couple more small cleanup items

* Fix broken shader

* Tighten/restore fog culling

* Update src/render/painter.js

* Revert fog opacity regression from #10578 with CPU side evaluation of horizon blend

* Demo: Update default location

* Revise comments and cleanup shader code for readability
- Also removes extraneous branching with min in fog_apply_premultiplied

* Share fog varying globally in new shader prelude

* Revise comments and naming

* Code cleanup for better readability

* Fix validation error on complex style expression of fog color validation

* Fix validation error on complex style expression of fog range validation and add validation tests

* Simplify fog validation checks for expression(s)

* Switch fog from percentage window height to CSS vh units (#10632)

* Switch fog from percentage window height to CSS vh units

* Fix tests

* Fix more tests

* Fix still more tests

* Update src/style-spec/reference/v8.json

* Don't leave alpha of fog color unused (#10633)

* Don't leave alpha of fog color unused

* Apply review comments: Simplify flow, reduce uniforms

* Fix naming

* Update docs

* Fix flow

* Update all render tests to use new units

* Fixup consistent naming

* Add symbol clipping render test

* Sample terrain to estimate de facto camera scale (#10621)

* Continuously adjust the fog scale by raycasting

* Remove unused function

* Fix linter errors

* Name thresholds for clarity

* Hard-code sampling pattern

* Rearrange conditional

* Adjust sampling by horizon line

* Replace arbitrary constant with epsilon

* Remove stray import

* Simplify logic

* Only calc fog matrices on avg elevation change

* Fix cloning of transform

* Fix bug in state management

* Fix linter errors

* Remove debug code

* Move samples points inside function

* Fix accidental call to calcMatrices

* We still love you, flow

* Clarify code

* Add tests for EasedVariable

* Add one more test

* Fix default per latest changes and add render tests

* Fix alpha composing of horizon blend with sky gradient (#10640)

Make sure to un/post multiply alpha before/after applying sky gradient
contribution, as color ramps are premultiplied-alpha colors.  This prevents
washed out colors when using transparency.

* Account for FOV in fog (#10629)

* Account for FOV in fog

* Fix a typo

* Fix flow errors

* Fix a factor of two to make tests pass

* Fix bad merge

* Fix tests

* Shift fog range zero to the map center

* Fix debug

* Fix ranges to account for fov

* Remove unused variable

* Apply review feedback: Directly update marker style opacity

* Update per latest discussion: Use nameless units to prevent confusion
when trying to reason about them. As the units of the range are representing
3d distances on a sphere centered around camera center point, they can
hardly be reasoned about in terms of window height.

* Fixup naming consistency

* Fix race condition in fog terrain sampling (#10642)

* Fix race condition in fog terrain sampling

* Use frame start time stamp

* Update render test expectation

* Remove fog vh units (#10648)

* Remove fog vh units

* Fix floating point errors in test expectations

* Fix geo test

* Add fog demo and transition fixes (#10649)

* Skip unnecessary uniform code path when terrain is rendering to texture
On a typical frame with terrain this was called ~200 times, where only
20 were necessary (~10%)

* Fix invalid transition evaluation
When `delay` was undefined but not `now`, properties.begin and properties.end
were assigned incorrect values, leading to an immediate transition interpolation.
This was noticed when testing transitions on fog.

* Fix style.hasTransition() when fog is transitioning values

* Add a fog demo for release

* Directly use DOM queue rencently introduced by #10530

* Apply review feedback: Directly clear the timer here instead of unsetting it

* Apply review feedback: Fix nits, mark queryFogOpacity private until we
need to expose it publicly

* Update src/util/util.js

Co-authored-by: Ricky Reusser <[email protected]>

* Update src/util/util.js

Co-authored-by: Ricky Reusser <[email protected]>

* Apply review feedback: Fix nits

* Apply review feedback: Simplify matrix initialization

* make private methods private for docs

Co-authored-by: Arindam Bose <[email protected]>
Co-authored-by: Ricky Reusser <[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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants