-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this 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.
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 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.
I agree. I assume that will be out of scope for this PR. I believe the reasons for this are documented in #11717 (comment). |
@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 simplescreenrecorder-2024-10-21_15.39.28.mp4 |
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:
|
@ggetz thank you for the pair programming on this. recapping a little here.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/engine/Source/Scene/Fog.js
Outdated
this.density * | ||
this.fogScalar * | ||
Math.pow( | ||
Math.max(height / this.maxHeight, 1e-6), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Math.max(height / this.maxHeight, 1e-6), | |
Math.max(height / this.maxHeight, CesiumMath.EPSILON6), |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
@jjspace I did some quick network testing:
|
@ggetz I think I'm fine with those values, especially if it helps with the network cost. I was also thinking of switching the I'm still working through the tests, mostly value checking which these numbers but that should just be a simple value update. |
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 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 |
@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 |
Sure, please change it to whatever the equivalent was for the previous behavior. |
There was a problem hiding this 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.
@ggetz I believe this should now have all your comments addressed |
Looks good! Thanks @jjspace! |
Description
0.15
to0.4
which should make the densest fog possible right around where tiles are culled0.15
0.4
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
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.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
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change