added per-sound channel individual volume core options (#166)

[eadmaster]

improvement suggestions are welcomed

[lgtm-com[bot]]

This pull request introduces 1 alert when merging ddd46640ac8c43e6b4072c5a8cd5f836deb9423c into 93e27debd228d65140f5107c6e7412b6c4abb3b6 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[jdgleaver]

@eadmaster This is a good idea, but this implementation won't work properly. You're replaced soChVol_ with soVol_, but the rest of the code still expects soVol_ to be used. Search for the function setSoVolume() - this is used to modify soVol_ dynamically at runtime, and when loading a save state. You need to dig a little deeper, and ensure that all instances of volume modification are properly accounted for :)

[eadmaster]

Actually i've tested my changes with a few games and the volume controls options are working fine as intended.

setSoVolume() is invoked only in 2 places: when loading a savestate, and inside Memory::nontrivial_ff_write.

Since i want to override the savestate volume in any case the 1st call can be ignored. (this also affects rewinding, which should not reset the custom set ch-volumes)

About the 2nd one i am not sure about its purpose, maybe some games use that for volume fading effects?

If this is the case then PSG::setSoChanVolume() should adjust soChVol according to the current value of soVol_, or these effects are lost (i may need some help with the bitwise math here).

[jdgleaver]

About the 2nd one i am not sure about its purpose, maybe some games use that for volume fading effects?

Yes, among other things - NR50 controls the left/right master volume, which can be set to zero in software, or ramped somewhat (you can see the later in e.g. Zelda Oracle of Ages). And because it can be set in software, you absolutely do have to restore the value when loading a save state :)

So basically, PSG::setSoChanVolume() should only record volume 'modifiers', which are then multiplied by soVol_ inside PSG::accumulateChannels().

One way you could do this is as follows:

• Each element of soChVol_ takes a number from [0,10]

• You call each channel update function in PSG::accumulateChannels() like this:

ch1_.update(buf, (soChVol_[0] * soVol_ * 0x1999999A) >> 32, cycles);

This would be the path of least resistance, since it means you wouldn't have to worry about changing the existing setSoVolume() calls.

Alternatively, you'd need to have 2 soChVol_ arrays - one to store the modifiers, and another to store the 'real' values. The modifiers would get set in check_variables(), and the real values would get set inside PSG::setSoVolume() (derived from the soVol_ obtained there). I'll leave it up to you to benchmark which method is faster :)

[eadmaster]

Thank you for your help, i'll definitively try the 1st solution since it looks cleaner.

[jdgleaver]

Yes, that's much better :)

You might just want to update the void setSoChanVolume(unsigned nr50, unsigned ch); function prototype in sound.h, though (nr50 -> percent)

[eadmaster]

all done :-)

I've tested the new core options with savestates and rewind and they are working fine.

EDIT: do you think it is safe to store soChVol_ as an unsigned short?

[jdgleaver]

EDIT: do you think it is safe to store soChVol_ as an unsigned short?

Ah, no - it should be 'safe' on most platforms, but you want to keep it as a 64 bit data type to avoid implicit casting

(i.e. you want everything in the (soChVol_[0] * soVol_ * 0x1999999A) >> 32 calculation to have the same data type, or you might see a tiny performance penalty)

So if you change that back, we should be all good :)

Also, could you squash your commits? There a quite a few now!

[eadmaster]

uhm, i am not sure if i've squashed them properly, sorry but i am new to git...

[jdgleaver]

uhm, i am not sure if i've squashed them properly, sorry but i am new to git...

No worries - everyone has to start somewhere :)

For this particular PR, I believe you just need to run:

git rebase -i fc79bb48a38093ace1d9667aa60ed644540b4137

...and replace 'pick' on the second and subsequent commits with 'squash' (that string is the hash of your first commit)

Once that's done, just force push to your branch:

git push --force origin master

For future reference, it's good practice to work in a feature branch, as described here: https://blog.scottlowe.org/2015/01/27/using-fork-branch-git-workflow/

The benefits of this are:

• It is trivial to rebase against current master - i.e.:

git checkout master
git pull upstream master
git checkout my_branch
git rebase master

• It is trivial to squash your commits into one - i.e. if you made 5 commits in your feature branch, you'd just do:

git rebase -i HEAD~5

...then follow the instructions here: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

If you work in master, then squashing commits can be bothersome (since you can end up with upstream commits in-between yours - that's not the case with this particular PR, but it happens a lot!)

[eadmaster]

i think i finally got it right: https://github.com/eadmaster/gambatte-libretro/commit/a0a0f84238ed84b3540f86d2b6f2943bc4aa8961

[jdgleaver]

Yes, that's all good now - ready for merging :)

[cmitu]

Looks like this PR broke sound on some system (#170).

I think 32 bit systems are getting a 0 sound level because of the bit operations in https://github.com/libretro/gambatte-libretro/pull/167/files#diff-6446e2d4750bfd3239e97aa0b1bfbd7274b03105941eabb36fe0ac3ef637970eR106-R109.

[jdgleaver]

I can confirm that there is no sound on 32 bit systems.

Clearly this code requires further testing. I'm afraid we can't have a bug of this magnitude in the core so close to the scheduled Christmas release - @twinaphex I think this will have to be reverted until after the new infrastructure work is completed.

[eadmaster]

Sorry about the regression, prolly it is due to an overflow on 32-bit systems.

I'll see if i can fix this, in the meanwhile i suggest you to revert to an older core build if you can (just download and install manually from the buildbot server).

[jdgleaver]

@eadmaster Yes, I didn't originally notice the 32 bit issue either - what an annoying bug!

We need to have a working core by Christmas Eve - if you can get a fix PR'd in time for that, then it's all good :)

If not, we'll revert temporarily, and revisit this after the holidays (either way is fine, to be honest)

[jdgleaver]

@eadmaster I believe this should fix the issue:

diff --git a/libgambatte/src/sound.cpp b/libgambatte/src/sound.cpp
index 1b0f3a0..2346723 100644
--- a/libgambatte/src/sound.cpp
+++ b/libgambatte/src/sound.cpp
@@ -103,10 +103,10 @@ namespace gambatte
       uint_least32_t *const buf = buffer_ + bufferPos_;

       std::memset(buf, 0, cycles * sizeof(uint_least32_t));
-      ch1_.update(buf, (soChVol_[0] * soVol_ * 0x1999999A) >> 32, cycles);
-      ch2_.update(buf, (soChVol_[1] * soVol_ * 0x1999999A) >> 32, cycles);
-      ch3_.update(buf, (soChVol_[2] * soVol_ * 0x1999999A) >> 32, cycles);
-      ch4_.update(buf, (soChVol_[3] * soVol_ * 0x1999999A) >> 32, cycles);
+      ch1_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[0] * soVol_) >> 32, cycles);
+      ch2_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[1] * soVol_) >> 32, cycles);
+      ch3_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[2] * soVol_) >> 32, cycles);
+      ch4_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[3] * soVol_) >> 32, cycles);
    }

    void PSG::generateSamples(unsigned long const cycleCounter, bool const doubleSpeed)

I am very busy with the new build infrastructure at the moment, however, and have not had time for proper testing. I will try to do this later...

[eadmaster]

ok, i'm adding an extra check to revert to the old formula if the volume is 100% to be sure...

[bslenul]

I now have sounds back with that diff on Windows 10 64bit, however it seems to only work properly with "volume %" at 100 or 50 (or well... 0 :p), any other value gives a terrible distorted sounds.

https://user-images.githubusercontent.com/33353403/102882209-563e6680-444e-11eb-9fc2-c3f67653ec0a.mp4

edit: Same results with #172

[eadmaster]

confimed! I'm writing another fix ... :-)

[eadmaster]

UPDATE: try my latest commit, should be fine on all systems now: https://github.com/eadmaster/gambatte-libretro/commit/987955b5813befae01b0d46a48f30ddb2e985c91

[bslenul]

No more sound distortions so it's much better 👍 But it sounds like depending on the "volume %" the sounds only come out from the left speaker, like 100 is fine but if I set the options to 80 I can hear most of the sounds only in my left ear and then if I set to 70 it's fine again.

https://user-images.githubusercontent.com/33353403/102996182-e8b03a00-4522-11eb-9c2c-4b96ce017a38.mp4

Check the video, it's especially obvious (even more if you use headphones) when I go from 80 to 70.

[inactive123]

Hi there, as per @jdgleaver's recommendation, I am reverting this PR for now until the Holidays are over, and then we can properly go over this and implement it in a way so everyone is happy and all platforms work. My bad.

[eadmaster]

sure, no problem, i am reopening the original issue: https://github.com/libretro/gambatte-libretro/issues/166