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

expose new scalar to change the visual density of fog #12248

Merged
merged 15 commits into from
Nov 1, 2024
Merged

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Oct 10, 2024

Description

  • Add a new property to adjust the visual density of fog relative to the "actual" density which also affects culling
  • change the default visual scalar from 0.15 to 0.4 which should make the densest fog possible right around where tiles are culled
Old default 0.15 New default 0.4
download (41) download (42)
download (43) download (44)
Density function details

These graphs show the density function values. The y value between 0 and 1 is used to determine the percentage for mixing the original color and the fog color. mix(originalColor, fogColor, percent)
ie. closer to 1 means more fog color

Any tile with a value greater than 1 is culled and thus not rendered
Any tile or model with a value less than 0.001 does not get any fog color at all to aid in performance, the fog would be negligible.

The red line is the "true" fog function that is calculated without any scalar. This is the one used for culling
The purple line is the fog function with the modifier included. The purple area is where fog is actually being displayed

This is the before value with a modifier of 0.15

2024-10-10_12-07

This is the same functions with a modifier of 0.4. As you can see the fog (purple) much closer matches reaching 1 around the same time the red fog function does. This means it's visually most dense at around the same distance tiles are culled.

2024-10-10_12-07_1

The density is cranked up way higher than normal to make seeing the curves clearer
Desmos link if you want to play with the values yourself
(the v value is there to exaggerate the graphs visually so you can still see the curves even if you stretch the density out really far)

Issue number and link

Part of #12126

Testing plan

  • Open the updated Fog sandcastle and play around with the visual density scalar value
    • Modify the actual density with different scalar values to see how they affect each other.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz October 10, 2024 16:13
Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Oct 11, 2024

This is the same functions with a modifier of 0.4. As you can see the fog (purple) much closer matches reaching 1 around the same time the red fog function does. This means it's visually most dense at around the same distance tiles are culled.

Good explanation! So for the default, we wouldn't want to go any less that 0.4 from the sound of it.

I was playing around with GE as a point of comparison, and their fog appears much denser, even than visual density equivalent of 1.0 in our system (which I think is the maximum, based on the function visualizations). What are your thoughts?

image

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @jjspace. A few comments on the code.

Please also add a unit test and update CHANGES.md.

packages/engine/Source/Core/Math.js Outdated Show resolved Hide resolved
packages/engine/Source/Renderer/AutomaticUniforms.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
@jjspace
Copy link
Contributor Author

jjspace commented Oct 11, 2024

I was playing around with GE as a point of comparison, and their fog appears much denser, even than visual density equivalent of 1.0 in our system (which I think is the maximum, based on the function visualizations). What are your thoughts?

I actually think the density of our fog is about the same. But ours is much darker. Maybe this has to do with some of the atmosphere values?

This is approximately the same location in CesiumJS (with 0.4) and Google Earth. (I just noticed GE seems to put a touch of a fish eye effect on the fov?)
2024-10-11_16-26
2024-10-11_16-27

This is that same view with the visualDensityScalar set to 1.0. I think this looks much more dense than GE's so i'm surprised to hear you say the opposite. does this match with what you saw?
2024-10-11_16-27_1

Edit (immediate follow up...):
I did just notice our fog seems inconsistently applied as we zoom in and out and very close to the ground it's not applied at all which I think might be the cause of your previous comment about it not being as dense as GE. They seem to apply fog consistently across all zoom levels. Not sure the cause for this.

simplescreenrecorder-2024-10-11_16.33.51.mp4
simplescreenrecorder-2024-10-11_16.36.30.mp4

@ggetz
Copy link
Contributor

ggetz commented Oct 14, 2024

I did just notice our fog seems inconsistently applied as we zoom in and out and very close to the ground it's not applied at all which I think might be the cause of your previous comment about it not being as dense as GE. They seem to apply fog consistently across all zoom levels. Not sure the cause for this.

Yes, I was looking at a horizon few similar to this one. This is what prompted my question. I agree that from your more "aerial" screenshots, the density does read correctly at a value of 0.4.

My guess for what is causing this may be the atmosphere scattering code returning transparent colors for fog when close to the ground. Perhaps try stubbing out the atmosphere color function with one that returns a solid color for the sake of debugging. Once we identify the cause, we can figure out an implementation plan, and if it's scope.

If that's not the case, my shot in the dark is that this may be related to #11922. The horizon angle is what's jumping out to me as similar.

But ours is much darker. Maybe this has to do with some of the atmosphere values?

I agree. I assume that will be out of scope for this PR. I believe the reasons for this are documented in #11717 (comment).

CHANGES.md Outdated Show resolved Hide resolved
@jjspace
Copy link
Contributor Author

jjspace commented Oct 21, 2024

@ggetz I fixed up the couple docs issues and moved the fog sandcastle as discussed offline.

That said I'm still confused what's causing the fog to be basically non-existant close to the ground and have such a massive spike in density at a certain height. I tried swapping the fog color out for a static magenta 1, 0, 1 instead of the atmosphere color and still observe the same issues. I'm thinking it might have something to do with the depth/distance calculations? I made a slightly modified sandcastle to easily reproduce the zoom.

simplescreenrecorder-2024-10-21_15.39.28.mp4

@ggetz
Copy link
Contributor

ggetz commented Oct 22, 2024

That said I'm still confused what's causing the fog to be basically non-existant close to the ground and have such a massive spike in density at a certain height. I tried swapping the fog color out for a static magenta 1, 0, 1 instead of the atmosphere color and still observe the same issues. I'm thinking it might have something to do with the depth/distance calculations?

Hmm, I see it as well. I wonder if we need to scale based on the camera height. For instance, here's an exponential decay function based on camera height which seems to be producing decent results:

const float maxHeight = 10000.0;
const float minHeight = 500.0;
float height = (czm_eyeHeight - minHeight) / (maxHeight - minHeight);
float heightModifier = 1.0 + exp(-decay * height);

const float modifier = 0.4;
finalColor = vec4(czm_fog(v_distance * heightModifier, finalColor.rgb, fogColor.rgb, modifier), finalColor.a);

@jjspace
Copy link
Contributor Author

jjspace commented Oct 23, 2024

@ggetz thank you for the pair programming on this. recapping a little here.

  • Ever since fog was initially implemented it's used an internal table of heights and densities at those heights that seems to be somewhat arbitrary. These were not a fully smooth curve and were the cause of the weird "spikes" in density and practically no fog near the ground that we were observing.
    • Desmos graph with those values showing inconsistent densities across heights and some test curves to fit (messy but wanted to record somewhere)
  • Those tables have been replaced with a smooth power function that somewhat closely matches the previous values.
  • The "magic numbers" in that new function have been exposed as a set of new properties on the Fog class to allow customization. We should have good defaults so they don't normally have to be altered but the option is there.
  • With this latest change I also updated the visualDensityScalar up to 1 from 0.4 which seems to have a nicer effect. We may even want to default to slightly higher.

I think the function and implementation is good now. The sandcastle has been updated with the new properties. Please play with them a little and see if you think there's a better set of default values, changing the numbers is easy.
I'll fix up the tests tomorrow as long as we agree on this approach.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @jjspace! I'm glad this all cleaned up nicely!

Do you think we can say this addresses #8454?

In addition to updating tests, please make sure new or properties are reflected in CHANGES.md.

I'm testing out behavior now.

packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
this.density *
this.fogScalar *
Math.pow(
Math.max(height / this.maxHeight, 1e-6),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Math.max(height / this.maxHeight, 1e-6),
Math.max(height / this.maxHeight, CesiumMath.EPSILON6),

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when this.maxHeight is 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens when this.maxHeight is 0?

It functionally turns off fog unless you're underground because of the check at the beginning of this function for if you're in space. If you're under height 0 the max value kicks in here and the divide by 0 doesn't seem to be a problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually I agree! In practice though, I think there is a case where, if maxHeight is 0 and height happens to be 0, we can get a value of NaN. Can we prevent that?

packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
@ggetz
Copy link
Contributor

ggetz commented Oct 25, 2024

@jjspace I did some quick network testing:

  • On average, the current settings are using around 1.5MB network for horizon views at various heights
  • To get the same network usage as in main, I used the following settings:
viewer.scene.fog.density = 0.001;
viewer.scene.fog.heightFalloff = 0.65;
  • What do you think of splitting the difference? This is, on average, using 0.4MB more network, and I'm still seeing increased resolution at the mid-range and closer to the camera.
viewer.scene.fog.density = 0.0006;
viewer.scene.fog.heightFalloff = 0.59;

@jjspace
Copy link
Contributor Author

jjspace commented Oct 25, 2024

@ggetz I think I'm fine with those values, especially if it helps with the network cost.

I was also thinking of switching the visualDensityScalar back down to 0.5 or 0.6 instead of 1 as I think 1 gets a bit too aggressivly dense at higher altitudes. Thoughts?

I'm still working through the tests, mostly value checking which these numbers but that should just be a simple value update.

@ggetz
Copy link
Contributor

ggetz commented Oct 28, 2024

I was also thinking of switching the visualDensityScalar back down to 0.5 or 0.6 instead of 1 as I think 1 gets a bit too aggressively dense at higher altitudes. Thoughts?

I did some digging into why fog is so dark, and I think that the opacity of the ground atmosphere color is not being correctly handled. If we go with something similar to the fix I have there, I think 1 works, as the fog closer to the camera will actually be pretty thin and therefor the color more transparent. However, I doubt we could get that fix in before the next release.

If possible, I'd like to avoid breaking changes for this default. Maybe a weird idea, but what do you think of marking this value as experimental for the first release in case we want to adjust it after the fact?

@jjspace
Copy link
Contributor Author

jjspace commented Oct 28, 2024

@ggetz I've added tests and marked the new scalar as experimental. I think that's fine for now but I also thought that if we want to update this in the future we could switch it back to 0.15 like it was before this PR to minimize impact until we get your other fix in?

@ggetz
Copy link
Contributor

ggetz commented Oct 28, 2024

I think that's fine for now but I also thought that if we want to update this in the future we could switch it back to 0.15 like it was before this PR to minimize impact until we get your other fix in?

Sure, please change it to whatever the equivalent was for the previous behavior.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @jjspace! This is super close!

I've added tests and marked the new scalar as experimental. I think that's fine for now but I also thought that if we want to update this in the future we could switch it back to 0.15 like it was before this PR to minimize impact until we get your other fix in?

I might have misunderstood you, but comparing main to this branch, I actually think a a value of 0.5 approximates the existing behavior better than 0.15 does. I think this is due to the changes made to the density itself.

packages/engine/Specs/Scene/FogSpec.js Show resolved Hide resolved
packages/engine/Specs/Scene/SceneSpec.js Outdated Show resolved Hide resolved
packages/engine/Specs/Scene/FogSpec.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Fog.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Fog.js Show resolved Hide resolved
@jjspace
Copy link
Contributor Author

jjspace commented Oct 30, 2024

@ggetz I believe this should now have all your comments addressed

@ggetz
Copy link
Contributor

ggetz commented Nov 1, 2024

Looks good! Thanks @jjspace!

@ggetz ggetz merged commit f21a07f into main Nov 1, 2024
9 checks passed
@ggetz ggetz deleted the visual-fog-density branch November 1, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants