Use dual-source blending for rendering the sky #17672

ecoskey · 2025-02-04T03:30:56Z

Objective

Since previously we only had the alpha channel available, we stored the mean of the transmittance in the aerial view lut, resulting in a grayer fog than should be expected.

Solution

Calculate transmittance to scene in render_sky with two samples from the transmittance lut
use dual-source blending to effectively have per-component alpha blending

alice-i-cecile · 2025-02-04T04:55:29Z

@mate-h this one is for you :)

mate-h · 2025-02-04T08:21:03Z

I've had an initial look at this, and using dual source blending is a great idea, the diff looks good to me. I went to test things out and noticed some inaccuracies between what I expect to see ground truth, vs the results seem to be shifting colors in the rendered geometry toward orange.
I dug a bit deeper and it's likely an issue in either how we sample the transmittance lut or how we calculate the transmittance though the atmosphere for a path segment to the scene depth from the camera using that function.
I will continue investigating and testing this further.

I pushed all my testing code to the https://github.com/mate-h/bevy/tree/atmosphere-test branch and will keep working on that.

@mate-h

thanks @mate-h for more debugging help and your fancy reference raymarcher!

mate-h · 2025-02-07T02:38:10Z

After this last iteration and more thorough testing we were able to identify the source of the issue. I've tested both cases of the geometry above and below the horizon and the math checks out compared to the raymarched reference.
Test case for when the ray intersects the ground:

Test case for when the ray does not intersect the ground (else block):

mate-h · 2025-02-07T02:56:35Z

crates/bevy_pbr/src/atmosphere/functions.wgsl

@@ -118,6 +118,23 @@ fn sample_transmittance_lut(r: f32, mu: f32) -> vec3<f32> {
    return textureSampleLevel(transmittance_lut, transmittance_lut_sampler, uv, 0.0).rgb;
 }

+//should be in bruneton_functions, but wouldn't work in that module bc imports. What to do wrt licensing?


I think we should just leave a comment saying that the license in the bruneton_functions.wgsl file applies to this function, or come up with some way to reorganize the imports. Also just in general, since the code is not exaclty the same as bruneton's verison, it is a WGSL re-write, not sure how the licensing works because I am no legal expert. but does it still apply?

If it's a direct port with no significant practical changes then I'd say the license still applies.

crates/bevy_pbr/src/atmosphere/render_sky.wgsl

mate-h · 2025-02-08T20:27:30Z

@alice-i-cecile this one is ready for final review and merge when you get a chance!

alice-i-cecile · 2025-02-09T17:30:57Z

Needs a second rendering review :)

alice-i-cecile · 2025-02-09T17:31:39Z

crates/bevy_pbr/src/atmosphere/functions.wgsl

@@ -118,6 +118,23 @@ fn sample_transmittance_lut(r: f32, mu: f32) -> vec3<f32> {
    return textureSampleLevel(transmittance_lut, transmittance_lut_sampler, uv, 0.0).rgb;
 }

+//should be in bruneton_functions, but wouldn't work in that module bc imports. What to do wrt licensing?


This needs to be resolved before we can merge. I'd leave a little comment here.

crates/bevy_pbr/src/atmosphere/functions.wgsl

tychedelia

Can still see the sun shining when it's night, which didn't seem to be the case on main.

mate-h · 2025-02-10T05:07:10Z

Good point! There is a couple issues on main that may be unrelated to this PR:

The shadow maps are broken on OSX
The PBR is not yet integrated to the atmosphere, I started working on that now at https://github.com/mate-h/bevy/tree/pbr-atmosphere

So we might see light on objects in this example because of shadows maps not working, and the directional light not being occluded by the atmosphere or the planet earth. As far as I can tell this has not changed compared to main.

tychedelia · 2025-02-10T05:18:46Z

So we might see light on objects in this example because of shadows maps not working, and the directional light not being occluded by the atmosphere or the planet earth. As far as I can tell this has not changed compared to main.

I tested this PR on macOS and main on windows so that would make sense!

tychedelia

Confirmed on windows. LGTM pending other comments!

alice-i-cecile

The comment about the license needs to be resolved before I can merge this. To resolve this:

Mention that this is related to the bruneton_functions
Credit the original author in the comment
Name the license that it's used under in that comment.

ecoskey · 2025-02-10T23:25:46Z

@alice-i-cecile fixed! :)

dual-source transmittance

e208198

ecoskey added A-Rendering Drawing game state to the screen D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 4, 2025

check for dual-source blending support

74b9d7f

alice-i-cecile added this to the 0.16 milestone Feb 4, 2025

ecoskey added 2 commits February 6, 2025 00:29

Merge branch 'main' into transmittance-v2

2bbd809

fix transmittance sampling

84b87e5

thanks @mate-h for more debugging help and your fancy reference raymarcher!

mate-h approved these changes Feb 7, 2025

View reviewed changes

Merge branch 'main' into transmittance-v2

ed7f031

mate-h reviewed Feb 7, 2025

View reviewed changes

crates/bevy_pbr/src/atmosphere/render_sky.wgsl Show resolved Hide resolved

alice-i-cecile requested a review from tychedelia February 9, 2025 17:31

alice-i-cecile reviewed Feb 9, 2025

View reviewed changes

mate-h reviewed Feb 9, 2025

View reviewed changes

crates/bevy_pbr/src/atmosphere/functions.wgsl Show resolved Hide resolved

tychedelia reviewed Feb 10, 2025

View reviewed changes

tychedelia approved these changes Feb 10, 2025

View reviewed changes

tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 10, 2025

alice-i-cecile requested changes Feb 10, 2025

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 10, 2025

licensing comment

cbb5a27

ecoskey force-pushed the transmittance-v2 branch from 6eafc05 to cbb5a27 Compare February 10, 2025 23:21

Merge branch 'main' into transmittance-v2

48edd29

ecoskey added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 10, 2025

alice-i-cecile approved these changes Feb 10, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Feb 10, 2025

Merged via the queue into bevyengine:main with commit 83370e0 Feb 11, 2025
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dual-source blending for rendering the sky #17672

Use dual-source blending for rendering the sky #17672

ecoskey commented Feb 4, 2025

alice-i-cecile commented Feb 4, 2025

mate-h commented Feb 4, 2025 •

edited

Loading

mate-h commented Feb 7, 2025

mate-h Feb 7, 2025 •

edited

Loading

superdump Feb 9, 2025

mate-h commented Feb 8, 2025

alice-i-cecile commented Feb 9, 2025

alice-i-cecile Feb 9, 2025

tychedelia left a comment

mate-h commented Feb 10, 2025

tychedelia commented Feb 10, 2025

tychedelia left a comment

alice-i-cecile left a comment

ecoskey commented Feb 10, 2025

Use dual-source blending for rendering the sky #17672

Use dual-source blending for rendering the sky #17672

Conversation

ecoskey commented Feb 4, 2025

Objective

Solution

alice-i-cecile commented Feb 4, 2025

mate-h commented Feb 4, 2025 • edited Loading

mate-h commented Feb 7, 2025

mate-h Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

superdump Feb 9, 2025

Choose a reason for hiding this comment

mate-h commented Feb 8, 2025

alice-i-cecile commented Feb 9, 2025

alice-i-cecile Feb 9, 2025

Choose a reason for hiding this comment

tychedelia left a comment

Choose a reason for hiding this comment

mate-h commented Feb 10, 2025

tychedelia commented Feb 10, 2025

tychedelia left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

ecoskey commented Feb 10, 2025

mate-h commented Feb 4, 2025 •

edited

Loading

mate-h Feb 7, 2025 •

edited

Loading