E6x - add pattern loop

[mweitzel]

Add pattern loop E6x

I'm hesitant to submit this PR for a few reasons, but I want to start the discussion:

• it references a global player object from within the eff_t0_e function
• it required changing the logic in nextRow
• it adds multiple variables to the top level of the channel object

Would you prefer effects set a flag on player or player read through channels and look for effects flags?

If it stays this way, should I clear the goto_row variable in stop or init?

I'm not too familiar with this file format, so if there's another way you'd prefer this implemented I'd happily make changes to the PR.

[mweitzel]

5

[mweitzel]

Super interesting.

It might not be necessary, but it does avoid the current behavior in eff_t0_d where the player displays the target - 1 as soon as the effect is interpreted, instead of the one currently being played. It also makes it much easier to avoid bugs with multiple e or d calls on the same row.

Attached is an example .xm file (triple-d.xm.zip) with a sequence of 3 patterns. A row on the first pattern has three d calls. jsxm currently interprets all three, skipping the other patterns and returning to the first. MilkyTracker interprets them all, but with the effects of only the last holding it proceeds to the next pattern.

Its also worth noting this comment in the openmpt docs:

E60 Pattern Loop Start — Marks the start of a pattern loop. Note: One of the most infamous Fasttracker 2 bugs concern the handling of this command: When E60 is used on pattern row x, the following pattern also starts from row x instead of the beginning of the pattern. This can be avoided by placing a D00 effect on the last row of the pattern in which E60 was used. This "infamous Fasttracker 2 bug" was re-implemented in other trackers (for consistency?)

If both e and d wrote to a hijack_next_row or goto_row on player, the last (furthest right) call would stick, and the behavior would be consistent with other trackers.

I'd be happy to provide test cases and switch the logic for d as well.

[a1k0n]

Sorry, somehow I forgot to reply to this earlier.

Yes, I guess this makes sense, so D00 should be fixed to also use goto_row. I don't love the goto_row idea; maybe it would be better to use next_row which is initially set to cur_row + 1 and then the player loop would just do cur_row = next_row at the beginning of nextRow(). But it would still be hacky to jump to the next pattern, etc. And it's not really very different from goto_row.

So, how about this... can you fix D00 to be consistent? Then I'll merge.

Thanks again for the feedback and the PR.