Skip to content
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 atmospheric scattering #16314

Open
wants to merge 113 commits into
base: main
Choose a base branch
from
Open

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Nov 9, 2024

Implement procedural atmospheric scattering from Sebastien Hillaire's 2020 paper. This approach should scale well even down to mobile hardware, and is physically accurate.

Co-author: @mate-h

He helped massively with getting this over the finish line, ensuring everything was physically correct, correcting several places where I had misunderstood or misapplied the paper, and improving the performance in several places as well. Thanks!

Showcase:

403275417-b48bd0c8-4d35-4a1e-bfad-419f12779c46

For followup

  • Integrate with pcwalton's volumetrics code
  • refactor/reorganize for better integration with other effects
  • have atmosphere transmittance affect directional lights
  • add support for generating skybox/environment map

Credits

Built off of @mtsr's original branch, which handled the transmittance lut (arguably the most important part, and I still don't understand the parametrization)

Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No showstoppers. Looks great! Some public API nits.

I can't comment on the quality of the compute shaders, or most of the rendering for that matter. The comments sure help though!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is supposed to be alphabetical

pub struct Atmosphere {
/// Radius of the planet
///
/// units: km
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it might be beneficial for the computations to be done in km, the public API should use meters to stay consistent with the rest of bevys units. This conversion precision loss will be a one time thing when computing the uniform, and will be completely negligible.

/// Radius at which we consider the atmosphere to 'end' for our
/// calculations (from center of planet)
///
/// units: km
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

/// The scattering optical density of rayleigh particulare, or how
/// much light it scatters per kilometer
///
/// units: km^-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use SI unless this is what measurements are usually given in.

/// it scatters per kilometer.
///
/// units: km^-1
pub mie_scattering: f32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't bore you by repeating myself any more. 😅


/// A conversion factor between scene units and kilometers, used to
/// combat floating-point precision issues.
pub scene_units_to_km: f32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here obviously, but I think it becomes even clearer when this defaults to 1.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment at the top of the file would be nice, describing when to use this?

Copy link

@mate-h mate-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compute shaders look physically correct and I've reviewed and tested them and compared it to reference with the Hillarie paper and accompanying example application.

// Sun
commands.spawn((
DirectionalLight {
color: Color::srgb(0.98, 0.95, 0.82),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the directional light's color to srgb white (1,1,1) here results in more physically correct result since the transmittance though the atmosphere of the directional light is already accounted for with the throughput variable and the analytical integration method used.

let sphere_mesh = meshes.add(Mesh::from(Sphere { radius: 1.0 }));

// light probe spheres
commands.spawn((
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment does not show up on the light probes, but this will be used as test subject in a future iteration where we generate the environment cube maps from sampling the sky-view LUT.

fn setup_camera_fog(mut commands: Commands) {
commands.spawn((
Camera3d::default(),
Camera {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the exposure lower at noon and higher at sunset would make the showcase look more balanced, as well as adding a secondary directional light (moon) at night time to showcase a day-night cycle.

@aevyrie
Copy link
Member

aevyrie commented Jan 18, 2025

FYI turning on Msaa causes the app to crash. Should probably defend against this. Some render features see MSAA turned on, warn, and don't run anything in the plugin.

existing color approximated atmosphere transmittance (yellowish), so it
was skewing the resulting atmosphere look
@ecoskey
Copy link
Contributor Author

ecoskey commented Jan 18, 2025

FYI turning on Msaa causes the app to crash. Should probably defend against this. Some render features see MSAA turned on, warn, and don't run anything in the plugin.

this is a bug then, msaa should be compatible. will fix 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

8 participants