SCUMM: Handle note value 1 as "hold current note" in MI1 Mac

After listening to the original music in a Mac emulator (which
unfortunately doesn't handle the music very well), I can only
conclude that note value 1 means the note should continue playing.
At first I thought maybe it was supposed to fade the current note,
or perhaps change its volume, but I can't hear any traces of
either. So I'm going to assume it just means "hold the current
note", though for the life of me I cannot think of any valid
reason for such a command. So it may be wrong, but it sounds
closer to the emulator than it did before.

4 files changed, 50 insertions, 29 deletions

diff --git a/engines/scumm/player_mac.cpp b/engines/scumm/player_mac.cpp
index a91050e889..bbb97d360a 100644
--- a/engines/scumm/player_mac.cpp
+++ b/engines/scumm/player_mac.cpp
@@ -31,13 +31,14 @@
 
 namespace Scumm {
 
-Player_Mac::Player_Mac(ScummEngine *scumm, Audio::Mixer *mixer, int numberOfChannels, int channelMask)
+Player_Mac::Player_Mac(ScummEngine *scumm, Audio::Mixer *mixer, int numberOfChannels, int channelMask, bool fadeNoteEnds)
 	: _vm(scumm),
 	  _mixer(mixer),
 	  _sampleRate(_mixer->getOutputRate()),
 	  _soundPlaying(-1),
 	  _numberOfChannels(numberOfChannels),
-	  _channelMask(channelMask) {
+	  _channelMask(channelMask),
+	  _fadeNoteEnds(fadeNoteEnds) {
 	assert(scumm);
 	assert(mixer);
 }
@@ -298,16 +299,7 @@ uint32 Player_Mac::durationToSamples(uint16 duration) {
 }
 
 int Player_Mac::noteToPitchModifier(byte note, Instrument *instrument) {
-	// TODO: Monkey Island 1 uses both note values 0 and 1 as rests.
-	// Perhaps 0 means an abrupt end of the current note, while 1 means it
-	// should drop off gradually? One of the voices in the main theme
-	// sounds a lot more staccato than what I hear in a Mac emulator. (But
-	// it's hard to tell since that emulator has problems with the music.)
-	// Also, some instruments (though not this particular one) have data
-	// after the loop end point, which could possible be used to fade out
-	// the instrument.
-
-	if (note > 1) {
+	if (note > 0) {
 		const int pitchIdx = note + 60 - instrument->_baseFreq;
 		// I don't want to use floating-point arithmetics here, but I
 		// ran into overflow problems with the church music in Monkey
@@ -356,7 +348,7 @@ int Player_Mac::readBuffer(int16 *data, const int numSamples) {
 			}
 			generated = MIN(_channel[i]._remaining, samplesLeft);
 			if (_channel[i]._velocity != 0) {
-				_channel[i]._instrument.generateSamples(ptr, _channel[i]._pitchModifier, _channel[i]._velocity, generated, _channel[i]._remaining);
+				_channel[i]._instrument.generateSamples(ptr, _channel[i]._pitchModifier, _channel[i]._velocity, generated, _channel[i]._remaining, _fadeNoteEnds);
 			}
 			ptr += generated;
 			samplesLeft -= generated;
@@ -375,7 +367,7 @@ int Player_Mac::readBuffer(int16 *data, const int numSamples) {
 	return numSamples;
 }
 
-void Player_Mac::Instrument::generateSamples(int16 *data, int pitchModifier, int volume, int numSamples, int remainingSamplesOnNote) {
+void Player_Mac::Instrument::generateSamples(int16 *data, int pitchModifier, int volume, int numSamples, int remainingSamplesOnNote, bool fadeNoteEnds) {
 	int samplesLeft = numSamples;
 	while (samplesLeft) {
 		_subPos += pitchModifier;
@@ -389,16 +381,23 @@ void Player_Mac::Instrument::generateSamples(int16 *data, int pitchModifier, int
 
 		int newSample = (((int16)((_data[_pos] << 8) ^ 0x8000)) * volume) / 255;
 
-		// Fade out the last 100 samples on each note. Even at low
-		// output sample rates this is just a fraction of a second,
-		// but it gets rid of distracting "pops" at the end when the
-		// sample would otherwise go abruptly from something to
-		// nothing. This was particularly noticeable on the distaff
-		// notes in Loom.
-
-		remainingSamplesOnNote--;
-		if (remainingSamplesOnNote < 100) {
-			newSample = (newSample * remainingSamplesOnNote) / 100;
+		if (fadeNoteEnds) {
+			// Fade out the last 100 samples on each note. Even at
+			// low output sample rates this is just a fraction of a
+			// second, but it gets rid of distracting "pops" at the
+			// end when the sample would otherwise go abruptly from
+			// something to nothing. This was particularly
+			// noticeable on the distaff notes in Loom.
+			//
+			// The reason it's conditional is that Monkey Island
+			// appears to have a "hold current note" command, and
+			// if we fade out the current note in that case we
+			// will actually introduce new "pops".
+
+			remainingSamplesOnNote--;
+			if (remainingSamplesOnNote < 100) {
+				newSample = (newSample * remainingSamplesOnNote) / 100;
+			}
 		}
 
 		int sample = *data + newSample;
diff --git a/engines/scumm/player_mac.h b/engines/scumm/player_mac.h
index c46495c333..09307b4e57 100644
--- a/engines/scumm/player_mac.h
+++ b/engines/scumm/player_mac.h
@@ -44,7 +44,7 @@ class ScummEngine;
  */
 class Player_Mac : public Audio::AudioStream, public MusicEngine {
 public:
-	Player_Mac(ScummEngine *scumm, Audio::Mixer *mixer, int numberOfChannels, int channelMask);
+	Player_Mac(ScummEngine *scumm, Audio::Mixer *mixer, int numberOfChannels, int channelMask, bool fadeNoteEnds);
 	virtual ~Player_Mac();
 
 	void init();
@@ -90,12 +90,13 @@ private:
 			_subPos = 0;
 		}
 
-		void generateSamples(int16 *data, int pitchModifier, int volume, int numSamples, int remainingSamplesOnNote);
+		void generateSamples(int16 *data, int pitchModifier, int volume, int numSamples, int remainingSamplesOnNote, bool fadeNoteEnds);
 	};
 
 	int _pitchTable[128];
 	int _numberOfChannels;
 	int _channelMask;
+	bool _fadeNoteEnds;
 
 	virtual bool checkMusicAvailable() { return false; }
 	virtual bool loadMusic(const byte *ptr) { return false; }
diff --git a/engines/scumm/player_v3m.cpp b/engines/scumm/player_v3m.cpp
index 35d2aaac89..e61463128a 100644
--- a/engines/scumm/player_v3m.cpp
+++ b/engines/scumm/player_v3m.cpp
@@ -97,7 +97,7 @@
 namespace Scumm {
 
 Player_V3M::Player_V3M(ScummEngine *scumm, Audio::Mixer *mixer)
-	: Player_Mac(scumm, mixer, 5, 0x1E) {
+	: Player_Mac(scumm, mixer, 5, 0x1E, true) {
 	assert(_vm->_game.id == GID_LOOM);
 
 	// Channel 0 seems to be what was played on low-end macs, that couldn't
diff --git a/engines/scumm/player_v5m.cpp b/engines/scumm/player_v5m.cpp
index 1cb9831621..500f3bbc40 100644
--- a/engines/scumm/player_v5m.cpp
+++ b/engines/scumm/player_v5m.cpp
@@ -82,7 +82,7 @@
 namespace Scumm {
 
 Player_V5M::Player_V5M(ScummEngine *scumm, Audio::Mixer *mixer)
-	: Player_Mac(scumm, mixer, 3, 0x07) {
+	: Player_Mac(scumm, mixer, 3, 0x07, false) {
 	assert(_vm->_game.id == GID_MONKEY);
 }
 
@@ -190,7 +190,6 @@ bool Player_V5M::loadMusic(const byte *ptr) {
 }
 
 bool Player_V5M::getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &velocity) {
-	_channel[ch]._instrument.newNote();
 	if (_channel[ch]._pos >= _channel[ch]._length) {
 		if (!_channel[ch]._looped) {
 			_channel[ch]._notesLeft = false;
@@ -206,9 +205,31 @@ bool Player_V5M::getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &
 	byte note = _channel[ch]._data[_channel[ch]._pos + 2];
 	samples = durationToSamples(duration);
 
+	if (note != 1) {
+		_channel[ch]._instrument.newNote();
+	}
+
 	if (note > 1) {
 		pitchModifier = noteToPitchModifier(note, &_channel[ch]._instrument);
 		velocity = _channel[ch]._data[_channel[ch]._pos + 3];
+	} else if (note == 1) {
+		// This is guesswork, but Monkey Island uses two different
+		// "special" note values: 0, which is clearly a rest, and 1
+		// which is... I thought at first it was a "soft" key off, to
+		// fade out the note, but listening to the music in a Mac
+		// emulator (which unfortunately doesn't work all that well),
+		// I hear no trace of fading out.
+		//
+		// It could mean "change the volume on the current note", but
+		// I can't hear that either, and it always seems to use the
+		// exact same velocity on this note.
+		//
+		// So it appears it really just is a "hold the current note",
+		// but why? Couldn't they just have made the original note
+		// longer?
+
+		pitchModifier = _channel[ch]._pitchModifier;
+		velocity = _channel[ch]._velocity;
 	} else {
 		pitchModifier = 0;
 		velocity = 0;