Closed aybe closed 5 months ago
Oh very nice! Many thanks for this!
Note that the bot's analysis of code is merely here as a general hint, and doesn't need to be taken as gospel. It's fine to leave some complexity in.
Another lesson learned for me today, getting it done was troublesome, complying to the rules even more! 🤣 I don't do C++ on a daily basis, so it's been an exercise and I should have paid attention to the analysis!
Honestly, I find the maximum parameters for a method to be 4 is a bit extreme, 6 would have been better. But it's good that you've put the analysis in place as it forces people to re-think over and over. Result is that I simplified it overall (at expense of time), there's still room for improvements I guess, lol.
I'll finish it up as it's a really interesting challenge and definitely a good lesson for me!
The more important tests are flagged as "Required" mind you :) And there's this one currently failing on MacOS:
src/spu/debug.cc:192:22: error: no member named 'all_of' in namespace 'std::ranges'
if (std::ranges::all_of(channels, [](const SPUCHAN& c) {
~~~~~~~~~~~~~^
The C++ compiler on MacOS isn't the greatest, I have no idea what Apple is doing exactly, but it's far from being conformant. This is why there's this file here:
https://github.com/grumpycoders/pcsx-redux/blob/main/src/support/polyfills.h
It might be possible to add a reference implementation from https://en.cppreference.com/w/cpp/algorithm/ranges/all_any_none_of into the MacOS polyfill and use it instead.
any_of
, all_of
should be fixed now, hopefully, just did like for for_each
.
You're right, it's not worth bothering much about the analysis, I fixed them but introduced two minor ones... That'll do for now.
Attention: Patch coverage is 0.33670%
with 296 lines
in your changes are missing coverage. Please review.
Project coverage is 9.94%. Comparing base (
be89961
) to head (1e635d0
). Report is 1 commits behind head on main.:exclamation: Current head 1e635d0 differs from pull request most recent head c24ce59
Please upload reports for the commit c24ce59 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
src/spu/debug.cc | 0.00% | 294 Missing :warning: |
src/spu/freeze.cc | 0.00% | 1 Missing :warning: |
src/spu/spu.cc | 50.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I've done a minor change to the code for hiding functions which were otherwise global, but otherwise, looks good.
I used it yesterday to debug the game I wanted to, it worked fine and haven't found any issues.
The mono/solo buttons and channel name were so helpful in understanding what the game actually does with pitch.
I've done a minor change to the code for hiding functions which were otherwise global, but otherwise, looks good.
Yes, I forgot about that one, thank you! 🥳
Hi!
I've been trying to debug the sound of a PSX game, figured out I could improve the SPU debugger somehow, here we are :)
In the beginning I slowly improved the initial design but wasn't satisfied seeing only one channel info at a time.
Tried going the nested tables route, and after much tweaking, it looks pretty good IMO.
Features:
Full view:
Customized view:
Notes:
ImGui is quite finicky when it comes to sizing columns and nested tables...
I tried to let it manage columns but it ended up very frustrating for the end-user!
After much trouble, I opted for simple yet effective fixes to get around these issues:
Overall I'm satisfied with it, hopefully you'll like it too.
Cheers!