nv2a: Handle anisotropic filter setting #2450
Conversation
hw/xbox/nv2a/pgraph/vk/texture.c
Outdated
| .anisotropyEnable = VK_FALSE, | ||
| // .anisotropyEnable = VK_TRUE, | ||
| // .maxAnisotropy = properties.limits.maxSamplerAnisotropy, | ||
| .anisotropyEnable = max_anisotropy > 1, |
There was a problem hiding this comment.
Before this can be VK_TRUE, the samplerAnisotropy device feature will need to be enabled. Then check that it is actually enabled via r->enabled_physical_device_features.samplerAnisotropy.
See here:
xemu/hw/xbox/nv2a/pgraph/vk/instance.c
Line 549 in 59626b1
There was a problem hiding this comment.
Done? I don't have a way to actually test.
Should we sort those desired features alphabetically? I can imagine it'll become hard to tell what has been requested at a glance as the list gets longer.
There was a problem hiding this comment.
sort those desired features alphabetically?
Probably yes
hw/xbox/nv2a/pgraph/vk/texture.c
Outdated
| pgraph_reg_r(pg, NV_PGRAPH_BORDERCOLOR0 + texture_idx * 4); | ||
| bool is_indexed = (state.color_format == | ||
| NV097_SET_TEXTURE_FORMAT_COLOR_SZ_I8_A8R8G8B8); | ||
| uint32_t xbox_max_anisotropy = |
There was a problem hiding this comment.
xbox_prefix is inconsistent with other fields- Will need to be added to texture key for caching. See below at line 1127
There was a problem hiding this comment.
Done.
Is the plan for the nearby FIXME to pull the sampler out of the texture binding?
There was a problem hiding this comment.
Is the plan for the nearby FIXME to pull the sampler out of the texture binding?
They need not be coupled, but it's also not a high priority. Keeping them together simplifies lookup/expiration at the cost of inflated sampler counts.
hw/xbox/nv2a/pgraph/vk/texture.c
Outdated
| uint32_t xbox_max_anisotropy = | ||
| 1 << (GET_MASK(pgraph_reg_r(pg, NV_PGRAPH_TEXCTL0_0 + texture_idx*4), | ||
| NV_PGRAPH_TEXCTL0_0_MAX_ANISOTROPY)); | ||
| uint32_t max_anisotropy = |
There was a problem hiding this comment.
Done, also moved closer to where it's actually used.
Implements handling of anisotropic filter level from SET_TEXTURE_CONTROL0.
abaire
force-pushed
the
fix_2444_respect_anisotropic_filter_level
branch
from
d2ab8e4 to
cce8e3a
Compare
hw/xbox/nv2a/pgraph/gl/texture.c
Outdated
| needs_border_color = needs_border_color || binding->addrp == NV_PGRAPH_TEXADDRESS0_ADDRU_BORDER; | ||
| } | ||
|
|
||
| glTexParameterf(binding->gl_target, GL_TEXTURE_MAX_ANISOTROPY, max_anisotropy); |
There was a problem hiding this comment.
Technically GL_TEXTURE_MAX_ANISOTROPY is not GL 4.0 core--it comes from the ARB_texture_filter_anisotropic extension.
The original extension was EXT_texture_filter_anisotropic and used the GL_TEXTURE_MAX_ANISOTROY_EXT token, which have identical numeric values.
Though it is ubiquitous, it would be good practice to confirm GL_EXT_texture_filter_anisotropic extension is present (See pgraph_gl_init) before using it.
There was a problem hiding this comment.
Done; added a struct to track extension use similar to VK enabled features.
hw/xbox/nv2a/pgraph/vk/texture.c
Outdated
| .anisotropyEnable = VK_FALSE, | ||
| // .anisotropyEnable = VK_TRUE, | ||
| // .maxAnisotropy = properties.limits.maxSamplerAnisotropy, | ||
| .anisotropyEnable = r->enabled_physical_device_features.wideLines && |
There was a problem hiding this comment.
Looks like a copy paste error. We need to check for samplerAnisotropy feature, not wideLines here.
There was a problem hiding this comment.
Stupid mistake/autosuggest fail, done.
| F(occlusionQueryPrecise, true), | ||
| F(samplerAnisotropy, false), | ||
| F(shaderClipDistance, true), | ||
| F(shaderTessellationAndGeometryPointSize, true), |
There was a problem hiding this comment.
These changes seem unrelated to the actual work. Consider moving this to a separate pull request, or elaborate why it should be in here
There was a problem hiding this comment.
These changes are related to the PR; samplerAnisotropy was added as part of emulating the max anisotropy field.
As confirmed previously in this code review, these are intended to be kept alphabetized to make it easier to determine what features are and are not being requested. In my opinion the overhead of yet another PR for a tiny additional cleanup is not justified.
|
Thanks! |
Implements handling of anisotropic filter level from SET_TEXTURE_CONTROL0.
Fixes #2444 (in conjunction with PR #2435 for
STEAL)Fixes #2385 (for whatever isn't already fixed by PR #2435)
Tests: https://github.com/abaire/nxdk_pgraph_tests/blob/4dc2b8f16749d0d9b9bd9e8c90b649944f96a662/src/tests/texture_anisotropy_tests.h#L10
HW results: https://abaire.github.io/nxdk_pgraph_tests_golden_results/results/Texture_anisotropy/index.html
PR results: https://abaire.github.io/xemu-dev_pgraph_test_results/fix_2444_respect_anisotropic_filter_level/index.html - Please note that there was a bleedover error in the test suite when I first ran the suite that tainted the diff for the
Texture_perspective_enableresult. If you manually compare theSourceandxemu Goldenimages they are now identical.