-
Notifications
You must be signed in to change notification settings - Fork 79
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
added per-sound channel individual volume core options (#166) #167
Conversation
This pull request introduces 1 alert when merging ddd4664 into 93e27de - view on LGTM.com new alerts:
|
@eadmaster This is a good idea, but this implementation won't work properly. You're replaced |
Actually i've tested my changes with a few games and the volume controls options are working fine as intended.
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 |
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, One way you could do this is as follows:
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 Alternatively, you'd need to have 2 |
Thank you for your help, i'll definitively try the 1st solution since it looks cleaner. |
Yes, that's much better :) You might just want to update the |
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 |
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 So if you change that back, we should be all good :) Also, could you squash your commits? There a quite a few now! |
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:
...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:
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:
...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!) |
fixes for max and 0 volumes reworked accumulateChannels to preserve volume effects minor fixes reverted soChVol_ to long
i think i finally got it right: |
Yes, that's all good now - ready for merging :) |
Looks like this PR broke sound on some system (#170). I think 32 bit systems are getting a |
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. |
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). |
@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) |
@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... |
ok, i'm adding an extra check to revert to the old formula if the volume is 100% to be sure... |
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. 2020-12-22.12-05-41.mp4edit: Same results with #172 |
confimed! I'm writing another fix ... :-) |
UPDATE: try my latest commit, should be fine on all systems now: |
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. 2020-12-23.13-28-45.mp4Check the video, it's especially obvious (even more if you use headphones) when I go from 80 to 70. |
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. |
sure, no problem, i am reopening the original issue: #166 |
improvement suggestions are welcomed