mikmod icon indicating copy to clipboard operation
mikmod copied to clipboard

Envelope fix

Open neumatho opened this issue 2 years ago • 14 comments

Two fixes in this pull request:

First fix will prevent last used sampled in a channel to be played, if afterwards an illegal instrument is set. Now nothing is played.

The second fix is a big one. Almost totally rewrote the envelope handling, so it behave like FastTracker II does, specially when used together with XM effect L. Inspiration to this fix is from a translation of the original FastTracker II code to C, which can be found here: Replayer source to FT II Clone. It is special the updateChannel() function I looked at.

These two fixes make Ebony Owl Netsuke.xm to play correctly.

Ebony Owl Netsuke.zip

neumatho avatar Nov 04 '21 15:11 neumatho

@AliceLR please have a look when you can.

sezero avatar Nov 04 '21 18:11 sezero

The second fix is a big one. Almost totally rewrote the envelope handling

Changes that affect multiple formats like this with no regression testing are, as sezero can confirm, a significant contributor to why MikMod's format accuracy has been in such a sad state for a long time. How many modules have you tested this patch against? How many are XM? IT? IMAGO Orpheus?

First fix will prevent last used sampled in a channel to be played, if afterwards an illegal instrument is set. Now nothing is played.

I think this is another thing that can vary significantly between formats (maybe @sagamusix can confirm...).

AliceLR avatar Nov 04 '21 22:11 AliceLR

There are significant differences between IT and XM style envelope handling, so just taking an XM player's code here is almost guaranteed to break other formats unless they were already broken to begin with.

I think this is another thing that can vary significantly between formats (maybe @sagamusix can confirm...).

I don't have tests handy but IIRC IT in instrument mode will disagree with that due to how the instrument to sample mapping works.

sagamusix avatar Nov 04 '21 23:11 sagamusix

I tried to be very careful when doing this change, because I know it may broke something. Did a lot of debugging on multiple modules. I have tested with some modules, but not that many. Will test more. Ofcourse it would be good, if there is some real good testing modules in all formats where you can easily hear how it should sound and if it is broken.

I did not just copy the XM envelope function, but tried to apply what this did in some situation with the original MikMod code. The only difference I could see out of MikMod code between XM and IT, is that XM has one sustain point, while IT has two (begin/end). This part is kept in my changes. Are there other differences I need to be aware of?

And while writing this, there is one thing I need to check. I'm not sure if there is made interpolation when running between multiple sustain points. Will check that and commit any changes if needed.

neumatho avatar Nov 05 '21 06:11 neumatho

It seems there are some issues with IT. The strange thing is that my C# port and the original MikMod code do not sound similar, which is very strange. I'm looking at it.

neumatho avatar Nov 05 '21 21:11 neumatho

After many hours of analyse and debugging, I finally found why there are differences. It has nothing to do with this patch. In the module "A Life In Termoil.it", there is played a sequence in 4 voices at the beginning. In the original MikMod code, only the first 2 can be heard, even when the code parses them, while my port of MikMod all 4 can be heard. I then found out, that the last two voices are marked as surround channels (via S9x effect). If I disable surround (via DMODE flag), then all 4 channels are played in MikMod. So there seem to be an issue with surround mixing in MikMod.

Back to my patch. This patch does not sound correctly on IT modules, so I think I will make two separate envelope handlers for XM and IT. I then need to figure out exactly how it works. Do any of you know what IMAGO Orpheus uses? XM, IT or its own?

neumatho avatar Nov 06 '21 10:11 neumatho

In general no two trackers from that period are alike because nobody really wrote done in a fully detailed way how their engine works. However, Imago behaves more like FT2 than Impulse Tracker when it comes to envelope handling, so as long as mikmod doesn't have any specialized handling for envelopes in IMF yet, it's probably best to follow what XM does.

sagamusix avatar Nov 06 '21 18:11 sagamusix

I have now separated the envelope handlers into two different methods. The main difference between the XM and IT way of handling envelopes, is that XM process the first point before it checks for sustain/looping, while IT do it for each tick. Lets take an example. We have a simple envelope of two points:

Pos  Value
 0    256
12      0

The loop has both a start and end on point 0. For XM, the volume will start at 256 and then fade down to 0 in 12 ticks and move to the next point. Because the new point is after the loop, the loop is ignored, so it keep the volume at 0.

For IT, it will keep the volume at 256 the whole time, because it keep looping at the first point.

For the illegal instrument fix, MikMod has always made a test for out-of-bounds instruments, it just forgot to clear some variables, which made it play the last used sample on that sample, whatever that was. So the fix, is just to clear those variables.

neumatho avatar Nov 08 '21 17:11 neumatho

Rebased this to current master. Reviews? Comments?

sezero avatar Mar 02 '22 09:03 sezero

@AliceLR: What is your final take in this? Reviews, comments?

sezero avatar Jan 10 '23 05:01 sezero

Just wanna say, that libxmp has the same problems.

neumatho avatar Jan 10 '23 15:01 neumatho

Rebased to mainstream with build fixes (no TRUE/FALSE..)

sezero avatar Jan 19 '23 09:01 sezero

Will rebase/patch this for #75 once it's in.

AliceLR avatar Feb 09 '23 03:02 AliceLR

Will rebase/patch this for #75 once it's in.

PING?

sezero avatar Sep 09 '23 15:09 sezero