search

stohrendorf / ppplay

An old-fashioned Module Player
37 stars 2 forks source link

Rhythm changes, is comment accurate? #1

Closed gtaylormb closed 1 month ago

gtaylormb commented 3 months ago

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

stohrendorf commented 3 months ago

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.

gtaylormb commented 3 months ago

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!

stohrendorf commented 3 months ago

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.

gtaylormb commented 2 months ago

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.

stohrendorf commented 2 months ago

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.