MOD retrigger effect is broken.

[AliceLR]

The .MOD (and .XM, .MED, and whatever else uses it...) retrigger effect currently does not initialize the retrigger timer, meaning it will always retrigger on the second tick of a given row.

Test file: in Protracker 3.15, libxmp, OpenMPT, this plays one snare hit, then three evenly spaced snare hits. In MikMod, this plays one snare hit, then four unevenly spaced snare hits. Speed=24, uses E98.

retrig.mod.zip

The only relevant changelog entry I could find re: this was for libmikmod-3.1.10:

- ProTracker effect E9 (Retrig) was not played correctly.

But weirdly, the only change to this effect between libmikmod-3.1.9 and libmikmod-3.1.10 is to prevent it from triggering on tick 0:
libmikmod-3.1.9:

                case 0x9: /* retrig note */
                        /* only retrigger if data nibble > 0 */
                        if (nib) {
                                if (!a->retrig) {
                                        /* when retrig counter reaches 0, reset counter and restart
                                           the sample */
                                        if (a->period) a->kick=KICK_NOTE;
                                        a->retrig=nib;
                                }
                                a->retrig--; /* countdown */
                        }
                        break;

libmikmod-3.1.10:

        case 0x9: /* retrig note */
                /* do not retrigger on tick 0, until we are emulating FT2 and effect
                   data is zero */
                if (!tick && !((flags & UF_FT2QUIRKS) && (!nib)))
                        break;
                /* only retrigger if data nibble > 0, or if tick 0 (FT2 compat) */
                if (nib || !tick) {
                        if (!a->retrig) {
                                /* when retrig counter reaches 0, reset counter and restart
                                   the sample */
                                if (a->main.period) a->main.kick=KICK_NOTE;
                                a->retrig=nib;
                        }
                        a->retrig--; /* countdown */
                }
                break;

The worst part of this is that I loaded a test .MOD into Protracker 3.15 and it does retrigger on tick 0.

Test file: this plays one snare, then plays a single snare note with an E9A effect on the note line and an E9A effect on the following line. In Protracker, the two lines with E9A both play on ticks 0, 10, and 20. libxmp, MikMod, OpenMPT, and MPT 1.16 all do something else(!).

retrig2.mod.zip

The first .MOD is trivial to fix (and I have a working patch) but I suspect finding the "correct" behavior for the second .MOD could be a mess, which is why this is an issue and not a PR yet.

diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index e346ceb..4029eca 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -987,10 +987,13 @@ static void DoEEffects(UWORD tick, UWORD flags, MP_CONTROL *a, MODULE *mod,
 		}
 		break;
 	case 0x9: /* retrig note */
-		/* do not retrigger on tick 0, until we are emulating FT2 and effect
+		/* do not retrigger on tick 0, unless we are emulating FT2 and effect
 		   data is zero */
-		if (!tick && !((flags & UF_FT2QUIRKS) && (!nib)))
-			break;
+		if (!tick) {
+			if (!(flags & UF_FT2QUIRKS) && (!nib))
+				break;
+			a->retrig = 0;
+		}
 		/* only retrigger if data nibble > 0, or if tick 0 (FT2 compat) */
 		if (nib || !tick) {
 			if (!a->retrig) {

edit: I should note that the 3.1.9 behavior is also wrong because it would result in the retrigger timer carrying over between lines, which it shouldn't. To get the behavior of this patch in line with the Protracker 3.15 behavior, the change would probably be to initialize a->retrig to 0 instead of nib. The FT2 quirks check is correct; E90 does retrigger exactly once in Fasttracker 2 but does not retrigger at all in Protracker.

edit 2: initializing a->retrig to 0 was actually required anyway so I updated the patch. It does make the playback for the second test identical to Protracker.

edit 3: in FT2 the E9As don't play the first note unless there's a note on the line. :(
Updated patch to take that into account:

diff --git a/libmikmod/playercode/mplayer.c b/libmikmod/playercode/mplayer.c
index e346ceb..6f4e812 100644
--- a/libmikmod/playercode/mplayer.c
+++ b/libmikmod/playercode/mplayer.c
@@ -987,10 +987,16 @@ static void DoEEffects(UWORD tick, UWORD flags, MP_CONTROL *a, MODULE *mod,
 		}
 		break;
 	case 0x9: /* retrig note */
-		/* do not retrigger on tick 0, until we are emulating FT2 and effect
-		   data is zero */
-		if (!tick && !((flags & UF_FT2QUIRKS) && (!nib)))
-			break;
+		/* Protracker: retriggers on tick 0 first; does nothing when nib=0.
+		   Fasttracker 2: retriggers on tick nib first, including nib=0. */
+		if (!tick) {
+			if (flags & UF_FT2QUIRKS)
+				a->retrig=nib;
+			else if (nib)
+				a->retrig=0;
+			else
+				break;
+		}
 		/* only retrigger if data nibble > 0, or if tick 0 (FT2 compat) */
 		if (nib || !tick) {
 			if (!a->retrig) {

[sagamusix]

Please note that the various versions of ProTracker do not share the same replayer code and may differ in details when it comes to effect reproduction. ProTracker 2.3d is the most-used version and very few people used ProTracker 3 in comparison, so you should always compare MOD behaviour against ProTracker 2.3d (or bubsy's cross-platform clone of that version).

Retrigger in particular is one of those quirky effects that are handled vastly differently between different trackers (as you noticed with FT2). If you want to fix it, I suggest to take a very surgical approach and only fix it one format at a time.

[AliceLR]

That is good to know. I just tried a similar test in ProTracker v2.3d and I got the same result as in ProTracker 3 (though I found two versions called "ProTracker v2.3d", I'm not sure which is correct). I'd definitely like to limit these sorts of fixes/tweaks on per-format basis but in this case the current implementation is plain wrong regardless of format. I'd like to at least correct that first. The tracker-specific behavior probably doesn't matter much for that part of things...

For reference though, formats aside from MOD/S3M with retrigger currently are implemented with:

• DSMI AMF: S3M Qxx
• ASYLUM AMF: ???

        if (effect == 0x1b) {
                return 0; /* UniEffect(UNI_S3MEFFECTQ,dat) ? */
        }

• DSM: PT E9x
• FAR: PT E9x (for now. I want to give this format a close look eventually because other effect conversions are definitely wrong!)
• GDM: S3M Qxx
• IMF: S3M Qxx
• IT: S3M Qxx
• MED: currently being fixed independent of this issue and the reason I found this bug in the first place :(
• MTM: PT E9x
• ULT: PT E9x
• XM: PT E9x and S3M Qxx (for Rxx)

edit: clarification.