Conversation
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
1 similar comment
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
kfc35
added
A-Rendering
| if !has_fab_lut { | ||
| let mut images = app.world_mut().resource_mut::<Assets<Image>>(); | ||
| let texture = images.add( | ||
| Image::from_buffer( |
There was a problem hiding this comment.
Don't we need to gate this behind the ktx2 feature?
There was a problem hiding this comment.
We should feature gate this in general. Including these bytes here will increase the size of the binary so it should be opt in.
I explored this BRDF lut approach earlier for the PR from a month ago (clamp FAB)
There was a problem hiding this comment.
I'll go with @mate-h's suggestion to feature-gate the LUT so ktx2 can just be enabled when we need it.
The LTC LUTs have the same problem, I'll open an issue.
|
Should we maybe have the LUT indexed via roughness instead of perceptual_roughness? |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
mate-h
left a comment
mate-h
left a comment
There was a problem hiding this comment.
Nice work but I have a few questions and comments. I also worked on something similar as a test but never posted a PR for it. I think having that extra KTX asset bundled into your binary might be something users will want to opt into rather than default feature.
| // F_AB | ||
| entries = entries.extend_with_indices(( | ||
| ( | ||
| 40, |
There was a problem hiding this comment.
I hope we're not close to hitting binding limits on this 😬 if we are might consider reusing a linear sampler
There was a problem hiding this comment.
We are, hitting max_sampled_textures_per_shader_stage on webgl -_-
I think I'll just make LTC reuse the same sampler for both of its LUTs to free up a slot.
| // Keep F_ab positive to avoid divide-by-zero in downstream BRDF terms. | ||
| let f_ab_epsilon = 0.00005; | ||
| return max(vec2<f32>(-1.04, 1.04) * a004 + r.zw, vec2<f32>(f_ab_epsilon)); | ||
| return textureSampleLevel(view_bindings::f_ab_lut, view_bindings::f_ab_lut_sampler, vec2<f32>(NdotV, perceptual_roughness), 0.0).rg; |
There was a problem hiding this comment.
I wouldn't completely replace the polynomial approximation with the lut lets keep both. Add a shader def and enable the LUT with a feature flag. We should discuss whether this should be opt in or on by default.
|
Oh and the graphs you attached to the PR description are amazing very helpful in seeing the difference |
4 participants
Objective
F_ABcurrently uses a polynomial approximation. This replaces it it with a more precise LUT.Solution
Add a LUT computed using Monte-Carlo sampling.
The minimum value was clipped to
1e-6, which modifies 0.4% of the values that were below that but should help prevent bugs like #22833.Open questions:
Testing
Showcase
F_AB tables
Current PR uses the Monte-Carlo based LUT, bevy currently uses the polynomial approximation.
Solari white furnace
This new approximation significantly improves the solari white furnace.
Main:
With this PR:
LUT size vs error
I ended up going with 64x64.
