libretro / Genesis-Plus-GX

An enhanced port of Genesis Plus - accurate & portable Sega 8/16 bit emulator
Other
75 stars 72 forks source link

Add per-sound channel individual volume core options #223

Closed eadmaster closed 3 years ago

eadmaster commented 3 years ago

This is meant to "fix" games having unbalanced SFX vs the BGM.

Will need to add separate options for the PSG and Yamaha chips.

eadmaster commented 3 years ago

I've checked the code and I am going to try implementing this myself starting with the SMS PSG. Simply adding an on/off switch for the singular channels seems trivial, but i'll try with proper volume scaling as well. If you have any suggestion please post here.

eadmaster commented 3 years ago

UPDATE: first addition made for the PSG chip. The noise channel 3 seem to work as toggle only though (any suggestion?)

eadmaster commented 3 years ago

UPDATE2: added options for the MegaDrive/Genesis FM chip too!

The Master System FM (YM2413) chip is still left, but this has a lower priority for me.

Please review the code in my fork before i sumbit the first PR.

eadmaster commented 3 years ago

UPDATE3: still need some help to scale the volume of the noise channel on the PSG chip.

ekeeke commented 3 years ago

The reason it does not work properly for PSG channels is that, since config.psg_ch_volumes[i] are unsigned int, not float, setting it to a value inferior to 100 and dividing it by 100 will always gives you 0 as a result. So basically, anytime you set one psg channel below 100%, you are actually muting it.

The modified code in psg_config should be corrected like this for it to work as you expect: psg.chanAmp[i][0] = ((preamp * config.psg_ch_volumes[i]) / 100) * ((panning >> (i + 4)) & 1);

so that the division occurs after multiplying individual channel volume by preamp value. EDIT: actually the same should be done for the FM core modifications, as your use of parentheses cause the division to occur before the multiplication.

Also, forcing delta value update whatever the current output level when your channel volume is below 100 is incorrect and should be removed for correct emulation behavior.

Finally, if you want this to be merged upstream, you will need to isolate and enclose your core modifications using #IFDEF USE_PER_SOUND_CHANNELS_CONFIG / #ENDIF for example, so the core still compile fines in non-libretro ports that don't have your additional config data defined and don't need this hack.

eadmaster commented 3 years ago

So basically, anytime you set one psg channel below 100%, you are actually muting it.

For some reason the tone PSG channels were scaling correctly the volume, and since i couldn't figure out the reason why the noise channel didn't i started making experiments...

Finally, if you want this to be merged upstream, you will need to isolate and enclose your core modifications using #IFDEF USE_PER_SOUND_CHANNELS_CONFIG / #ENDIF for example, so the core still compile fines in non-libretro ports that don't have your additional config data defined and don't need this hack.

Actually my initial idea was to init the volume arrays with 100 values and then do the volume scaling only if the frontend overrides these values instead of using prepoc vars.

Btw i've done all the changes you suggested and everything seems to work fine. Since you already confirmed there should be no side-effects with these changes i'll send a PR soon.

eadmaster commented 3 years ago

UPDATE: i've just pushed a first implementation of the SMS FM (YM2413) channel volume scaler, please review!

ekeeke commented 3 years ago

Please do not use LIBRETrO specific defines in core sourcecode, so that non-libretro ports can still support this hack if they want by setting specific define in their Makefile. Using a more generic #ifdef as initially proposed is a better long term solution. You only need to had -Dxxxxx in Makefile.libretro core defines, along with other core specific defines (like the one allowing cpu overclocking or sprites limit removal hacks)

EDITED: removed psg code modification proposal as it would not work

I didn't looked at your ym2413 core modifications yet.

eadmaster commented 3 years ago

Please do not use LIBRETrO specific defines in core sourcecode, so that non-libretro ports can still support this hack if they want by setting specific define in their Makefile. Using a more generic #ifdef as initially proposed is a better long term solution. You only need to had -Dxxxxx in Makefile.libretro core defines, along with other core specific defines (like the one allowing cpu overclocking or sprites limit removal hacks)

I see, it definitively makes sense, i am changing them now.

I am also adding a switch to hide all the new audio core options like snes9x has.

eadmaster commented 3 years ago

Please confirm me my latest changes are ok, so i can submit a PR for this...

eadmaster commented 3 years ago

nevermind, i've just sent the PR, comment there if you want.