SCUMM: DOTT: Incorrect MIDI pitch bending

[AndywinXp]

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.

[athrxx]

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...

[athrxx]

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.

[AndywinXp]

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

[AndywinXp]

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

[AndywinXp]

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

[AndywinXp]

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 */ }
  }
}

[AndywinXp]

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); 
    }
  }
}

[AndywinXp]

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

[athrxx]

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...

[athrxx]

So, I have attempted a fix for the General Midi which seems to work well for the DOTT intro when comparing to DOSBox.

I haven't looked at the AdLib sound, since this seems less obvious. At least the upwards pitch bends right after starting seem accurate. But the downwards bending when the bird is dropping down might indeed be broken.

Maybe it's just the calculation formula, too...

(For the GM it was mostly missing initialization, but also the formula)

And it would make sense to check SAMNMAX. It might have a different initialization and formula.

[athrxx]

I have now made a follow-up fix for the AdLib driver.

I'll experiment a bit more with this, maybe there will be another update (SAMNMAX AdLib is probably wrong like this, but it isn't easy to test, since there is not much use of pitch bend, transpose, etc.).

[AndywinXp]

First of all, great job, thanks for this!

I have been testing both the GM and Adlib version since these very recent fixes, and the problematic section appears to be accurate! The midi file seems to be switching to the following one (the tentacles' music) a bit too early (some notes are not being heard), but I'm guessing this is a separate iMUSE issue.

What I think is a correlated issue though, is that during the tentacles' music the pitch value of each track is not being reset properly, so some of the instruments appear to be a tiny bit out of tune. It seems to be random each time, though...

[athrxx]

Replying to AndywinXp:

What I think is a correlated issue though, is that during the tentacles' music the pitch value of each track is not being reset properly, so some of the instruments appear to be a tiny bit out of tune. It seems to be random each time, though...

I have now added the detune setting to the parameters that get reset to default values for new midi parts. Does it solve the issue? (The description is a bit vague: is it even a regression? does it happen only with AdLib or any other specific driver?).

[AndywinXp]

First of all, sorry about the lack of precise info on my end, I should work on proofreading my ticket responses and making sure they don't sound confusing :')

I was referring to AdLib, whose detune handling now appears to work perfectly. The only issue remaining is the one mentioned above which I described as a "separate iMUSE issue". I think that this issue might be something related to the fact that we might not be handling the transpose (or maybe even detune/pitchbend) value bounds correctly.

As a matter of fact I have compiled a small series of recording comparisons (which I'm attaching here) between 2.7.0git and DREAMM (on MT-32, AdLib and GM) which show the exact issue.

The issue can be heard in each of the mp3 files named with the "270" suffix, towards 8-9 seconds, which is the end of the transpose/detune/pitchbending heavy section. When comparing with the other three mp3 files ("DREAMM" suffix) we can hear that at the same point of the section, the transpose value (at least that's what I think it is, from the sound of it) goes up instead of continuing to go down.

It happens on all three audio devices, so that makes me think that it's not a driver related issue, but an iMUSE one, and in particular one related to value bounds, as I said. In the original, the transpose value seems be trying to go down so many times that at a certain point it stops going down and it returns to what seems to be its default value.

Please let me know if this makes sense.

[AndywinXp]

Very small update on that; I had a couple of idle minutes so I checked the interpreter functions I posted a while ago in here (in particular the one I named IMUSE_PART_SetTranspose):
in our Part::set_transpose(), changing _transpose_eff = (_transpose == -128) ? 0 : transpose_clamp(_transpose + _player->getTranspose(), -24, 24); to _transpose_eff = (_transpose == -128) ? 0 : transpose_clamp(_transpose + _player->getTranspose(), -12, 12); seems to fix the issue.

Of course, I'm guessing that by doing directly that I'm surely breaking something else, and I unfortunately don't have time right now to properly look at it, but at least I found the source of this peculiar problem.

[athrxx]

Thanks, that is very helpful, I am sure it can be fixed based on that. I always felt very reluctant to make any changes to these bounds, because

• they can be device/driver specific
• I don't know why the authors of the code chose the bounds the way they did
• I never noticed any difference

But if the same bug appears in the same way on all devices you tested, maybe the values can also be fixed in the same manner for all of them.

[athrxx]

I have made another fix that should take care of the transpose clipping boundaries. My impression is that it fixes the DOTT intro music. It is basically the solution that you have already tried, just with some adaptions towards different games/versions...

[AndywinXp]

And indeed it now works precisely as intended! Outstanding work! Closing the ticket...