ppplay icon indicating copy to clipboard operation
ppplay copied to clipboard

Rhythm changes, is comment accurate?

Open gtaylormb opened this issue 10 months ago • 5 comments

Hey Steffen, it's been a while! How are you?

I have been revisiting https://github.com/gtaylormb/opl3_fpga lately and wanted to update the rhythm operators to what you currently have, as it looks like things have changed in the last 9 years or so.

Is this comment valid? From the code inspection it appears the top cymbal and and snare drum are now only using the hi hat phase and none of their own. Is that correct or am I missing something? https://github.com/stohrendorf/ppplay/blob/3df08971d7c9584113f708c82edb836effc06e44/src/ymf262/operator.cpp#L77

gtaylormb avatar Mar 30 '24 13:03 gtaylormb

Oh dear, it's been quite a while, I believe I have forgotten nearly everything about the OPL3. Looking through the git history didn't help as I did some of the worst commits of my life around back then. I fear I can't help you here.

stohrendorf avatar Mar 31 '24 07:03 stohrendorf

I know and that's okay, it's been nearly a decade since I've looked at this stuff. Had some extra time lately between jobs so I decided to pick it up again for stuff I knew could be improved.

FYI I managed some fairly substantial logic reductions in the core and I believe the rhythm section now matches your code.

Hope you've been well!

gtaylormb avatar Mar 31 '24 16:03 gtaylormb

I just checked the code, and currently the high hat phase is also used by the top cymbal operator as well as the snare drum operator. I think this was done to reduce components as these three are usually noisy enough so that a shared phase is no issue, but that assumes the implementation is correct.

stohrendorf avatar Apr 01 '24 08:04 stohrendorf

Yeah it's interesting because in the old code the hi_hat was used together with the other operators, and now it's just used alone in replace of those operators. It wouldn't save any hardware, since those operators are still present but would just be unused if rhythm mode is turned on.

gtaylormb avatar Apr 05 '24 19:04 gtaylormb

I really don't know whether that change came from me fooling around, or from reading some stuff from the internet (but I suspect the latter). However. since https://github.com/stohrendorf/ppplay/blob/3df08971d7c9584113f708c82edb836effc06e44/src/ymf262/operator.cpp#L81 and https://github.com/stohrendorf/ppplay/blob/3df08971d7c9584113f708c82edb836effc06e44/src/ymf262/operator.cpp#L90 are the same, I suspect my thoughts back then had some ground truth. And I also remember trying to match the output to what I remembered when listening to HSC tracker.

stohrendorf avatar Apr 07 '24 11:04 stohrendorf