-
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
11716 visualize fog #11744
11716 visualize fog #11744
Conversation
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.
Notes on things I still need to do
packages/engine/Source/Shaders/Builtin/Functions/getDynamicAtmosphereLightDirection.glsl
Outdated
Show resolved
Hide resolved
packages/engine/Source/Shaders/Builtin/Functions/computeScattering.glsl
Outdated
Show resolved
Hide resolved
packages/engine/Source/Shaders/Builtin/Functions/computeGroundAtmosphereScattering.glsl
Show resolved
Hide resolved
packages/engine/Source/Shaders/Builtin/Functions/computeEllipsoidPosition.glsl
Outdated
Show resolved
Hide resolved
* @param {vec3} mieColor The Mie scattering color computed by a scattering function | ||
* @param {float} opacity The opacity computed by a scattering function. | ||
*/ | ||
vec4 czm_computeAtmosphereColor( |
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.
Add a note to #11681 that this can be used in other shaders once the API change happens.
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.
Can we eliminate the code duplication even before the API change?
I know at one point you mentioned the Globe uses defines rather than a uniform. That seems like a change we could make internally without affecting the API.
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.
I certainly would like that but I need to examine how big of a change this would be. The problem is one version of these functions use uniforms set by a specific primitive (u_atmosphereWhatever
) and others use the new automatic uniforms (czm_atmosphereWhatever
). You'd need to tie the two together. I have an idea about how to do that, but I have to see if it's too big for this PR.
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.
#11681 (comment) -- here are more ideas of how to make this work during the deprecation period (in short, have the globe/sky hold a reference to the atmosphere whenever attached to a scene, and the old parameters will just update the new common one).
While certainly this could be done before the API deprecation, I think it's beyond the scope of this PR since it would add a decent chunk of code here. @ggetz would you agree?
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.
Let's try to keep the scope of this PR small if possible.
Thanks @ptrgags! Can/Should there be an API option to alter the fog falloff visually in order to bring it closer to the camera? |
@ggetz there already is, |
Today I've been comparing the different approaches to computing the ellipsoid position in the shader to see the visual differences and performance differences. Observations:
I tried the FPS monitor but it doesn't really have the granularity I need to distinguish the two methods (both hover at 59-60 FPS) . I'll see if there's another way to measure it. If there isn't a large perf difference, I'd rather use the iterative method since it better matches the ellipsoid model of the earth. |
I made a local Sandcastle to give a visual comparison of P3DT with terrain.
Notes:
|
Next, a side-by-side with Google Earth. To give a fairer comparison, I adjusted the following settings:
Sandcastle with updated settings
The thing that stands out to me is Google Earth's atmosphere effects have more pinks and purples when ours only has blue and white (with dynamic lighting off). Even our sunsets are more orange than the pinks and purples. |
I added the code to darken the lighting, now the colors match better between 3D Tiles and the globe: Perhaps due to different geometry and different methods for computing the ellipsoid position, 3D Tiles fog tends to go dark at slightly higher sun angles than terrain does: (but if I wasn't looking for such subtle differences I might not have noticed this. |
Thanks @ptrgags! Would you mind updating the Testing Plan section in the PR description when you get the chance? |
packages/engine/Specs/Scene/Model/AtmospherePipelineStageSpec.js
Outdated
Show resolved
Hide resolved
@ggetz I rounded up all the Sandcastles from the issue and this PR and put them in the description (+ some new ones!). I'll look into the test failure, I must have formatted my JSDoc incorrectly or referenced something incorrectly. |
@ggetz I've addressed your feedback, this is ready for review again. |
@jjhembd Would you mind taking a pass on the shaders in this PR? |
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 @ptrgags, the Model implementation looks good, and this also looks like a very nice cleanup of the atmosphere shaders.
My main question is: can we drop some of the old versions sooner, even before the API change suggested in #11681? Some of the smaller functions can be trivially consolidated (see comments below). It would be nice to do a bigger consolidation now if we can.
packages/engine/Source/Shaders/Builtin/Functions/applyHSBShift.glsl
Outdated
Show resolved
Hide resolved
* @param {vec3} mieColor The Mie scattering color computed by a scattering function | ||
* @param {float} opacity The opacity computed by a scattering function. | ||
*/ | ||
vec4 czm_computeAtmosphereColor( |
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.
Can we eliminate the code duplication even before the API change?
I know at one point you mentioned the Globe uses defines rather than a uniform. That seems like a change we could make internally without affecting the API.
@jjhembd Please merge once you are happy! |
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.
Looks good, thanks @ptrgags!
Description
While #11718 enabled and tuned the performance aspects of fog for 3D Tiles, this PR is about rendering the fog for 3D Tiles. This includes atmosphere color and dynamic lighting to make more realistic looking fog that accounts for Rayleigh and Mie scattering. The goal is to make world-scale 3D Tiles tilesets like Google Photorealistic 3D Tiles look more realistic like we do for terrain.
Local Sandcastle Example
To do this, it involves quite a few moving parts, as
Model
does not have access to the Scene, and the model isn't always added to the scene anyway.Atmosphere
class (scene.atmosphere
) that will hold the atmosphere related settings (see Proposal: Consolidate SkyAtmosphere and Globe atmosphere settings? #11681 for the longer-term plan which involves breaking changes to the API)FrameState
each frame (this is modeled after whatFog
does)UniformState
adds some new uniforms (czm_atmosphere*
) whose values come from the FrameState.FogPipelineStage
adds the shader code for enabling fogAll this together,
Issue number and link
Fixes #11716
Testing plan
For testing, I've been adjusting
Here's a roundup of the Sandcastles I used for testing:
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my changeGlobeVS
andGlobeFS
and see what other details I might have overlooked, as those shaders are rather involved.