Skip to content

Adjust texture filtering presets. #19127

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

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

Conversation

axlitEels
Copy link
Contributor

@axlitEels axlitEels commented May 8, 2025

Objective

  • Do something about ImageSamplerDescriptor::default() not being the actual default (it's nearest instead of linear; nearest() doesn't trust default() to be nearest even though it is).
  • Bring image sampler preset names to industry standard (what we're referring to as just "linear" is more precisely called "trilinear"); add bilinear and anisotropic presets.
  • Make 16x anisotropic filtering an engine-wide default. The previous default is trilinear. The difference is present only with mipmapped textures, and aniso is just better looking with not that big of a performance impact.

Solution

  • Reversed ImageSamplerDescriptor::default()'s hierarchy. It now returns the preset we currently consider default. Everything builds upon nearest() instead.
  • Added new presets, deprecated linear() for trilinear().
  • Switched the default.

Copy link
Contributor

github-actions bot commented May 8, 2025

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!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-19127

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.

@axlitEels
Copy link
Contributor Author

Now that's an interesting artifact

@axlitEels
Copy link
Contributor Author

axlitEels commented May 8, 2025

testbed_ui that is. Replicated. Know how to fix. Don't know why that happened. With 0 depth texture, aniso shouldn't mess up anything compared to linear. The texture is just 48x48 32bit png, no mips included.

Major factor is that these elements don't have any padding and are tiny, that's why it's so intense.

Things start at examples\testbed\ui.rs, L449, the texture is textures/fantasy_ui_borders/numbered_slices.png

@ChristopherBiscardi ChristopherBiscardi added the A-Rendering Drawing game state to the screen label May 8, 2025
@ChristopherBiscardi ChristopherBiscardi added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 8, 2025
@axlitEels
Copy link
Contributor Author

axlitEels commented May 9, 2025

Although fine on modern PCs, 16x AF seems to slow down CPU rendering by up to 6 times and might also work bad on some mobile targets. I take it back, this PR is just for coherent generators.

@axlitEels axlitEels changed the title Adjust texture filtering defaults and presets. Adjust texture filtering presets. May 9, 2025
axlitEels added 3 commits May 9, 2025 21:01
Now that `ImageSamplerDescriptor::default()` actually represents engine-wide default, we can derive plugin defaults.
This one relied on `default()` being nearest.
@Elabajaba Elabajaba added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 9, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels May 9, 2025
@JMS55
Copy link
Contributor

JMS55 commented May 9, 2025

Please also update the lightmap bicubic_sampling docs.

@axlitEels
Copy link
Contributor Author

@JMS55 why tho? We seem to be using custom interpolation code and it's actual bicubic, which is not provided by wgpu (afaik neither Vulcan or DirectX don't expose it natively)

#ifdef LIGHTMAP_BICUBIC_SAMPLING
let texture_size = vec2<f32>(lightmap_size(lightmap_slot));
let texel_size = 1.0 / texture_size;
let puv = lightmap_uv * texture_size + 0.5;
let iuv = floor(puv);
let fuv = fract(puv);
let g0x = g0(fuv.x);
let g1x = g1(fuv.x);
let h0x = h0_approx(fuv.x);
let h1x = h1_approx(fuv.x);
let h0y = h0_approx(fuv.y);
let h1y = h1_approx(fuv.y);
let p0 = (vec2(iuv.x + h0x, iuv.y + h0y) - 0.5) * texel_size;
let p1 = (vec2(iuv.x + h1x, iuv.y + h0y) - 0.5) * texel_size;
let p2 = (vec2(iuv.x + h0x, iuv.y + h1y) - 0.5) * texel_size;
let p3 = (vec2(iuv.x + h1x, iuv.y + h1y) - 0.5) * texel_size;
let color = g0(fuv.y) * (g0x * sample(p0, lightmap_slot) + g1x * sample(p1, lightmap_slot)) + g1(fuv.y) * (g0x * sample(p2, lightmap_slot) + g1x * sample(p3, lightmap_slot));
#else
let color = sample(lightmap_uv, lightmap_slot);
#endif

@JMS55
Copy link
Contributor

JMS55 commented May 10, 2025

The shader code doesn't need to be touched, but the docs should be updated to remove the reference to linear, and replace it with bilinear/trilinear (not sure if lightmaps typically use mipmaps?)

https://docs.rs/bevy/latest/bevy/pbr/struct.Lightmap.html#structfield.bicubic_sampling

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-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants