ekeeke
commented
3 years ago It looks more like an error in your initial implementation as you don't use a specific state for sustain but instead apparently use a 'DECAY' switch case to implement both decay and sustain states.
As the envelope level continues to increase during sustain state, it will logically becomes greater than SL at some point (unless zero rates are used) but, obviously, you should still use Sustain rate as long as you are in Sustain state. However, your initial implementation sets the envelope rate based on envelope level instead of EG state, and forced back the use of decay rate as soon as envelope level became greater than SL in sustain state, which is incorrect.
I think a more correct implementation would be to use a separate state for sustain and only checks SL during decay state, to trigger transition to sustain state.
As for the use of equality comparator, I know it was verified on OPLL and, although I am not sure for OPN2, it would make more sense that the same costless design was reused between chips.
jotego
I had originally coded this transition with a greater than comparison with the sustain level. When I saw your code
https://github.com/nukeykt/Nuked-OPN2/blob/64704a443f8f6c1906ba26297092ea70fa1d45d7/ym3438.c#L662
I changed that to an equal. I thought it was a bit dangerous to make it an equal, but didn't find issues straightaway and I assumed that the original hardware worked that way.
I was reported yesterday that the jump sound effect in Bubble Bobble produced a noise in the last update, which used an equal comparison instead of greater than. Moving back to the old comparison fixed the bug.
It might be a problem of my particular implementation, that requires the > comparison. I don't know. I'm just writing it here in case your code should be a > too.