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

Fix mipmap bias behavior by refactoring how samplers are created by Material Storage. #81350

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

DarioSamo
Copy link
Contributor

@DarioSamo DarioSamo commented Sep 5, 2023

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.

@DarioSamo DarioSamo requested a review from a team as a code owner September 5, 2023 17:15
@Chaosus Chaosus added this to the 4.x milestone Sep 5, 2023
@Chaosus Chaosus requested review from reduz and clayjohn September 5, 2023 17:22
@DarioSamo DarioSamo force-pushed the mipmap-bias branch 2 times, most recently from aaf487b to fe687fe Compare September 5, 2023 17:35
@DarioSamo
Copy link
Contributor Author

Thanks @AThousandShips for the suggestions, I forgot about the parameter rules for a bit there.

Copy link
Member

@clayjohn clayjohn left a 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

@DarioSamo DarioSamo force-pushed the mipmap-bias branch 3 times, most recently from 02bf1ed to a6d16d4 Compare September 6, 2023 14:02
@DarioSamo
Copy link
Contributor Author

DarioSamo commented Sep 6, 2023

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.
@Calinou
Copy link
Member

Calinou commented Sep 9, 2023

This also fixes the issue where it was not possible to use different mipmap biases across different viewports.

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.

@DarioSamo
Copy link
Contributor Author

DarioSamo commented Sep 10, 2023

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).

@DarioSamo
Copy link
Contributor Author

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.

Copy link
Member

@clayjohn clayjohn left a 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

@clayjohn clayjohn added bug and removed enhancement labels Sep 22, 2023
@clayjohn clayjohn modified the milestones: 4.x, 4.2 Sep 22, 2023
Copy link
Member

@Calinou Calinou left a 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.

@akien-mga akien-mga merged commit 525c72e into godotengine:master Sep 22, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mipmap bias slider in the project settings no longer has any effect
6 participants