Opened 3 months ago

Last modified 3 months ago

#13460 new defect

SCUMM: DOTT: Incorrect MIDI pitch bending

Reported by: AndywinXp Owned by:
Priority: normal Component: Engine: SCUMM
Version: Keywords: adlib
Cc: Game: Day of the Tentacle

Description

There appears to be an issue in the pitch bending range setting when playing DOTT.
Here's how the intro should sound (this applies to both General Midi and Adlib but not MT-32 which instead plays as it should):
https://youtu.be/dxqIHifKPms?t=24

And here's how it currently sounds on 2.6.0git and below:
https://youtu.be/hNeshw8jS18?t=24

It sounds to me like a pitch range command not being executed (I frankly don't know what the interpreter uses, the standard IIRC allows you to do that by setting CC 100 (RPN LSB) or CC 101 (RPN MSB) to 0 and finally setting CC 6 (Data Entry MSB) to the target number of semitones.

Change History (9)

comment:1 by athrxx, 3 months ago

Unfortunately, in our imuse implementation too many things have been factored out of the ims drivers into the common code.

The original drivers do have their individual clipping ranges and their individual handlings of transpose, detune and pitch bend commands.

What we do in imuse_part.cpp, line 374, might actually be right for the MT-32 driver. I don't know if that driver has that sort of code (could be, since it appparently works as intended) or if it is an invention of the author of our code.

When running the DOTT intro with AdLib setting, at the point where the bird is dropping down, it will repeatedly call Player::setTranspose() and that clipping from imuse_part.cpp, line 374 kicks in. That clipping is probably the cause of the sound glitch.

When I made the Amiga driver I fixed the transpose issue for that driver (see imuse_part.cpp, lines 148-153, 373, 377-388), but it seems it would be worthwhile to check the other drivers, too. Maybe it would suffice to adapt the clipping range...

comment:2 by athrxx, 3 months ago

Makes no sense really to clip the transpose to -24/24 and then do that (transpose * 8192 / 12) calculation and clip it to -8192/8192. For that, you could just clip the transpose to -12/12. Which is actually the correct setting for Amiga.

comment:3 by AndywinXp, 3 months ago

Ooh! First of all, great work finding out where the root of the issue is. It makes me wonder, are we maybe missing driver info even for other games? But that's a can of worms I'm not sure I want to open right now :-P

comment:4 by AndywinXp, 3 months ago

Okay, I lazily reversed what happens on Aaron Giles' interpreter (which uses General MIDI):

These two functions appear to be called constantly during the bird event (forgive the names, I'm directly using my mapped function names, and I'm just putting the relevant parameters in):

IMUSE_SetTranspose(partPtr, relative, transpose) {
  uint8 clampedTranspose;

  clampedTranspose = transpose;
  if ( transpose > 24 || transpose < -24 || relative > 1 )
    return -1;
  if ( relative )
    clampedTranspose = IMUSE_UTILS_ClampTuning(transpose + partPtr->effTranspose, -7, 7);
  partPtr->effTranspose = clampedTranspose;
  IMUSE_PART_SetTranspose(partPtr, 16, relative, clampedTranspose);
  return 0;
}
IMUSE_SetDetune(partPtr, detune) {
  partPtr->detune = detune;
  IMUSE_PART_SetDetune(partPtr, 16, detune);
  return 0;
}

These is the rest of the relevant auxiliary functions:

IMUSE_UTILS_ClampTuning(tuning, min, max)
{
  if ( tuning < min )
    clampedTuning = tuning + 12 * ((min - tuning + 11) / 12);
  if ( tuning > max )
    clampedTuning = tuning - 12 * ((tuning - max + 11) / 12);

  return clampedTuning;
}
IMUSE_PART_SetTranspose(partPtr, partId, relative, transpose) {
  if ( transpose <= 24 && transpose >= -24 ) {
    if ( partId == 16 ) {
      for ( curPart = partPtr->next; curPart; curPart = curPart->next ) {
        curPart->effTranspose = IMUSE_UTILS_ClampTuning(transpose + curPart->transpose, -12, 12);
        IMUSE_PART_SendTransposeToMIDIDevice(curPart);
      }
    } else { /* Don't care */ }
  }
}
IMUSE_PART_SetDetune(partPtr, partId, relative, increment) {

  if ( partId == 16 ) {
    for ( curPart = partPtr->next; curPart ; curPart = curPart->next ) {
      curPart->effDetune = CLIP(increment + curPart->detune, -128, 127);
      IMUSE_PART_SendTransposeToMIDIDevice(curPart);
    }
  } else { /* Don't care */ }

}

The latter two both call this one:

IMUSE_PART_SendTransposeToMIDIDevice(partPtr) {
  effTranspose = CLIP(partPtr->pitchbend + partPtr->effDetune + (partPtr->effTranspose * 128), -2048, 2047);

  partPtr->effTranspose = effTranspose;
  if ( partPtr->midiChannel) {
    midiPartId = midiChannel->chanNum;

    if ( midiPartsTransposeValues[midiPartId] != effTranspose ) {
      midiPartsTransposeValues[midiPartId] = effTranspose;

      // 8192 is added because this number appears to be the center 
      // value for the pitchbend in this case
      sendPitchWheelValueToMIDIDevice(midiPartId, 4 * partPtr->effTranspose + 8192); 
    }
  }
}

Too tired to test it out for now, though

Last edited 3 months ago by AndywinXp (previous) (diff)

comment:5 by AndywinXp, 3 months ago

Update: FOA/MI2 has a couple of differences here and there and SAMNMAX has a completely different interface for performing those detune and transpose operations but luckily it's exactly the same interface of the Digital counterpart of the iMUSE engine, so I guess I'll be able to find my way inside that Pandora Box

Last edited 3 months ago by AndywinXp (previous) (diff)

comment:6 by AndywinXp, 3 months ago

Here's what's different for v5 (FOA/MI2):

IMUSE_UTILS_ClampTuning(tuning, min, max) {
  int16 clampedTuning;

  clampedTuning = tuning;
  if ( tuning < min ) {
    clampedTuning = 12 * ((uint16)(min - tuning - 1) / 12) + 12 + tuning;
    tuning = clampedTuning;
  }

  if ( clampedTuning > max )
    clampedTuning += -12 - 12 * ((uint16)(tuning - max - 1) / 12);

  return clampedTuning;
}
IMUSE_PART_SetTranspose(partPtr, partId, relative, transpose) {
  if ( transpose <= 24 && transpose >= -24 ) {
    if ( partId == 16 ) {
      for ( curPart = partPtr->next; curPart; curPart = curPart->next ) {
        if (curPart->effTranspose != 128) {
          curPart->effTranspose = IMUSE_UTILS_ClampTuning(transpose + curPart->transpose, -12, 12);
          IMUSE_PART_SendTransposeToMIDIDevice(curPart);
        }
      }
    } else { /* Don't care */ }
  }
}

comment:7 by AndywinXp, 3 months ago

Here's what's different for SAMNMAX with respect to DOTT, loosely adapted from the newer iMUSE system to the older one which appears to be what ScummVM's one is based on:

// Appears to be sent track by track instead of globally
IMUSE_SetTranspose(partPtr, relative, transpose) {
  if ( value < -12 || value > 12 ) {
    if ( value != 0 ) {
      partPtr->transpose = partPtr->effTranspose = IMUSE_UTILS_ClampTuning(value + partPtr->transpose, -12, 12);
    } else {
      partPtr->transpose = partPtr->effTranspose = 0;
    }
    IMUSE_PART_SendTransposeToMIDIDevice(partPtr);
  }
}
// Appears to be sent track by track instead of globally
IMUSE_SetDetune(partPtr, detune) {
  if ( value < -9216 || value > 9216 ) {
    partPtr->detune = partPtr->effDetune = detune;
    IMUSE_PART_SendTransposeToMIDIDevice(partPtr);
  }
}
IMUSE_PART_SendTransposeToMIDIDevice(partPtr) {
  effTranspose = partPtr->pitchbend + partPtr->effDetune + (partPtr->effTranspose * 256);

  partPtr->effTranspose = effTranspose;
  if ( partPtr->midiChannel) {
    midiPartId = midiChannel->chanNum;

    if ( midiPartsTransposeValues[midiPartId] != effTranspose ) {
      midiPartsTransposeValues[midiPartId] = effTranspose;

      // 8192 is added because this number appears to be the center 
      // value for the pitchbend in this case; SAMNMAX multiplies the transpose by 2
      sendPitchWheelValueToMIDIDevice(partPtr->midiChannel, 2 * partPtr->effTranspose + 8192); 
    }
  }
}

comment:8 by AndywinXp, 3 months ago

Also we seem to never actually send the transpose value to the part object, sending instead its current transpose value... is that intended?

comment:9 by athrxx, 3 months ago

Looks fishy. But what's the point of even asking, that commit is from a guy who left the scene 20 years ago...

Just checking why this never came up for MI2/Amiga, since I did have issues with that, there. Turns out that Player::setTranspose() is never used there...

At first glace I would assume that the

part->set_transpose(part->_transpose)

is a typo and it should just be

part->set_transpose(_transpose)

However, the actual operation seems to take place in Part::set_transpose. The player transpose value is queried and added there. So it might be intended. Don't ask me why. But otherwise the player transpose would probably be added twice...

Note: See TracTickets for help on using tickets.