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

Add anisotropic filtering to GLES2 backend #45654

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Conversation

furrykef
Copy link

@furrykef furrykef commented Feb 2, 2021

Fixes #45641

I made this change by copying the relevant code from the GLES3 backend to the GLES2 backend. In so doing, I noticed a strange line, line 8358 of rasterizer_storage_gles3.cpp:

config.use_anisotropic_filter = config.extensions.has("rendering/quality/filters/anisotropic_filter_level");

The strange thing about it is, as far as I can tell, this variable soon gets overwritten without having been used. Still, I copied this line even though I'm not sure it has any effect.

(EDIT: I have discussed this line with other devs and removed it from the GLES2 backend. It's still in the GLES3 backend, though.)

(EDIT: It's gone from both backends now.)

Move definition of rendering/quality/filters/anisotropic_filter_level to
servers/visual_server.cpp, since both GLES2 and GLES3 now use it

rasterizer_storage_gles3.cpp: Remove a spurious variable write (the
value gets overwritten soon after)
@lawnjelly
Copy link
Member

Looks fine to me although I haven't tested. Great first PR. 👍

Seeing the duplication in texture_set_data and texture_set_flags reminded me I noticed a lot of code duplication here when converting this for Godot 4.0. In there I used a separate function to do all the common stuff between the two functions. It should be fine to leave like this though in 3.2.

@akien-mga
Copy link
Member

Looks good to me! I'll let @clayjohn do the final review.

As a tip for future PRs, you might want to make them from a dedicated branch instead of your 3.2 branch, so that you can easily create new branches for new PRs even before the first one is merged.

@Calinou
Copy link
Member

Calinou commented Feb 2, 2021

I'd like to do some testing on Android and HTML5 before this is merged, just in case 🙂

Edit: I tested this PR and it works successfully on Linux (NVIDIA), HTML5 (Linux + Firefox, Linux + Chromium, Android + Chromium) and Android (OnePlus 6).

Minimal reproduction project: test_anisotropic_gles2.zip (includes Android and HTML5 exports)

In the above project, an isotropic texture is displayed on the left, an anisotropic texture is displayed on the right with 16× anisotropic filtering.

Screenshots

Linux (NVIDIA)

2021-02-02_17 23 38

HTML5 (Linux + Firefox)

2021-02-02_17 23 58

HTML5 (Linux + Chromium)

2021-02-02_17 24 20

HTML5 (Android + Chromium)

Screenshot_20210202-172702

Android (OnePlus 6)

Screenshot_20210202-172736

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!

@akien-mga akien-mga merged commit 24cb608 into godotengine:3.2 Feb 2, 2021
@akien-mga
Copy link
Member

akien-mga commented Feb 2, 2021

Thanks! And congrats for your first merged Godot contribution 🎉

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.

5 participants