-
Notifications
You must be signed in to change notification settings - Fork 0
WIP: Atmosphere PBR support #10
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
Conversation
|
Todos:
|
|
|
Reusing the shadow functions requires significant changes, my initial approach completely failed when I attempted to pass the directional light as a reference / pointer to the wgsl functions. still the shadow samplers bindings are conflicting, and without some hacky work arounds like shader defs or simply copying things, not the right approach. My second approach is this PR #11 which could work in theory, but is still a large refactoring change and probably out of scope for just the PBR integration of the atmosphere. |
| pub rotation: Quat, | ||
| /// Whether the diffuse contribution should affect meshes that already have lightmaps. | ||
| pub affects_lightmapped_mesh_diffuse: bool, | ||
| /// Cubemap resolution in pixels (must be a power-of-two). |
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.
Is POT-ness verified/asserted anywhere?
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.
Nope, good idea to add that assert
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.
assert is panic, hold off on this for now, theres a pattern we gotta make for this
crates/bevy_light/src/probe.rs
Outdated
| pub struct AtmosphereEnvironmentMapLight { | ||
| /// Luminance multiplier in cd/m². | ||
| pub intensity: f32, | ||
| /// World-space rotation applied to the generated cubemap. |
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.
what is this for? is there a practical application, or is it just cus you can? if i set this to like, 45 degrees around X, will the sky be sideways, or will the pixelation be sideways (but the sky still the right way)
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.
Not needed, let's remove this. The probe should always follow the orientation of the light being received from the atmosphere. So it should be relative to that and not relative to anything else.
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 I was merely copying the same attributes as the GeneratedEnvironmentMapLight
| fn transmittance_lut_r_mu_to_uv(atm: Atmosphere, r: f32, mu: f32) -> vec2<f32> { | ||
| // Distance along a horizontal ray from the ground to the top atmosphere boundary | ||
| let H = sqrt(atmosphere.top_radius * atmosphere.top_radius - atmosphere.bottom_radius * atmosphere.bottom_radius); | ||
| let H = sqrt(atm.top_radius * atm.top_radius - atm.bottom_radius * atm.bottom_radius); |
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.
is this atm now to avoid name conflict with a binding of the same name? i prefer atmosphere
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.
Yes that's right I was getting name conflicts. Any tips on how to keep it atmosphere ?I would also prefer that :D
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 putting this function in a file that doesnt import the binding should work, not sure though
| origin: Vec3::new(0.0, 3_389_500.0, 0.0), | ||
| ground_albedo: Vec3::splat(0.1), | ||
| rayleigh_density_exp_scale: 1.0 / 10_430.0, | ||
| rayleigh_scattering: Vec3::new(0.019918e-3, 0.01357e-3, 0.00575e-3), |
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.
why are these using a mix of both e-3 and 0.00xx? why not 1.9918e-5 etc
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 values are copied from scientific papers so I kept the units roughly the same as cited in the paper. While we could shorten these, it does degrade the readability and make them seem like arbitrarily-chosen constants. Maybe it would be helpful to add references.
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.
lets keep them yeah and add reference
| env_map_light.size.x / 8, | ||
| env_map_light.size.y / 8, |
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.
are we relying on this being divisible by 8? i usually write div_ceil here. iirc theres a check in the shader to discard threads that are outside the useful range, is that path ever hit
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.
Good point, should add an assert for it, and stress test it with different POT values for the env map size.
| } | ||
|
|
||
| // exposure compensation | ||
| inscattering *= view.exposure; |
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.
is this physically based or is it a "look good" factor? what is it compensating for
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.
Previously the exposure multiplication was inside of the functions, see other comment
Exposure compensation is needed for compositing the atmosphere over the rendered scene both before/after these changes.
| (1, uniform_buffer::<AtmosphereSettings>(true)), | ||
| (2, uniform_buffer::<AtmosphereTransform>(true)), | ||
| (3, uniform_buffer::<ViewUniform>(true)), | ||
| (4, uniform_buffer::<GpuLights>(true)), |
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 can be ::sequential now
|
|
||
| inscattering += (*light).color.rgb * (scattering_factor + multiscattering_factor); | ||
| } | ||
| return inscattering * view.exposure; |
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.
Removed exposure here @atlv24
| (1, uniform_buffer::<AtmosphereSettings>(true)), | ||
| (2, uniform_buffer::<AtmosphereTransform>(true)), | ||
| (3, uniform_buffer::<ViewUniform>(true)), | ||
| (4, uniform_buffer::<GpuLights>(true)), |
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.
same here, ::sequential
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.
For the sequential, I've thought about cleaning this up, yet another separate, small PR
| (1, settings_binding.clone()), | ||
| (2, transforms_binding.clone()), | ||
| (3, view_binding.clone()), | ||
| (4, lights_binding.clone()), |
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.
sequential
Co-authored-by: atlv <[email protected]>
| let r = view_radius(); | ||
| // let world_pos = vec3<f32>(0.0, 0.0, 0.0); // get_view_position(); | ||
| let r = view_radius_constant(); | ||
| let world_pos = vec3<f32>(0.0, r, 0.0); |
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.
should this be get_view_position?
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.
Good question, I have a long answer to this. No, the sky view lut calculation assumes a non-spherical coordinate system because of the way it is parameterized. This is a limitation of this optimization technique. The result is that the horizon may be out of alignment for renderings that rely on the sky view lut. at the present we use it for environment maps, and if the raymarching is turned on this out of alignment of the horizon is not noticeable at high altitudes. That's thanks to the spherical coordinate system that the raymarched rendering introduces.
| AtmosphereSettings { | ||
| aerial_view_lut_max_distance: 3.2e5, | ||
| scene_units_to_m: 1e+4, | ||
| // set this to Raymarching for high quality volumetric shadows |
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 have a runtime toggle for this? maybe pressing R or something
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.
Yes good idea, will add that.
Co-authored-by: atlv <[email protected]>
|
After some discussion on Discord it's best if I split this into multiple smaller PRs:
|
# Objective - Add generated environment lighting to the atmosphere for reflections and diffuse - Tracking issue: #20374 ## Solution - created a new atmosphere node type called environment - up-sampling the existing sky-view lookup texture and tied to the current view only. this atmosphere cube map pass has negligible performance impact, however the filtering pipeline does have a small performance impact to the example. - using the new filtering pipeline, creates mip chain for different roughness levels as well as diffuse ## Testing - ran the atmosphere example --- ## Showcase <img width="1281" height="752" alt="Screenshot 2025-08-12 at 12 19 08 PM" src="https://github.com/user-attachments/assets/8f00ff25-5f48-4c51-b67e-abcbf421abc4" /> ```rs commands.spawn(( Camera3d::default(), // ... // Renders the atmosphere to the environment map from the perspective of the camera AtmosphereEnvironmentMapLight::default(), )); ``` ## Limitations, out of scope - The generation does not support light probes (yet). This allows to render the atmosphere as a light probe from any "location" within the scene and within the overall atmosphere. - therefore we assume that the relative scale of the scene to the atmosphere are orders of magnitude different , this is a safe assumption now with the current impl not officially supporting space views, yet - the PBR directional light is still unaffected by the atmosphere (not tinted by it) out of scope for this PR - unrelated issue to this PR: small black pixels around the fully reflective sphere. narrowed it down, this is a problem in the inscattering calculation based on the depth value in the render sky shader. however the addition of the env map light makes this problem more "apparent". to be addressed separately. - past work staged for further PRs, up next: mate-h#10
Work in progress pull request for rebasing bevyengine#19037 onto Bevy main branch and address some feedback.