Possible error in EG attack phase implementation ( incorrect step values for 11.1 to 11.3 attack rates)

[ekeeke]

I recently improved MAME YM2413 core EG implementation for attack phase, based on reverse engineering notes from andete (https://www.smspower.org/Development/YM2413ReverseEngineeringNotes2017-01-26) and when I double-checked my implementation against yours, it appeared that the steps patterns matched for all rates except attack rates 11.1, 11.2, 11.3.

Looking at the results given by your implementation, I believe there is a small error on your side as 11.2 and 11.3 resulting patterns are both identical and identical to 12.0 pattern, i.e resulting in not(env)/16 "decrement" on each sample, which seems incorrect.

Looking at your code (OPLL_EnvelopeGenerate function), I believe there is an error with the following code

            int32_t shift = chip->eg_rate_hi - 11 + chip->eg_inc_hi;
            if (chip->eg_inc_lo) {
                shift = 1;
            }

Indeed, for rates 11.x, this means the shift value depends on both chip->eg_inc_hi and chip->eg_inc_low value, which results in shift value being always set for rates 11.2 and 11.3 (as either chip->eg_inc_hi or chip->eg_inc_low are set on any given sample for these two ones).

I believe a more correct implementation would be
int32_t shift = (chip->eg_rate_hi > 11) ? (chip->eg_rate_hi - 11 + chip->eg_inc_hi) : chip->eg_inc_lo;
so that chip->eg_inc_hi is only taken in account for rates greater or equal to 12.0 (and chip->eg_inc_lo only for rates lower than 12.0), like it is the case for decay phases.

I quickly tested this implementation and, as expected, it results in exact same patterns as previous implementation except for rates 11.1 to 11.3 where it now results in expected patterns.

See the attached file for the sourcecode of the test program I used to test your implementation, which is basically your OPLL_EnvelopeGenerate function (with fixed algorithm as described above) called for 18 x N cycles, with chip->eg_rate fixed to tested rate and chip->eg_state / chip->eg_level forced to attack state and max attenuation on each cycle to check for the whole increment/decrement pattern. I also included the original results for 11.x and 12.0 rates, as well as the fixed 11.1 to 11.3 patterns.

opll_env.zip

Also note that you can remove the check for condition shift > 4 as this condition never occurs with your implementation, maximal value of chip->eg_rate_hi being 14 for this calculation to occur (when chip->eg_rate_hi = 15, chip->eg_maxrate is also set so there is no envelope update) so maximal value of shift is 4 anyways.

[nukeykt]

I've double checked die and indeed I made mistake. Great work 👍
Your fix looks good, will you send PR to fix this?

[ekeeke]

Sure, no problem.

[ekeeke]

Closed by #7