View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000751 | OpenMPT | Playback Compatibility | public | 2016-02-19 05:46 | 2017-07-01 00:43 |
Reporter | Revenant | Assigned To | Saga Musix | ||
Priority | normal | Severity | minor | Reproducibility | N/A |
Status | resolved | Resolution | fixed | ||
Product Version | OpenMPT 1.25.04.00 / libopenmpt 0.2-beta16 (upgrade first) | ||||
Target Version | OpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first) | Fixed in Version | OpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first) | ||
Summary | 0000751: Soundtracker heuristic tweaks | ||||
Description | Here's a patch which fixes two Soundtracker modules in SLL's Modland directory (also attached) which are currently not identified correctly. "multi.megamix.mod" - is Ultimate Soundtracker 1.8, misidentified as SoundTracker II
"digital touch.mod" - is SoundTracker III, misidentified as SoundTracker 2.x
I also fixed a typo in "SoundTracker II" which I assume was left over from Fraggie's notes. I couldn't find any regressions caused by any of these changes, especially since I tried to base my Dxx behavior changes on the most realistic use cases, so hopefully it keeps other ST modules working well. | ||||
Tags | No tags attached. | ||||
Attached Files | SLLfixes.patch (2,933 bytes)
diff --git OpenMPT/soundlib/Load_mod.cpp OpenMPT/soundlib/Load_mod.cpp index 33c791a..582b648 100644 --- OpenMPT/soundlib/Load_mod.cpp +++ OpenMPT/soundlib/Load_mod.cpp @@ -936,7 +936,7 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) totalSampleLen += Samples[smp].nLength; - if(m_szNames[smp][0] && ((memcmp(m_szNames[smp], "st-", 3) && memcmp(m_szNames[smp], "ST-", 3)) || m_szNames[smp][0] < '0' || m_szNames[smp][0] > '9')) + if(m_szNames[smp][0] && ((memcmp(m_szNames[smp], "st-", 3) && memcmp(m_szNames[smp], "ST-", 3)) || m_szNames[smp][5] != ':')) { // Ultimate Soundtracker 1.8 and D.O.C. SoundTracker IX always have sample names containing disk names. hasDiskNames = false; @@ -1018,8 +1018,10 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) { uint8 data[4]; file.ReadArray(data); + const ROWINDEX row = i / 4; const uint8 eff = data[2] & 0x0F, param = data[3]; - if(emptyCmds != 0 && !memcmp(data, "\0\0\0\0", 4)) + // Check for empty space between the last Dxx command and the beginning of another pattern + if(emptyCmds != 0 && (row & 63) != 0 && !memcmp(data, "\0\0\0\0", 4)) { emptyCmds++; if(emptyCmds > 32) @@ -1041,7 +1043,7 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) { // If a 1xx / 2xx effect has a parameter greater than 0x20, it is assumed to be UST. minVersion = hasDiskNames ? UST1_80 : UST1_00; - } else if(eff == 1 && param < 0x03) + } else if(eff == 1 && param > 0 && param < 0x03) { // This doesn't look like an arpeggio. minVersion = std::max(minVersion, ST2_00_Exterminator); @@ -1054,12 +1056,18 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) case 0x0D: case 0x0E: minVersion = std::max(minVersion, ST2_00_Exterminator); - if(eff == 0x0D && param == 0) + if(eff == 0x0D) { - // Assume this is a pattern break command. - minVersion = std::max(minVersion, ST2_00); + emptyCmds = 1; + if (param == 0 && (row & 1) != 0) + { + // Assume this is a pattern break command. + // (now only if it's on an odd-numbered row, like it usually is. + // This fixes "digital touch" by SLL, which does runs of + // D00 D04 D08 D0F volslides repeatedly in one of its patterns) + minVersion = std::max(minVersion, ST2_00); + } } - emptyCmds = 1; break; case 0x0F: minVersion = std::max(minVersion, ST_III); @@ -1184,7 +1192,7 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) m_madeWithTracker = "Ultimate Soundtracker 1.8-2.0"; break; case ST2_00_Exterminator: - m_madeWithTracker = "SoundTracker 2.0 / D.O.C. Sountracker II"; + m_madeWithTracker = "SoundTracker 2.0 / D.O.C. SoundTracker II"; break; case ST_III: m_madeWithTracker = "Defjam Soundtracker III / Alpha Flight SoundTracker IV / D.O.C. SoundTracker IV / VI"; STfixes2.patch (4,787 bytes)
diff --git OpenMPT/soundlib/Load_mod.cpp OpenMPT/soundlib/Load_mod.cpp index 33c791a..2fc250c 100644 --- OpenMPT/soundlib/Load_mod.cpp +++ OpenMPT/soundlib/Load_mod.cpp @@ -880,7 +880,6 @@ enum STVersions ST_IX, // D.O.C. SoundTracker IX (Unknown/D.O.C.) MST1_00, // Master Soundtracker 1.0 (Tip/The New Masters) ST2_00, // SoundTracker 2.0, 2.1, 2.2 (Unknown/D.O.C.) - ST2_00_with_Bxx, // Bxx effect found, so definitely SoundTracker 2.0, 2.1, 2.2 (Unknown/D.O.C.) }; @@ -936,7 +935,7 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) totalSampleLen += Samples[smp].nLength; - if(m_szNames[smp][0] && ((memcmp(m_szNames[smp], "st-", 3) && memcmp(m_szNames[smp], "ST-", 3)) || m_szNames[smp][0] < '0' || m_szNames[smp][0] > '9')) + if(m_szNames[smp][0] && ((memcmp(m_szNames[smp], "st-", 3) && memcmp(m_szNames[smp], "ST-", 3)) || m_szNames[smp][5] != ':')) { // Ultimate Soundtracker 1.8 and D.O.C. SoundTracker IX always have sample names containing disk names. hasDiskNames = false; @@ -1014,24 +1013,39 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) // Scan patterns to identify Ultimate Soundtracker modules. uint8 emptyCmds = 0; + uint8 numDxx = 0; for(size_t i = 0; i < numPatterns * 64u * 4u; i++) { uint8 data[4]; file.ReadArray(data); + const ROWINDEX row = (i / 4) % 64; const uint8 eff = data[2] & 0x0F, param = data[3]; - if(emptyCmds != 0 && !memcmp(data, "\0\0\0\0", 4)) + // Check for empty space between the last Dxx command and the beginning of another pattern + if(emptyCmds != 0 && (i % (64 * 4)) != 0 && !memcmp(data, "\0\0\0\0", 4)) { emptyCmds++; if(emptyCmds > 32) { // Since there is a lot of empty space after the last Dxx command, // we assume it's supposed to be a pattern break effect. - minVersion = ST2_00_with_Bxx; + minVersion = ST2_00; } } else { emptyCmds = 0; } + + // Check for a large number of Dxx commands in the previous pattern + if(numDxx != 0 && (i % (64 * 4)) == 0) + { + if(numDxx < 3) + { + // not many Dxx commands in one pattern means they were probably pattern breaks + minVersion = ST2_00; + } + + numDxx = 0; + } switch(eff) { @@ -1041,25 +1055,29 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) { // If a 1xx / 2xx effect has a parameter greater than 0x20, it is assumed to be UST. minVersion = hasDiskNames ? UST1_80 : UST1_00; - } else if(eff == 1 && param < 0x03) + } else if(eff == 1 && param > 0 && param < 0x03) { // This doesn't look like an arpeggio. minVersion = std::max(minVersion, ST2_00_Exterminator); } break; case 0x0B: - minVersion = ST2_00_with_Bxx; + minVersion = ST2_00; break; case 0x0C: case 0x0D: case 0x0E: minVersion = std::max(minVersion, ST2_00_Exterminator); - if(eff == 0x0D && param == 0) + if(eff == 0x0D) { - // Assume this is a pattern break command. - minVersion = std::max(minVersion, ST2_00); + emptyCmds = 1; + if(param == 0 && row == 0) + { + // Fix a possible tracking mistake in Blood Money title - who wants to do a pattern break on the first row anyway? + break; + } + numDxx++; } - emptyCmds = 1; break; case 0x0F: minVersion = std::max(minVersion, ST_III); @@ -1107,14 +1125,10 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) } if(m.command == 0x0D) { - if((m.param != 0 && minVersion != ST2_00_with_Bxx) || minVersion < ST_IX) + if(minVersion != ST2_00) { // Dxy is volume slide in some Soundtracker versions, D00 is a pattern break in the latest versions. m.command = 0x0A; - } else if(m.param == 0 && row == 0 && minVersion != ST2_00_with_Bxx) - { - // Fix a possible tracking mistake in Blood Money title - who wants to do a pattern break on the first row anyway? - m.command = m.param = 0; } else { m.param = 0; @@ -1184,7 +1198,7 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) m_madeWithTracker = "Ultimate Soundtracker 1.8-2.0"; break; case ST2_00_Exterminator: - m_madeWithTracker = "SoundTracker 2.0 / D.O.C. Sountracker II"; + m_madeWithTracker = "SoundTracker 2.0 / D.O.C. SoundTracker II"; break; case ST_III: m_madeWithTracker = "Defjam Soundtracker III / Alpha Flight SoundTracker IV / D.O.C. SoundTracker IV / VI"; @@ -1196,7 +1210,6 @@ bool CSoundFile::ReadM15(FileReader &file, ModLoadingFlags loadFlags) m_madeWithTracker = "Master Soundtracker 1.0"; break; case ST2_00: - case ST2_00_with_Bxx: m_madeWithTracker = "SoundTracker 2.0 / 2.1 / 2.2"; break; } | ||||
Has the bug occurred in previous versions? | |||||
Tested code revision (in case you know it) | 5939 | ||||
Sounds good in general, but regarding "digital touch.mod", I have an idea that might work better - namely that if there are too many Dxx effects in one pattern, assume that they must be volume slides instead. I think that makes more sense than looking for breaks on odd vs even rows. |
|
Yeah, that makes sense. I've uploaded another patch that does it that way. I also made a few other changes while I was at it:
Hopefully nothing else broke in the process of me doing that. |
|
Edit: I looked at the wrong version of the Blood Money title, and you are correct that my fix didn't work. :) |
|
Patch was applied in r5941. |
|
Cool, thanks :) |
|
Here's a tricky one, if I can reuse this issue report... The broken module this time is "jjk55" by Jesper Kyd: How it's supposed to sound: Seems to have been made using a version of Soundtracker with no tempo support, but has an unusual starting tempo value anyway, which makes it play back about twice as fast as it should. I guess you could just reset the starting tempo at the end of the loading process if it's determined to be a version other than the two that have tempo support, but I haven't touched it myself since I didn't want to risk breaking some other modules (meanwhile this is the only one I know of that has this issue). (Just to be sure, I ripped the module myself directly from the Kefrens demo just to make sure the one on Modland wasn't just a bad copy, and it isn't.) |
|
I don't think that's feasible. FWIW, the file is detected as ST_III, but the only difference to reliably tell it apart from ST_IX would be that an ST_IX module would have to use the E00/E01 filter command. So a fix for this one module would break all modules made with D.O.C. SoundTracker IX that use a custom tempo but no filter command. We could do a strcmp on the song title ("jjk55"), but that feels very dirty. |
|
Yeah, that was pretty much exactly the sort of compatibility breakage I was afraid of which is why I didn't try to write a fix for this one myself. Maybe I ought to just spread a fixed version of the module instead. (I definitely don't want to check for specific modules by name, either.) |
|
It feels bad but I added a heuristic based on the module title in r6296 - it's unique enough and the format "closed" enough (i.e. noone still creates new SoundTracker modules) to allow for this. In the same commit I also fixed the heuristics for properly loading sleepwalk.mod by Karsen Obarski. |
|
Looking at the code from this patch, you use EDIT: I think I understand the patch a bit better now and I guess I was wrong. I have cleaned up that loop in r8383 to have separate loops for patterns, rows and channels like in the regular pattern loading code. This makes these conditions much more readable. If you have the time, maybe you can look over the changes and verify that they still do what you intended to do with this patch? :) |
|
Date Modified | Username | Field | Change |
---|---|---|---|
2016-02-19 05:46 | Revenant | New Issue | |
2016-02-19 05:46 | Revenant | File Added: SLLfixes.patch | |
2016-02-19 05:46 | Revenant | File Added: SLLfixes.zip | |
2016-02-19 07:49 | Saga Musix | Assigned To | => Saga Musix |
2016-02-19 07:49 | Saga Musix | Status | new => assigned |
2016-02-19 18:13 | Saga Musix | Note Added: 0002265 | |
2016-02-20 01:03 | Revenant | File Added: STfixes2.patch | |
2016-02-20 01:07 | Revenant | Note Added: 0002266 | |
2016-02-20 13:45 | Saga Musix | Note Added: 0002267 | |
2016-02-20 16:24 | Saga Musix | Note Edited: 0002267 | |
2016-02-20 16:27 | Saga Musix | Note Added: 0002268 | |
2016-02-20 16:27 | Saga Musix | Status | assigned => feedback |
2016-02-20 20:26 | Revenant | Note Added: 0002269 | |
2016-02-20 20:26 | Revenant | Status | feedback => assigned |
2016-02-21 01:50 | Saga Musix | Status | assigned => resolved |
2016-02-21 01:50 | Saga Musix | Resolution | open => fixed |
2016-02-21 01:50 | Saga Musix | Product Version | => OpenMPT 1.25.04.00 / libopenmpt 0.2-beta16 (upgrade first) |
2016-02-21 01:50 | Saga Musix | Fixed in Version | => OpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first) |
2016-02-21 01:50 | Saga Musix | Target Version | => OpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first) |
2016-04-17 09:53 | Revenant | Note Added: 0002336 | |
2016-04-17 09:53 | Revenant | Status | resolved => feedback |
2016-04-17 09:53 | Revenant | Resolution | fixed => reopened |
2016-04-17 13:54 | Saga Musix | Note Added: 0002337 | |
2016-04-17 13:55 | Saga Musix | Note Edited: 0002337 | |
2016-04-17 17:55 | Revenant | Note Added: 0002338 | |
2016-04-17 17:55 | Revenant | Status | feedback => assigned |
2016-04-17 21:17 | Saga Musix | Status | assigned => resolved |
2016-04-17 21:17 | Saga Musix | Resolution | reopened => fixed |
2016-04-26 15:38 | Saga Musix | Note Added: 0002351 | |
2017-07-01 00:31 | Saga Musix | Note Added: 0003093 | |
2017-07-01 00:34 | Saga Musix | Note Edited: 0003093 | |
2017-07-01 00:42 | Saga Musix | Note Edited: 0003093 | |
2017-07-01 00:43 | Saga Musix | Note Edited: 0002337 | |
2017-07-01 00:43 | Saga Musix | Note Edited: 0002267 |