-
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
Procedural IBL Enhancement in CesiumJS #12129
Conversation
Thank you for the pull request, @ggetz! ✅ We can confirm we have a CLA on file for you. |
if (defined(uniformMap)) { | ||
const manualUniforms = this._manualUniforms; | ||
len = manualUniforms.length; | ||
for (i = 0; i < len; ++i) { | ||
const mu = manualUniforms[i]; | ||
mu.value = uniformMap[mu.name](); | ||
try { |
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.
This update is not strictly necessary, but I found it helpful for debugging.
@jjspace Could you please take a review pass on this? |
@ggetz Couple initial comments playing with the new sandcastle.
|
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.
Had a handful of really small, mostly optional, code comments in addition to the comments above
I don't know if I feel fully qualified to review the shaders but they look ok to me at a cursory glance
if (!this._environmentMapManager.isDestroyed()) { | ||
this._environmentMapManager.destroy(); | ||
} |
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.
No change needed but curious about this pattern.
It feels like the responsibility for this check should live in the destroy
function itself. I feel like you should be able to just call destroy()
regardless and let it handle checking if it's already destroyed and essentially turn into a noop function. Then in this function the logic would just be "We know we want to guarantee this is destroyed so call destroy()
" and no wrapper check for if it's already destroyed?
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.
Hm, something to consider. See destroyObject.js
for the implementation of this pattern. I'll leave it up to you whether you want to write up a new issue for discussion.
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.
It's a minor thing but I opened #12266 to discuss
packages/engine/Source/Scene/Model/ImageBasedLightingPipelineStage.js
Outdated
Show resolved
Hide resolved
packages/engine/Source/Shaders/Builtin/Functions/computeAtmosphereColor.glsl
Outdated
Show resolved
Hide resolved
Thanks for taking a look @jjspace!
Yes, but since this is a developer sandcastle example oriented for testing, I think it would be more trouble than it's worth. If you think it's critical, let me know.
Fair enough. I added a For a point of comparison see the glTF Sample Viewer. You can change out the environment map to one with a blue sky for a similar lighting scenario, such as "Wide Street" or "Helipad Golden Hour":
Good catch. That should be cleaned up now. I'm taking a last pass on your other feedback. |
I think this helps a ton with the appearance of the light around objects like the pot of coals, it's no longer uncannily blue.
I think overall this is looking pretty good now, is there more you wanted to do? |
Great, thanks @jjspace!
I just need to add a few unit tests to reflect my changes– doing so now. |
@jjspace This should be good to go! |
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.
Sorry for the delay on this @ggetz I think this is good to go now. I also tested a handful of other sandcastles that are lighting adjacent and they all seemed to still behave and look as expected (or better)
Description
This pull request introduces a new system for "procedural" lighting that builds on the Image-Based Lighting (IBL) approach in CesiumJS, aimed at improving the realism and visual quality of the default lighting environment. The changes focus on techniques that create a plausible environment map dynamically based on the current lighting conditions an existing atmospheric scattering code which renders the sky.
Image-Based Lighting Sandcastle
After
Before
Mirror Ball
After
Before
High Altitude Mirror Ball
After
Before
Dynamic Lighting
TODO: More screenshot comparisons
Key Changes
DynamicEnvironmentMapManager
: This new component generates lighting value dynamically based on the existing atmospheric calculations. It uses compute command to generate the following when position or sun position has significantly changed.DynamicEnvironmentMapManager
options accessible via theModel
andCesium3DTileset
only, at least for this PR, mainly due to the performance impact if these values are constantly updated.Additional Updates
ImageBasedLighting.imageBasedLightingFactor
: This property was broken and went unnoticed.ImageBasedLighting.luminanceAtZenith
: This property was removed instead of deprecated because there is no direct equivalent in the new implementation, and is likely not widely used (based on the fact that the above bug went unnoticed).CubeMap
: New utility functions in support of the operations inDynamicEnvironmentMapManager
, such as copying a texture to a specific face.Issue number and link
N/A
Testing plan
Author checklist
DynamicEnvironmentMapManager
options for Model entities?CONTRIBUTORS.md
CHANGES.md
with a short summary of my change