Envelope fix #63
Envelope fix #63
Conversation
|
@AliceLR please have a look when you can. |
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?
I think this is another thing that can vary significantly between formats (maybe @sagamusix can confirm...). |
|
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 don't have tests handy but IIRC IT in instrument mode will disagree with that due to how the instrument to sample mapping works. |
|
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. |
|
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. |
|
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? |
|
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. |
|
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: 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. |
|
Rebased this to current master. Reviews? Comments? |
|
@AliceLR: What is your final take in this? Reviews, comments? |
|
Just wanna say, that libxmp has the same problems. |
| if (inst>=mod->numins) break; /* safety valve */ | ||
| if (inst>=mod->numins) { | ||
| a->main.i=NULL; | ||
| a->main.s=NULL; | ||
| a->main.sample=-1; | ||
| break; /* safety valve */ | ||
| } |
There was a problem hiding this comment.
I don't have any useful examples right now but I think this might be completely format-dependent. Judging just on how libxmp does this (not necessarily correct, but generally pretty good):
- MOD: completely cuts the playing note.
- XM/other XM-likes: sets the volume to 0 but continues playing the current note.
- S3M/other S3M-likes: ignores the invalid instrument, continues playing the current note.
- IT (sample mode): sets the volume to 0 but continues playing the current note.
- IT (instrument mode): ignores the invalid instrument, continues playing the current note.
- MED: completely cuts the playing note.
If the above behavior descriptions are accurate, this change could be at the expense of S3M, XM, and IT accuracy.
| @@ -1613,21 +1719,18 @@ static int DoXMEffectL(UWORD tick, UWORD flags, MP_CONTROL *a, MODULE *mod, SWOR | |||
| dat=UniGetByte(); | |||
| if ((!tick)&&(a->main.i)) { | |||
| INSTRUMENT *i=a->main.i; | |||
| MP_VOICE *aout; | |||
| MP_VOICE *aout = &mod->voice[channel]; | |||
There was a problem hiding this comment.
Not sure what exactly is going on with MikMod's handling of channel assignment; is there a reason this was changed from a->slave?
There was a problem hiding this comment.
It is a while ago, since I made this patch, so I cannot exactly remember. I think it is because a->slave is null, since it is a XM effect, so I need to get the voice in a different way.
Prevent last used sampled in a channel to be played, if afterwards an illegal instrument is set. Now nothing is played. Combined with the next patch, these two fixes make Ebony Owl Netsuke.xm to play correctly.
Updated the envelopes, so there are two separate handlers. One for XM/IMF and one for IT. Almost totally rewrote the envelope handling, so it behaves like FastTracker II does, specially when used together with XM effect L. These two fixes make Ebony Owl Netsuke.xm to play correctly.
|
Rebased to mainstream with build fixes (no TRUE/FALSE..) |
|
Will rebase/patch this for #75 once it's in. |
PING? |
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