-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Fix mipmap bias behavior by refactoring how samplers are created by Material Storage. #81350
Conversation
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.h
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.h
Outdated
Show resolved
Hide resolved
aaf487b
to
fe687fe
Compare
Thanks @AThousandShips for the suggestions, I forgot about the parameter rules for a bit there. |
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.
Great work! I left a few comments for your consideration
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
02bf1ed
to
a6d16d4
Compare
Added the change to mobile renderer as well, although originally they didn't use the 'custom' samplers in the projectors and decals. |
…aterial Storage. Introduces a new structure to store samplers created with certain parameters instead of storing a 'custom' set of samplers. Allows viewports to correctly configure the mipmap bias and use it when rendering the scene.
a6d16d4
to
9b91750
Compare
Would this make it possible to use a different mipmap bias for 2D and 3D rendering in a single viewport in the future? Right now, changing mipmap bias in a viewport affects both 2D and 3D. (Note that mipmaps are disabled by default in 2D; you need to enable them in the 2D filter property of Viewport, and enable the Mipmaps import option on the texture.) This should be achievable using two viewports thanks to your PR now, but using a single viewport should reduce the performance overhead. |
Should be easily doable. Basically the PR allows any class with access to MaterialStorage to create a set of samplers configured however they need it. My implementation chose to do it for each set of RenderBuffers as the mipmap bias required for rendering the scene differs depending on settings that affect the buffers, but even the same renderer pulls from different set of samplers depending on the content (e.g. shadowmaps use the version without mipmap bias despite being in a viewport with bias, as it wouldn't make much sense to use it there). |
Bumping this for later, we'll most likely need this to be merged first before #81197 is as it directly affects the quality of the upscaling. |
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.
Looks good to me. I haven't tested locally, but I trust that Dario tested adequately. This isn't a very risky change.
This PR introduces a slight change in behaviour as Decals and projectors in the mobile renderer now properly use the custom samplers with mipmap bias applied (whereas before they ignored mipmap bias). Since the mipmap bias was broken anyway, users should only notice an improvement
This PR is technically fixing a regression that occurred sometime after 4.1 (I think) so it shouldn't be cherrypicked unless we find out that the regression happened before 4.1
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.
Tested locally (rebased on top of master
fe5b1c8), it works as expected.
Note that this PR does not fix #65690, which still occurs when adjusting mipmap bias manually, enabling antialiasing properties or decreasing 3D resolution scaling (as these automatically adjust mipmap bias). Either way, that can be looked into separately.
Thanks! |
Introduces a new structure to store samplers created with certain parameters instead of storing a 'custom' set of samplers. Allows viewports to correctly configure the mipmap bias and use it when rendering the scene.
This effectively fixes #81214 and also introduces rendering improvements for the FSR 2 PR #81197, which will be adapted to account for this behavior properly when this is merged.
The most obvious way to test this is that changing the mipmap bias setting at the project level or per viewport effectively did nothing before. This also fixes the issue where it was not possible to use different mipmap biases across different viewports.
The fix should work for both RenderingDevice-based renderers Forward+ and Mobile.