-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Generalized atmospheric scattering media #20838
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
Generalized atmospheric scattering media #20838
Conversation
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
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.
Consider this an approval for architecture/direction. This is not a review of the implementation, which I won't have time for.
|
This is some amazing work! I can't wait to create more types scenes with this new parameterization. I checked out the code locally and started playing around with the new parameterization, and what i noticed is that the sky turns completely black if there is only 1 mie scatterer or no scatterers at all. I posted my example file on this branch: Also created this quick chart/ visualization for the exponential falloff: https://observablehq.com/@mateh/exponential-falloff |
- add ScatteringMedium - CPU-side Atmosphere code
29312e1 to
c6727c7
Compare
c6727c7 to
14518e9
Compare
|
Posted a PR to this branch to fix some NaNs in the LUTs in case of single mie scatterer or no scatterers. |
14518e9 to
5f34082
Compare
Fix NaNs in scattering integral
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 am generally inclined to approve this PR! It's a very cool feature to have this generalized scattering, we could have atmospheres that the previous parameterization would not allow for sure. i have yet to come up with more test scenes. First I have a bunch of questions and I want to stress test this a bit more, measure precision loss, and especially come up with a solution for the sequential vs indexed bindings because this is going to cause a lot of churn otherwise.
| struct ScatteringMediumMissingError(AssetId<ScatteringMedium>); | ||
|
|
||
| #[derive(Clone, Component, ShaderType)] | ||
| pub struct GpuAtmosphere { |
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 think both ExtractedAtmosphere and GpuAtmosphere needs some rust doc strings to explain the "render world representation" vs the "shader uniform"
|
The diff is really messed up. Could you sync and merge from main again? |
c17dd9a to
1d3a05e
Compare
dbdf25e to
570abbe
Compare
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 think the general architectural decision to go this route makes sense, but I do want to make the user facing API concise as before. we can revise the naming and the user facing API in later PRs. I gave this a thorough test and found that maybe the strength parameter is counterintuitive and I prefer to use the scale_height as described in the paper.
I created a branch as a demo: ecoskey#9
no need to merge this but sort of encapsulates my feedback on the parameterization. the remaining comment I left also also non-blocking.
| &textures.aerial_view_lut.default_view, | ||
| &samplers.aerial_view_lut, | ||
| &textures.environment, | ||
| &BindGroupEntries::with_indices(( |
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.
Agreed that doing the switch to with_indices is warranted in this case, so let's just stick to it for now until we re-organize the binding in a dedicated PR.
| // samples from the atmosphere density LUT. | ||
| // | ||
| // calling with `component = 0.0` will return the atmosphere's absorption density, | ||
| // while calling with `component = 1.0` will return the atmosphere's scattering density. |
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.
Since scattering density and the absorption density uses the same distribution, I think it would make more sense to simply store the distribution of the main function (in this case exponential) and then multiplying by the absorption or scattering coefficients by passing them as uniforms to the shader. unless we want to use another compute shader to compute the scattering and absorption coefficients independently which sounds like a niche use case.
After thinking about this some more it would definitely make the code more "messy". So even though baking these values into the lut may not make sense for the use case, the current approach still makes for cleaner code. Also since these luts are super small, wouldn't be saving much VRAM anyway if we reduce the number of rows, and we don't need to worry too much about hitting floating point precision limits. (hopefully f16 texture can store these small values on all devices) .
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'm not sure things commute properly to be able to factor that out, at least when there's multiple scattering terms present:
density(p) = d1 * f1(p) + d2 * f2(p) + ... + dN * fN(p) where dN are the optical density coefficients and fN are the falloff distributions, which can be arbitrary functions
Something slightly different that we can do is after calculating all the values, normalize them wrt the greatest density value, and store RGBA16Unorm values in each texel instead
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.
Oh I see so it's a combination of functions for each scattering term. Okay then never mind it would be hard to factor out any coefficients if the functions can be any density distribution function.
| (7, &textures.multiscattering_lut.default_view), | ||
| (8, &samplers.multiscattering_lut), | ||
| (13, &textures.sky_view_lut.default_view), | ||
| (5, &gpu_medium.density_lut_view), |
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.
The diffs here is very ugly. I think there has to be a better to manage bindings in version control and code review
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.
going forward I'll try to factor out different chunks of the bindings into functions, after we merge #21625
| /// | ||
| /// [Mie scattering]: https://en.wikipedia.org/wiki/Mie_scattering | ||
| /// [Henyey-Greenstein phase function]: https://www.oceanopticsbook.info/view/scattering/level-2/the-henyey-greenstein-phase-function | ||
| Mie { |
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.
Also a dual-lobe Mie scattering function is common for clouds. but can be added later. ref: https://research.nvidia.com/labs/rtr/approximate-mie/publications/approximate-mie.pdf
| let extinction = sample_density_lut(r_i, 0.5) * 2.0; | ||
| // PERF: A possible later optimization would be to sample at `component = 0.5` | ||
| // (getting the average of the two rows) and then multiplying by 2 to find the sum. | ||
| let absorption = sample_density_lut(r_i, ABSORPTION_DENSITY); |
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.
Maybe it's better to create two separate functions sample_absorption and sample_density?
Objective
Right now, only a very small set of atmospheres are possible with Bevy's atmospheric scattering system because of the fixed set of scattering terms available. For example, a scene set in a dry, desert environment might want a dense low-lying layer of dust particulate separate from the ambient dust in the atmosphere. This PR introduces a mechanism for generalized scattering media, replacing the fixed scattering terms with a customizable asset.
Solution
see other new types
ScatteringMediumcontains a list ofScatteringTerms, which are processed into a set of two LUTs:falloff_resolution x 2LUT which contains the medium's optical density with respect to the atmosphere's "falloff parameter", a linear value which is 1.0 at the planet's surface and 0.0 at the edge of space. Absorption density and scattering density correspond to the first and second rows respectively.falloff_resolution x phase_resolutionLUT which contains the medium's scattering density multiplied by the phase function, with the U axis corresponding to the falloff parameter and the V axis corresponding toneg_LdotV * 0.5 + 0.5, whereneg_LdotVis the dot product of the light direction and the outgoing view vector.Testing
TODOS:
Showcase
Click to view showcase