-
-
Notifications
You must be signed in to change notification settings - Fork 382
nv2a: Fix DstAlpha blending for const-alpha surface modes #2478
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
base: master
Are you sure you want to change the base?
nv2a: Fix DstAlpha blending for const-alpha surface modes #2478
Conversation
|
It fixes the issue on Vulkan and OpenGL. But the water is missing on OpenGL. It probably needs #2472 to render properly. VulkanOpenGL |
|
Thanks for confirming! The missing env mapping on GL is indeed expected without #2472 (though I didn't realize it always worked on Vulkan) |
|
On NVIDIA Water rendering missing on both renders using vulkan makes no differences on my 3060ti. Issue is resolved on my end as well smoke effect all seem to be fixed haven't got to fog area yet but from what you written seem unrelated. |
|
Interesting, so AMD has better handling of 2D textures in 3D modes than Nvidia on Vulkan? 😄 |
|
/gemini review |
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.
Code Review
This pull request correctly addresses an issue with DstAlpha blending on surfaces with forced alpha bits by introducing a fixup_blend_factor_for_surface function. This new function remaps the blend factors for both OpenGL and Vulkan renderers, which should fix the incorrect blending behavior. The changes are well-structured and the addition of a new surface.c file to contain this logic is a good approach. My review includes a few suggestions to improve code readability and maintainability by caching a register value to avoid redundant lookups and refactoring a series of if statements into a switch.
hw/xbox/nv2a/pgraph/gl/draw.c
Outdated
| uint32_t sfactor = fixup_blend_factor_for_surface( | ||
| GET_MASK(pgraph_reg_r(pg, NV_PGRAPH_BLEND), | ||
| NV_PGRAPH_BLEND_SFACTOR), | ||
| &pg->surface_shape); | ||
| uint32_t dfactor = fixup_blend_factor_for_surface( | ||
| GET_MASK(pgraph_reg_r(pg, NV_PGRAPH_BLEND), | ||
| NV_PGRAPH_BLEND_DFACTOR), | ||
| &pg->surface_shape); |
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.
For improved readability and to avoid multiple lookups of the same register, you could cache the value of pgraph_reg_r(pg, NV_PGRAPH_BLEND) in a local variable.
const uint32_t blend_reg = pgraph_reg_r(pg, NV_PGRAPH_BLEND);
uint32_t sfactor = fixup_blend_factor_for_surface(
GET_MASK(blend_reg, NV_PGRAPH_BLEND_SFACTOR),
&pg->surface_shape);
uint32_t dfactor = fixup_blend_factor_for_surface(
GET_MASK(blend_reg, NV_PGRAPH_BLEND_DFACTOR),
&pg->surface_shape);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/surface.c
Outdated
| if (blend_factor == NV_PGRAPH_BLEND_SFACTOR_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_SFACTOR_ONE; | ||
| } | ||
| if (blend_factor == | ||
| NV_PGRAPH_BLEND_SFACTOR_ONE_MINUS_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_SFACTOR_ZERO; | ||
| } | ||
| if (blend_factor == NV_PGRAPH_BLEND_DFACTOR_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_DFACTOR_ONE; | ||
| } | ||
| if (blend_factor == | ||
| NV_PGRAPH_BLEND_DFACTOR_ONE_MINUS_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_DFACTOR_ZERO; | ||
| } |
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.
To improve readability and make the logic clearer, consider refactoring the series of if statements into a switch statement on blend_factor.
| if (blend_factor == NV_PGRAPH_BLEND_SFACTOR_DST_ALPHA) { | |
| return NV_PGRAPH_BLEND_SFACTOR_ONE; | |
| } | |
| if (blend_factor == | |
| NV_PGRAPH_BLEND_SFACTOR_ONE_MINUS_DST_ALPHA) { | |
| return NV_PGRAPH_BLEND_SFACTOR_ZERO; | |
| } | |
| if (blend_factor == NV_PGRAPH_BLEND_DFACTOR_DST_ALPHA) { | |
| return NV_PGRAPH_BLEND_DFACTOR_ONE; | |
| } | |
| if (blend_factor == | |
| NV_PGRAPH_BLEND_DFACTOR_ONE_MINUS_DST_ALPHA) { | |
| return NV_PGRAPH_BLEND_DFACTOR_ZERO; | |
| } | |
| switch (blend_factor) { | |
| case NV_PGRAPH_BLEND_SFACTOR_DST_ALPHA: | |
| return NV_PGRAPH_BLEND_SFACTOR_ONE; | |
| case NV_PGRAPH_BLEND_SFACTOR_ONE_MINUS_DST_ALPHA: | |
| return NV_PGRAPH_BLEND_SFACTOR_ZERO; | |
| case NV_PGRAPH_BLEND_DFACTOR_DST_ALPHA: | |
| return NV_PGRAPH_BLEND_DFACTOR_ONE; | |
| case NV_PGRAPH_BLEND_DFACTOR_ONE_MINUS_DST_ALPHA: | |
| return NV_PGRAPH_BLEND_DFACTOR_ZERO; | |
| } |
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/vk/draw.c
Outdated
| uint32_t sfactor = fixup_blend_factor_for_surface( | ||
| GET_MASK(pgraph_reg_r(pg, NV_PGRAPH_BLEND), | ||
| NV_PGRAPH_BLEND_SFACTOR), | ||
| &pg->surface_shape); | ||
| uint32_t dfactor = fixup_blend_factor_for_surface( | ||
| GET_MASK(pgraph_reg_r(pg, NV_PGRAPH_BLEND), | ||
| NV_PGRAPH_BLEND_DFACTOR), | ||
| &pg->surface_shape); |
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.
Similar to the change in gl/draw.c, you can cache the value of pgraph_reg_r(pg, NV_PGRAPH_BLEND) to improve readability and avoid redundant register reads.
const uint32_t blend_reg = pgraph_reg_r(pg, NV_PGRAPH_BLEND);
uint32_t sfactor = fixup_blend_factor_for_surface(
GET_MASK(blend_reg, NV_PGRAPH_BLEND_SFACTOR),
&pg->surface_shape);
uint32_t dfactor = fixup_blend_factor_for_surface(
GET_MASK(blend_reg, NV_PGRAPH_BLEND_DFACTOR),
&pg->surface_shape);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.
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.
Pull request overview
This PR fixes an issue with destination alpha (DstAlpha) blending on surface formats that have forced alpha bits (e.g., X8R8G8B8 formats where "X" represents constant/unused alpha bits). The fix remaps DstAlpha blend factors to appropriate alternatives when the target surface format has forced alpha values.
Changes:
- Adds a new
fixup_blend_factor_for_surface()function to handle blend factor remapping for special surface formats - Updates both GL and Vulkan rendering backends to apply the blend factor fixup before using the factors
- Minor whitespace cleanup in GL draw code
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hw/xbox/nv2a/pgraph/surface.c | New file implementing the blend factor fixup logic for surfaces with forced alpha bits |
| hw/xbox/nv2a/pgraph/surface.h | Adds function declaration and includes for the new fixup function |
| hw/xbox/nv2a/pgraph/vk/draw.c | Applies blend factor fixup before creating Vulkan pipeline |
| hw/xbox/nv2a/pgraph/gl/draw.c | Applies blend factor fixup before setting OpenGL blend function, includes whitespace fix |
| hw/xbox/nv2a/pgraph/meson.build | Adds surface.c to the build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hw/xbox/nv2a/pgraph/surface.c
Outdated
| if (blend_factor == NV_PGRAPH_BLEND_SFACTOR_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_SFACTOR_ONE; | ||
| } | ||
| if (blend_factor == | ||
| NV_PGRAPH_BLEND_SFACTOR_ONE_MINUS_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_SFACTOR_ZERO; | ||
| } | ||
| if (blend_factor == NV_PGRAPH_BLEND_DFACTOR_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_DFACTOR_ONE; | ||
| } | ||
| if (blend_factor == | ||
| NV_PGRAPH_BLEND_DFACTOR_ONE_MINUS_DST_ALPHA) { | ||
| return NV_PGRAPH_BLEND_DFACTOR_ZERO; | ||
| } |
Copilot
AI
Jan 15, 2026
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.
The function checks both SFACTOR and DFACTOR constants for the same blend_factor parameter. While this works because the numeric values are identical (DST_ALPHA=6, ONE_MINUS_DST_ALPHA=7 for both), it's confusing and could be error-prone if the constants ever diverge. Consider simplifying by checking numeric values or documenting this assumption clearly. For example, you could add a comment explaining that SFACTOR and DFACTOR constants share the same numeric values.
9fbb60a to
e211bf4
Compare
This remaps DstAlpha to alternate source/destination factors when performing blending on target surfaces with forced alpha bits (e.g., NV097_SET_SURFACE_FORMAT_COLOR_LE_X8R8G8B8_Z8R8G8B8). Fixes xemu-project#2473 Tests: https://github.com/abaire/nxdk_pgraph_tests/blob/de548172ac3166f03f3a75999829ba17aade73aa/src/tests/blend_surface_tests.cpp#L249 HW results: https://abaire.github.io/nxdk_pgraph_tests_golden_results/results/Blend_surface/index.html
e211bf4 to
77eec63
Compare


This remaps DstAlpha to alternate source/destination factors when performing blending on target surfaces with forced alpha bits (e.g., NV097_SET_SURFACE_FORMAT_COLOR_LE_X8R8G8B8_Z8R8G8B8).
Fixes #2473
Tests: https://github.com/abaire/nxdk_pgraph_tests/blob/de548172ac3166f03f3a75999829ba17aade73aa/src/tests/blend_surface_tests.cpp#L249
HW results: https://abaire.github.io/nxdk_pgraph_tests_golden_results/results/Blend_surface/index.html
PR results: https://abaire.github.io/xemu-dev_pgraph_test_results/fix_2473_prevent_alpha_updates_for_forced_value_surfaces/index.html