-
-
Notifications
You must be signed in to change notification settings - Fork 312
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: Fix blend tests #1901
base: master
Are you sure you want to change the base?
nv2a: Fix blend tests #1901
Conversation
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.
@abaire Should we create more test cases for different RGB/alpha scaling factors and for enabled alpha test?
Please do, ideally there would at least be tests for all of the blend equations https://github.com/XboxDev/nxdk/blob/1edc29f849799e21ec8c2cab27f82088f9112629/lib/pbkit/nv_regs.h#L209
I've also been working on https://github.com/abaire/xemu-nxdk_pgraph_tests_results which includes tools to generate diffs against test results for older versions of xemu, which might be useful in regression testing.
hw/xbox/nv2a/pgraph/pgraph.c
Outdated
case NV097_SET_BLEND_EQUATION_V_FUNC_ADD_SIGNED: | ||
equation = 5; break; |
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.
Have you checked this against the register value in hardware?
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.
You mean original hardware? unfortunately I can't since I don't own one, I actually made the calculations on paper and got that when using that src scaling factor, a sum turns into a reverse subtract and vice versa, +/- an offset equal to src squared. Does that make sense?
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.
Yes, Xbox hardware.
If you can't test the HW behavior I suggest you find someone who can check before changing register values.
I don't entirely follow your explanation; it looks like this change is made without any regard to the source factor (which it should be, I don't believe that SET_BLEND_EQUATION
triggers a draw, so the order of setting the equation versus source/destination factors is not guaranteed).
I also share your concern that the test suite itself isn't broad enough. I don't recall whether the textures in #996 were signed or unsigned but it looks like the test is just checking for use of the signed blends with an unsigned texture. It would be good to expand the test and verify that this change matches HW for a variety of inputs. If it does, a better fix might be to swap the relevant entries in pgraph_blend_equation_gl_map with an explanation as to why they should be mismatched versus the nv2a/pgraph values (and a note explaining that signed channels would be handled separately in the shader).
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.
Yes, I'd better change the entries in pgraph_blend_equation_gl_map and pgraph_blend_equation_vk_map instead of here.
Sorry, I mean changing ONE
for ONE_MINUS_SRC_COLOR
in src factor in addition to this change. Without this swap, the tests are inverted.
Also, if the issue you're trying to solve is adding support for signed textures, https://github.com/abaire/nxdk_pgraph_tests_golden_results/wiki/Results-Texture_signed_component_tests was specifically written to cover them. I recall that at least Ninja Gaiden Black's water textures are signed (see #587) and the HW has the ability to treat each channel as signed or unsigned independently. |
I suspect the key is to emulate the signed equations by remapping the scaling factors for those specific equations we don't have in OGL and Vulkan. Would it be possible to implement these equations from shader?
No, actually there is a dev branch that adresses this and fixes all of those tests in that commit...but the ones with signed blending equations. In fact, this branch sets an unimplemented flag for those in another commit. Would you try it out and see if you find some clues? It also does not fix the blend tests adressed in this PR, that's why I opened it. These are most likely two different issues. |
Yes-ish, I think I had planned to either try to rotate channels in the shader or preprocess textures on the CPU before being uploaded.
I would suggest you double check with the author of that PR and make sure that swapping the operations isn't going to break their fix. |
I have, and they do not break each other. The results for signed equations + signed textures do differ, but they don't get close to HW anyway. Edit: Also, the equations + signed textures tests do not render when running them one by one. There's only the background to see. |
Please file an issue against the nxdk_pgraph_tests project, it's possible that there's some bleedover between the tests due to state not being fully reset between tests. It's also possible that there's a state bleed issue in xemu, but it is easier to check in the test program first.
Rather than speculating (which makes it harder to review the PR), you can just log what is being sent via the pushbuffer by running the game in xemu and using debug output to verify whether it's using the signed blend operations at all, and also whether it's using signed channels for the relevant textures, since that would not be addressed by your change. I have a branch https://github.com/abaire/xemu/tree/work that adds a bunch of debugging stuff to xemu to make this easier (you can view the log of the commits there to see what they do or look at my various debug/* branches). You can set As an FYI: in the future if you're creating new tests, the above method to dump pgraph events coupled with https://github.com/abaire/nv2a_to_pbkit will give you a headstart on reproducing interesting events. |
I could not build your branch from source, but I printed the commands using the monitor and got some things. I'm leaving the snapshots for future info. (forelast capture) Edit: first two work like HW, last two don't. |
You'll want to look at the entire frame's output for possible signed texture filter use (sometimes you need to go back even farther, but I'd guess Halo 2 doesn't use signed textures everywhere). It's interesting that |
Cool, might be worth fixing in this PR as well then, if it turns out to be straightforward. I would expect that any clamping D3D might do for blending would happen before it gets down to the pushbuffer level, so unless there's an interesting undocumented command or register value in play (there certainly is not one in the nxdk_pgraph_tests) I don't think that's something to worry about. My first guess would be that there's a bug in xemu, but I suppose it is possible that OpenGL is doing something; can you also check Vulkan to see if it has the same behavior? |
Hadn't noticed you had made further changes in the tests. Yes, behavior is the same in OGL and Vulkan: Could this be useful? maybe extending the blend equations would do the trick. More documentation |
Wonder if it's related to Half-Life 2's also missing shadows -- they'll render at a distance, but not up close. |
Overriding RGB src scale factor with ONE_MINUS_SRC_COLOR and swapping the signed blending equations make blend tests work like HW (signed reverse subtract might be a little too transparent). RGB dst factor is overriden to ONE, but it's set to ONE in the tests anyway.
@abaire Should we create more test cases for different RGB/alpha scaling factors and for enabled alpha test?
https://github.com/abaire/nxdk_pgraph_tests_golden_results/wiki/Results-Blend_tests


PR
Removes stretched shadows in Ed, Edd & Eddy: The Mis-Edventures (OGL issue only). There are still no shadows.