-
Notifications
You must be signed in to change notification settings - Fork 556
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
SPU: Logic re-write, remove spu-advanced
toggle
#562
base: master
Are you sure you want to change the base?
Conversation
it would be helpful if those 2 fixes were in separate commits, so someone reading git log can figure out the relation |
Alright, no problem. I'll attempt a rebase (apologies in advance if I mess something up; I'm still not all that comfortable invoking |
Some FPS testing between builds from Github Actions before the Interpolation rework (48f5a82, testing master isn't fair since it had a few bugs) and this PR (477d4ec):
Testing on my Win10 machine with FX8320 + RX570 with JIT 18, OGL Display+3d renderer, 15bit Color, rest default. Tested on Pokemon Heartgold inside a building with fps based on just 1 loop (since I tested so many this time) and by observing FPS based on the counter (can't get any tools to report correct results). All Sync testing with N. Didn't test DirectSound since it gives lower FPS than XAudio and probably only useful for XP or lower. |
Before I mark this PR ready for review, I've got a couple of questions (mostly because I'm still trying to get used to contributing on projects instead of just working on my own ones). So one of the changes was "Add support for Catmull-Rom interpolation on Linux (macOS still doesn't have the option)" (b6da15a). Should this be split out into a separate "Add Catmull-Rom support for Linux/macOS" PR/issue (and hopefully have someone look over what needs to be done to incorporate it properly), or is it fine to keep it here? Not having that interpolation option won't break anything, so I figure it might be better to do it as a new PR. My other question was: I partially removed the advanced SPU option from the macOS code, but I know there's more places that I've missed. If the toggle is left in, then even though it won't actually do anything, the code won't break. Should I undo those changes and leave it for someone who actually knows what they're doing (or possibly even just undo the changes and raise it as an issue to be addressed at another time)? |
For this project our policy is break it and let someone else fix it. We can't wait for everyone to do everything perfectly or else nobody would ever do anything. |
Just do it -- I'll fix it later. @Aikku93: In the future, if you are working on unfamiliar code, then whenever you want to remove options and you see the flag variables controlled by accessor functions, it is better coding practice to hard code the getter function to YES/true or NO/false, and then comment out the flag variable assignment in the setter function. By doing this, you risk breaking absolutely nothing while still achieving your intent in removing the option. |
Alright, thanks for the help and tips. Appreciate it. |
@Aikku93: I'm currently reviewing the addition of the Catmull-Rom interpolation method and trying to get a handle on what it can do from the user's perspective so that I can write a proper tooltip for it. What's probably going to happen is that the Cocoa port's GUI is going to be updated to add Catmull-Rom at the same time as the Advanced SPU Logic option is removed. |
Hm, before this version ends up in |
2597a07
to
832772b
Compare
The UI still presents the option, but does nothing
Enforce delay for channel playback and adjust capture delay prior to FIFO buffering.
I've split up the single mega-commit into multiple ones, which should hopefully result in a clearer commit history. I've also more-or-less followed rogerman's advice re. the Cocoa port, and instead of removing all the associated code that I'm unfamiliar with, I've just commented out the line that actually writes to Please let me know if there's still more commit refactoring to do. I think I'm starting to get the idea of how commits and PRs are supposed to work, but I'm still getting there. |
This PR covers an almost complete re-write of all the SPU mixing logic (plus some miscellaneous bits and pieces related to it), which has been tested to achieve performance on-par with the "non-advanced" SPU logic from before my interpolation-related re-write.
Further improvements include:
I introducedwhere using capture-based echo effects would break in dual-SPU mode (this bug was present before my changes)ENABLE_DUMMY_SPU_CAPTURE
toggle in SPU.cpp, in which case capture will only generate silence)I've tried my best to remove all references to the "advanced SPU logic" toggle, but have only really been able to test on Windows at the moment, so other platforms might not be functional at the moment (I believe the macOS build in particular needs some more attention).
Special thanks to Rise of Seren for better performance profiling than I could get on my computer.