View Issue Details

IDProjectCategoryView StatusLast Update
0000921OpenMPT[All Projects] Playback Compatibilitypublic2017-03-19 13:58
ReporterRevenant 
Assigned ToSaga Musix 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product VersionOpenMPT 1.27.00.* (current testing) 
Target VersionOpenMPT 1.26.09.00 / libopenmpt 0.2-beta22 (current stable)Fixed in VersionOpenMPT 1.26.09.00 / libopenmpt 0.2-beta22 (current stable) 
Summary0000921: A few SoundFX command fixes
Description

After listening to a lot of SoundFX modules in OpenMPT, I noticed a few issues that this patch addresses:

  • When converting 7xy/8xy commands, the param for 1xx/2xx is now clamped to keep the pitch from sliding past the target note
  • When a pitch slide is active, it now takes precedence over some other commands, and moves volume commands to the volume column
    (both of these noticeably affect "youramiga.sfx" by Kerni)

[also changed 9xx but probably incorrectly, see comment]

TagsNo tags attached.
Has the bug occurred in previous versions?
Tested code revision (in case you know it)

Relationships

Activities

Revenant

Revenant

2017-02-19 08:04

reporter  

soundfx-fix.patch (3,789 bytes)
diff --git a/OpenMPT/soundlib/Load_sfx.cpp b/OpenMPT/soundlib/Load_sfx.cpp
index c83ef78..2c61d1a 100644
--- a/OpenMPT/soundlib/Load_sfx.cpp
+++ b/OpenMPT/soundlib/Load_sfx.cpp
@@ -10,6 +10,7 @@
 
 #include "stdafx.h"
 #include "Loaders.h"
+#include "Tables.h"
 
 OPENMPT_NAMESPACE_BEGIN
 
@@ -73,6 +74,27 @@ struct SFXSampleHeader
 
 MPT_BINARY_STRUCT(SFXSampleHeader, 30)
 
+static uint8 ClampSlideParam(uint8 value, uint8 lowNote, uint8 highNote)
+//----------------------------------------------------------------------
+{
+	uint16 lowPeriod, highPeriod;
+
+	if(lowNote  < highNote &&
+	   lowNote  >= 36 + NOTE_MIN &&
+	   highNote >= 36 + NOTE_MIN &&
+	   lowNote  < CountOf(ProTrackerPeriodTable) + 36 + NOTE_MIN &&
+	   highNote < CountOf(ProTrackerPeriodTable) + 36 + NOTE_MIN)
+	{
+		lowPeriod  = ProTrackerPeriodTable[lowNote - 36 - NOTE_MIN];
+		highPeriod = ProTrackerPeriodTable[highNote - 36 - NOTE_MIN];
+
+		// with a fixed speed of 6 ticks/row, and excluding the first row,
+		// 1xx/2xx param has a max value of (low-high)/5 to avoid sliding too far
+		return std::min<uint8>(value, static_cast<uint8>((lowPeriod - highPeriod) / 5));
+	}
+
+	return 0;
+}
 
 bool CSoundFile::ReadSFX(FileReader &file, ModLoadingFlags loadFlags)
 //-------------------------------------------------------------------
@@ -241,11 +263,23 @@ bool CSoundFile::ReadSFX(FileReader &file, ModLoadingFlags loadFlags)
 						break;
 
 					case 0x3: // enable filter/LED
+						// give precedence to 7xy/8xy slides
+						if(slideRate[chn])
+						{
+							m.command = m.param = 0;
+							break;
+						}
 						m.command = CMD_MODCMDEX;
 						m.param = 0;
 						break;
 
 					case 0x4: // disable filter/LED
+						// give precedence to 7xy/8xy slides
+						if(slideRate[chn])
+						{
+							m.command = m.param = 0;
+							break;
+						}
 						m.command = CMD_MODCMDEX;
 						m.param = 1;
 						break;
@@ -255,6 +289,15 @@ bool CSoundFile::ReadSFX(FileReader &file, ModLoadingFlags loadFlags)
 						{
 							m.command = CMD_VOLUME;
 							m.param = std::min(ModCommand::PARAM(0x3F), static_cast<ModCommand::PARAM>((Samples[m.instr].nVolume / 4u) + m.param));
+
+							// give precedence to 7xy/8xy slides (and move this to the volume column)
+							if(slideRate[chn])
+							{
+								m.volcmd = VOLCMD_VOLUME;
+								m.vol = m.param;
+								m.command = m.param = 0;
+								break;
+							}
 						} else
 						{
 							m.command = m.param = 0;
@@ -269,6 +312,15 @@ bool CSoundFile::ReadSFX(FileReader &file, ModLoadingFlags loadFlags)
 								m.param = static_cast<ModCommand::PARAM>(Samples[m.instr].nVolume / 4u) - m.param;
 							else
 								m.param = 0;
+
+							// give precedence to 7xy/8xy slides (and move this to the volume column)
+							if(slideRate[chn])
+							{
+								m.volcmd = VOLCMD_VOLUME;
+								m.vol = m.param;
+								m.command = m.param = 0;
+								break;
+							}
 						} else
 						{
 							m.command = m.param = 0;
@@ -279,14 +331,20 @@ bool CSoundFile::ReadSFX(FileReader &file, ModLoadingFlags loadFlags)
 						slideTo[chn] = lastNote[chn] - (m.param >> 4);
 
 						m.command = CMD_PORTAMENTODOWN;
-						m.param = slideRate[chn] = m.param & 0xF;
+						slideRate[chn] = m.param & 0xF;
+						m.param = ClampSlideParam(slideRate[chn], slideTo[chn], lastNote[chn]);
 						break;
 
 					case 0x8: // 8xy: slide up x semitones at speed y
 						slideTo[chn] = lastNote[chn] + (m.param >> 4);
 
 						m.command = CMD_PORTAMENTOUP;
-						m.param = slideRate[chn] = m.param & 0xF;
+						slideRate[chn] = m.param & 0xF;
+						m.param = ClampSlideParam(slideRate[chn], lastNote[chn], slideTo[chn]);
+						break;
+
+					case 0x9: // 9xx: set sample offset
+						m.command = CMD_OFFSET;
 						break;
 
 					default:
soundfx-fix.patch (3,789 bytes)
Revenant

Revenant

2017-02-19 08:04

reporter   ~0002882

ftp://ftp.modland.com/pub/modules/SoundFX/Kerni/youramiga.sfx
ftp://ftp.modland.com/pub/modules/SoundFX/Zenith/it's%20my%20life.sfx

Revenant

Revenant

2017-02-19 08:34

reporter   ~0002883

Okay, maybe I was wrong about 9xx - flod seems to actually treat it as an automatic vibrato (see https://github.com/photonstorm/Flod/blob/master/Flod%204.0/neoart/flod/soundfx/FXPlayer.as#L237)

I'm sure I've said this before, but it'd be nice if I could actually find any official documentation about this tracker...

Saga Musix

Saga Musix

2017-02-19 13:28

administrator   ~0002884

Should I wait until the 9xx part is fixed before looking at the patch? Or just drop in everything apart from 9xx for now?

Revenant

Revenant

2017-02-19 21:56

reporter   ~0002885

Just leave it out for now. I don't think it's used by very many modules, and based on a quick look at Flod's player source it seems to behave fairly differently than the Protracker etc. vibrato commands anyway.

Saga Musix

Saga Musix

2017-02-21 00:20

administrator   ~0002887

Patch (apart from 9xx import) applied in r7664.

Issue History

Date Modified Username Field Change
2017-02-19 08:04 Revenant New Issue
2017-02-19 08:04 Revenant File Added: soundfx-fix.patch
2017-02-19 08:04 Revenant Note Added: 0002882
2017-02-19 08:20 manx Assigned To => Saga Musix
2017-02-19 08:20 manx Status new => assigned
2017-02-19 08:20 manx Target Version => OpenMPT 1.27.01.00 (upcoming stable)
2017-02-19 08:34 Revenant Note Added: 0002883
2017-02-19 09:37 Revenant Description Updated View Revisions
2017-02-19 13:28 Saga Musix Note Added: 0002884
2017-02-19 21:56 Revenant Note Added: 0002885
2017-02-21 00:20 Saga Musix Status assigned => resolved
2017-02-21 00:20 Saga Musix Resolution open => fixed
2017-02-21 00:20 Saga Musix Fixed in Version => OpenMPT 1.27.01.00 (upcoming stable)
2017-02-21 00:20 Saga Musix Note Added: 0002887
2017-03-19 13:58 Saga Musix Fixed in Version OpenMPT 1.27.01.00 (upcoming stable) => OpenMPT 1.26.09.00 / libopenmpt 0.2-beta22 (current stable)
2017-03-19 13:58 Saga Musix Target Version OpenMPT 1.27.01.00 (upcoming stable) => OpenMPT 1.26.09.00 / libopenmpt 0.2-beta22 (current stable)