-
-
Notifications
You must be signed in to change notification settings - Fork 345
nv2a: Implement texture LOD bias #2435
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
nv2a: Implement texture LOD bias #2435
Conversation
3ece4e2
to
a895ea2
Compare
a895ea2
to
b8ada79
Compare
@Triticum0 excellent, thanks for testing! Could you also check Vulkan with the newest build? I don't have access to a VK capable machine at the moment and put in a prospective fix. |
Also tested crash and burn as well Wreckless The Yakuza Missions Both seem to be unrelated. |
Thanks! I'll take a look at the traces you added to #2385, we might want to split them into a separate issue if they're due to a novel issue. |
Opps actually fixes Crash 'N' Burn floor rendering and tree rendering. Look at outline around far floor textures and outline around 2d tree texture. #1110
![]() ![]()
|
Renderdoc are on this issue if your interested have add new one for the railing issue as I missed it before |
Doesn't fix Racing Evoluzione/Apex ground texture issue. |
Fixes #2377 |
Fixed metal mesh overhead sign next to Xicat logo rendering incorrectly. on Motor Trend Presents Lotus Challenge. Hopefully why car blue or green could be looked at only happens with no shader cache. Other Issues in #1228 are unaffected |
@Triticum0 just to confirm, does this fully resolve #1110 or are there still remaining issues? I don't want to accidentally close out that issue if there is still work to be done. |
Create new issue which supersede old issue with remaining issues so you can close it. other issue unrelated so I will move it |
Improves the behavior of burnout 2 grass texture. #1161 No 100% sure if it matches hardware seem behavior present on hardware but not if it the same. need hardware capture to verify. here video where you can see line on hardware https://youtu.be/HGxRSVldAHk?t=333 @abaire Leave it to you judgment where you want to close the issue. the other issues on the pr either occur on hardware or are already fixed so can be closed if you thing the fix sufficient for bug above. |
Does the Z-fighting mentioned not occur anymore? I assumed that bug would need coldhex's #2240 to fix some parts of it. We might need to break that bug up into separate issues, maybe we can just do that after this change is merged and leave off whatever is fixed by LOD biasing.
|
It hard to see but It also occurs on hardware https://youtu.be/HGxRSVldAHk?t=502 so you can close that too. ask cold-hex about similar issue and he mention it occurred on hardware so in most of my old z-fighting I mentioned weren't valid. |
hw/xbox/nv2a/pgraph/vk/texture.c
Outdated
.minLod = mipmap_en ? MIN(state.min_mipmap_level, state.levels - 1) : 0.0, | ||
.maxLod = mipmap_en ? MIN(state.max_mipmap_level, state.levels - 1) : 0.0, | ||
.mipLodBias = 0.0, | ||
.mipLodBias = convert_lod_bias( |
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.
Absolute value needs to be clamped to device limits (see VkPhysicalDeviceLimits::maxSamplerLodBias
)
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.
Done, I think; will need to see if my change builds on CI since I can't build VK locally right now.
hw/xbox/nv2a/pgraph/gl/texture.c
Outdated
return possibly_dirty; | ||
} | ||
|
||
static inline float convert_lod_bias(uint32_t lod_bias) |
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.
If implementation is identical between renderers, prefer to have this in common code
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.
Done.
hw/xbox/nv2a/pgraph/gl/texture.c
Outdated
unsigned int addru = GET_MASK(address, NV_PGRAPH_TEXADDRESS0_ADDRU); | ||
unsigned int addrv = GET_MASK(address, NV_PGRAPH_TEXADDRESS0_ADDRV); | ||
unsigned int addrp = GET_MASK(address, NV_PGRAPH_TEXADDRESS0_ADDRP); | ||
unsigned int lod_bias = |
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.
nit: Prefer to group filter params
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.
Done.
pgraph_texture_mag_filter_gl_map[mag_filter]); | ||
binding->mag_filter = mag_filter; | ||
} | ||
if (lod_bias != binding->lod_bias) { |
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.
binding->lod_bias
not initialized?
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.
Good catch, done.
hw/xbox/nv2a/pgraph/gl/renderer.h
Outdated
bool border_color_set; | ||
GLenum gl_target; | ||
GLuint gl_texture; | ||
uint32_t lod_bias; |
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.
nit: Prefer to group filter params
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.
Done
float lod_bias = pgraph_convert_lod_bias_to_float( | ||
GET_MASK(filter, NV_PGRAPH_TEXFILTER0_MIPMAP_LOD_BIAS)); | ||
if (lod_bias > r->device_props.limits.maxSamplerLodBias) { | ||
lod_bias = r->device_props.limits.maxSamplerLodBias; |
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.
Need to also handle negative bias clamp (absolute value must not exceed maxSamplerLodBias)
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.
Oops, misread your comment, sorry!
That said, is it actually necessary to manually clamp these at the application level?
The docs lead me to believe that the driver would handle the clamping, since it ends up being a summation of the bias and the sampling so simply clamping the bias may be insufficient to remain in bounds anyway:
maxSamplerLodBias is the maximum absolute sampler LOD bias. The sum of the mipLodBias member of the [VkSamplerCreateInfo](https://registry.khronos.org/vulkan/specs/latest/man/html/VkSamplerCreateInfo.html) structure and the Bias operand of image sampling operations in shader modules (or 0 if no Bias operand is provided to an image sampling operation) are clamped to the range [-maxSamplerLodBias,+maxSamplerLodBias]. See https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#samplers-mipLodBias.
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.
is it actually necessary
Maybe, maybe not. According to spec: The absolute value of mipLodBias must be less than or equal to VkPhysicalDeviceLimits::maxSamplerLodBias
the driver would handle the clamping
In general, Vulkan drivers are a lot less forgiving. Even when operating fully within spec, they can be problematic. If we are lucky, a case like this would trigger a validation warning and have no other impact, if we are unlucky it results in some random overflow and a crash, so it's best to follow spec guidance when populating struct params.
static inline float pgraph_convert_lod_bias_to_float(uint32_t lod_bias) | ||
{ | ||
int sign_extended_bias = lod_bias; | ||
if (lod_bias & (1 << 12)) { |
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.
Does this 12th bit have any significant name or is it merely tested to "or in" the subsequent 0x00001FFF const? And whats the significance of that? Fixed point ceiling, because of the final division by 256.0f ? Xemu is not strong in code comments but this looks like a hard to maintain bit of logic of anyone ever needs to revisit this again.
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.
Never mind, it is actually doing sign extension on the 12th bit onwards, assuming the trust contains a 12 bit signed fixed point number (4 integer + 8 fractional bits, in twos complement, a.k.a. Q3.8). Apparently there's no helper function for that yet.
Just 1 thought: the output range is now -8.0 to +7.99609375 which is not exactly 8.0
Would that be any problem?
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.
I don't think there are many places where 5.8 fixed point numbers are used and it doesn't seem clearly useful to bother with a more complicated generic sign extended fixed point -> float conversion function.
I also doubt it'd 7.99609375 versus 8 would result in noticeable differences if it actually ends up being materially different from how the nv2a HW handles the 5.8 value. I'd guess there's likely more variance across HW/drivers than that fractional delta in any case.
Fixes #973 |
Fixes #1204 |
Should Fix #1930 but haven't tested it |
Thanks! |
Did fix #1930 |
Fixes #767
Fixes #973
Fixes #1110
Fixes #1204
Fixes #2377
Tests: https://github.com/abaire/nxdk_pgraph_tests/blob/2aa0cb89543f3853a3830ba15a03c9f7dad58d69/src/tests/texture_lod_bias_tests.h#L1
HW results: https://abaire.github.io/nxdk_pgraph_tests_golden_results/results/Texture_LOD_Bias/index.html
PR results: https://github.com/abaire/xemu-dev_pgraph_test_results/blob/fix/implements_lod_bias/results/xemu-0.8.101-1-g3ece4e2a17-fix_implements_lod_bias-3ece4e2a170f38aef3f220ab007b090bbb29233b/Darwin_arm64/gl_Apple_Apple_M3_Max/gslv_4.10/Texture_LOD_Bias/LODBias.png