Fix handling of single-row loops

[bruceg]

In S3M files, at least, a pattern loop effect (SBn) without a preceding
start marker (SB0) repeats that single row. If a mod file has a single
row loop after a normal loop in the same pattern, this would cause an
infinite loop, since the first loop resets the later loop's counter, but
both loop to the same point.

This fixes the endless loop bug I reported on SourceForge

[sezero]

Thanks for this, I merged this in my fork (https://github.com/sezero/libmodplug.git)
The project seems dead for a while now, I wonder we will see any updates..

[Konstanty]

👍 There are quite a few changes to merge in...
The problem with some others is that they require extra variables, which break C++ ABI compatibility - unlike this one - which fixes and keeps ABI the same.

[sagamusix]

This patch is incorrect and should not be merged as-is. After a loop has finished playing, the loop start row should be set to the row following the loop end command, which just happens to be the same row as the row where the SB7 effect is placed in this particular example. Also, this behaviour is only correct for IT and S3M files, but the patch applies it to all formats. The correct way of handling this can be found in libopenmpt's Snd_fx.cpp (basically, instead of setting the loop start to 0, set it to m_nRow + 1 -- but only for IT and S3M files).

[sezero]

@sagamusix: So it should be a two lines addition as in the following, yes?

diff --git a/src/snd_fx.cpp b/src/snd_fx.cpp
index c7f8549..d424936 100644
--- a/src/snd_fx.cpp
+++ b/src/snd_fx.cpp
@@ -2118,11 +2118,16 @@ int CSoundFile::PatternLoop(MODCHANNEL *pChn, UINT param)
        if (param)
        {
                if (pChn->nPatternLoopCount)
                {
                        pChn->nPatternLoopCount--;
-                       if (!pChn->nPatternLoopCount) return -1;
+                       if (!pChn->nPatternLoopCount)
+                       {
+                               if (m_nType & (MOD_TYPE_S3M|MOD_TYPE_IT))
+                                       pChn->nPatternLoop = m_nRow + 1;
+                               return -1;
+                       }
                } else
                {
                        MODCHANNEL *p = Chn;
                        for (UINT i=0; i<m_nChannels; i++, p++) if (p != pChn)
                        {

[sagamusix]

That looks better indeed. I think you also have to take care that the loop start is reset on pattern transitions, and S3M technically only has one global nPatternLoop / nPatternLoopCount rather than per channel, but that's probably outside the scope of this pull request.

[sezero]

@sagamusix: OK, I applied this to my fork for now:
sezero@92d855e

I'm not much in my capable-of-thinking mode at the moment, so if you have further fixes
please post :)

[bruceg]

I can confirm the patch I proposed causes problems for other mods. However, the patch proposed by sezero also causes artifacts on some other loops. I haven't looked further, but I would assume it is due to issues raised by sagamusix.

[sezero]

the patch proposed by sezero also causes artifacts on some other loops.

Having a patch for that would be welcome.

[sezero]

the patch proposed by sezero also causes artifacts on some other loops.

Having a patch for that would be welcome.

Anybody?