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

VertexLoader: Fix loading tangent/binormal caches with NormalIndex3 #11432

Merged

Conversation

Pokechu22
Copy link
Contributor

@Pokechu22 Pokechu22 commented Jan 12, 2023

Fixes the generic nightly builder (see #10808 (comment)).

I didn't catch this because I did my testing using COMPARE_VERTEXLOADERS, which did not check the various caches. The issue was the software vertexloader not writing to the tangent/binormal caches, but this was obscured since the JIT vertexloader did load to them.

Additionally, COMPARE_VERTEXLOADERS wouldn't be visible in unit tests since logging wasn't enabled. I changed that to asserts, and also changed the test system to register a custom message alert handler (as otherwise the assertion popup would appear on the msvc buildbot and never close, forcing the 20-minute timeout, and on other platforms a failed dolphin assertion wouldn't be treated as a test failure).

@Pokechu22 Pokechu22 force-pushed the generic-vertex-loader-test-error branch 2 times, most recently from 8e55758 to 0de9145 Compare January 12, 2023 23:08
@Pokechu22 Pokechu22 changed the title VertexLoader: Fix failing tests on generic builder VertexLoader: Fix loading tangent/binormal caches with NormalIndex3 Jan 12, 2023
@Pokechu22 Pokechu22 force-pushed the generic-vertex-loader-test-error branch 2 times, most recently from 9ccd6c1 to fd408ad Compare January 13, 2023 00:09
@shuffle2
Copy link
Contributor

not very related but, dolphin's version of gtest hasn't been updated in a while, any idea if updating it would improve anything?

@Pokechu22 Pokechu22 force-pushed the generic-vertex-loader-test-error branch from fd408ad to 2e3d105 Compare January 13, 2023 22:02
This prevents a failed assertion from hanging on the MSVC buildbots.
Logs don't show up in unit tests, and since this is debugging functionality (though not enabled for tests by default) it's better to do it this way.
This is the behavior in the x64 and ARM64 vertex loaders. I don't know if it makes sense (the whole skipped vertex system seems jank, but several games behave incorrectly without it).
@Pokechu22 Pokechu22 force-pushed the generic-vertex-loader-test-error branch from 2e3d105 to 3910bdd Compare January 13, 2023 23:38
@Pokechu22
Copy link
Contributor Author

I don't think updating it would help anything for this PR, but I'll look into doing a separate PR as it probably would be easy to make it a submodule.

This PR now passes everything when built with COMPARE_VERTEXLOADERS (2e3d105), and also without it. So it should be good to go.

@Pokechu22 Pokechu22 marked this pull request as ready for review January 13, 2023 23:42
@delroth delroth merged commit a7d1683 into dolphin-emu:master Jan 17, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants