Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AGOS: RFC: Fix Waxworks AdLib music (bug #13048) #3472

Merged
merged 3 commits into from Nov 2, 2021

Conversation

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Oct 30, 2021

This is an attempt at fixing the Waxworks AdLib music. See https://bugs.scummvm.org/ticket/13048 for some details.

The problem turned out to be that when calculating the value to be written to the 40 - 55 AdLib registerers, it could underflow so instead of 0 (max volume) it would write some negative value, probably close to the minimum volume.

Much of this calculation is done in the noteOn() function, but the channelVolumeAdjust variable (which is clipped to the appropriate interval) is never used. I assume that was the main error. I've also changed it so that the clipping is done after all the adjustments have been made. (And since we're working with small 16-bit integers, there is no need to use floating-point math either, so I've eliminated that.)

Is it correct? I don't know. At least for Waxworks, it seems that most notes end up being played at max volume, but perhaps that's how it should be? I'm just guessing here, and if anyone has a better fix I'd be happy to drop this one. Whatever the fix is, it should almost certainly go into the 2.5 branch as well.

Because only the unused channelVolueAdjust value was clipped, the actual
velocity of the note would often overflow, causing the note to be almost
muted instead of played at full volume.

I assume it was an oversight that chanelVolumeAdjust wasn't used, so
I've fixed that and moved the clipping to after all adjustments have
been made. Also eliminated some unneeded floating-point math.

Is it correct now? I honestly don't know. It seems that, at least in
Waxworks, almost all notes end up being played at max volume. But maybe
that's how it should be?
@athrxx
Copy link
Member

@athrxx athrxx commented Oct 30, 2021

I have tested this and can confirm that it at least fixes the issue. The music seems okay now.

But I can also confirm that mostly everything seems to be played at max volume. It seems irrelevant whether I set the GMM music volume to 255 or to 192, since it is all clipped to the same max vol.
I suspect that the sound driver is not accurate, but it is hard to tell without more research. I haven't compared with DOSBox. And sometimes the original drivers just aren't really good either.

Anyway, that's not the point of this PR. If someone wants to rework the driver he/she can always do it...

Loading

// adjust velocity with the master volume
byte volumeAdjust = adjustedVelocity * ((float) (128 + _masterVolume) / 128);
channelVolumeAdjust = (channelVolumeAdjust * (128 + _masterVolume)) / 128;
Copy link
Contributor

@NMIError NMIError Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is here. _masterVolume has a range of -128 to 127, so after adding 128 0 to 255. It is then multiplied with a MIDI velocity (0-127) and then divided by 128, leaving a value in the range of 0 to 255. It should probably be divided by 255 to again get a MIDI velocity value range (0-127).

Loading


adjustedVelocity = volumeAdjust;
byte adjustedVelocity = channelVolumeAdjust;
Copy link
Contributor

@NMIError NMIError Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this value is later set on _channels[FMvoiceChannel].velocity. This is used in setVolume, where a new _masterVolume value is applied. So the value set there should be just velocity and volumeAdjust, without _masterVolume applied. Also, setVolume has the same bugged volume calculation.

Loading

Copy link
Member Author

@eriktorbjorn eriktorbjorn Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm getting pretty confused over here so I'm just going to write down some stuff for my own benefit here... :-)

Before the changes, we have the following variables:

  • The velocity input parameter.
  • channelVolumeAdjust which is calculated as CLIP(velocity + _channels[FMvoiceChannel].volumeAdjust)
  • volumeAdjust which is calculated as velocity * ((floag) (128 + _masterVolume) / 128)
  • adjustedVelocity, which is set to volumeAdjust.

After this point, velocity, channelVolumeAdjust and volumeAdjust are no longer used. (Which means that channelVolumeAdjust isn't used at all.)

adjustedVelocity is then further adjusted, before it's stored in _channels[FMvoiceChannel].velocity and passed on to noteOnSetVolume().

So if _masterVolume should not be factored into _channels[FMvoiceChannel].velocity, I guess it would be more sensible to do that adjustment in noteOnSetVolume()?

Loading

Copy link
Member Author

@eriktorbjorn eriktorbjorn Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some further changes. Now the volume seems a bit low to me, but of course we could have a different value than 15 for the master volume.

Loading

Copy link
Contributor

@NMIError NMIError Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that will put it to half volume. I thought master volume was set from the user volume setting, but that does not seem to be the case. That should probably be added to MidiPlayer::setVolume (only for this driver).

Loading

Copy link
Member Author

@eriktorbjorn eriktorbjorn Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confuse things - or at least me - even further, this is what AGOS's MidiPlayer::pause() does:

	if (musicType == MT_ADLIB && _musicMode == kMusicModeAccolade) {
		static_cast <MidiDriver_Accolade_AdLib*> (_driver)->setVolume(_paused ? 0 : 128);
	}

So here it seems like 0 means silence, and 128 means... unchanged volume? Half volume? Either way, 128 won't reset _masterVolume to 15.

I'm not sure how to proceed here...

Loading

Copy link
Contributor

@NMIError NMIError Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The range for the setVolume parameter is 0 - 255. This is then converted to -128 to 127 for _masterVolume, then back to 0 - 255 for the volume calculations because... reasons. The unpaused value should be the previously set volume, i.e. MidiPlayer::_musicVolume or _sfxVolume, whichever one you decide to use in MidiPlayer::setVolume.

Loading

After feedback from NMIError, and some more guesswork. The master volume
isn't applied until noteOnSetVolume(), so that the correct adjusted
volume can be stored in the channel data structure.

Now the volume seems too low to me, but of course that could be fixed by
having some other default value than 15 for _masterVolume.
At this point, the driver still tries to play notes at the maximum
allowed volume or louder, but ScummVM's music volume setting can bring
them back down to the expected range. That may have to be good enough
for now.

Also, since the master volume is only used internally there's no need to
keep converting back and forth. Just use the interval 0-255 throughout.
@eriktorbjorn
Copy link
Member Author

@eriktorbjorn eriktorbjorn commented Nov 1, 2021

That's probably as good as it's going to get at my hands. It's still trying to play music at or above the maximum volume, but now the ScummVM music volume setting can bring it down. Clipping is done after the master volume has been applied.

Of course, if anyone has a better idea how to do it I'd be happy to drop this pull request in a heartbeat.

Loading

@NMIError
Copy link
Contributor

@NMIError NMIError commented Nov 1, 2021

This needs some further investigation, but it looks good enough to fix the immediate problem.

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

@NMIError thus, do you think it is OK to merge?

Loading

@NMIError
Copy link
Contributor

@NMIError NMIError commented Nov 1, 2021

Yes, looks fine to me.

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 2, 2021

Thank you, @eriktorbjorn for the work and @NMIError for the review. Merging

Loading

@sev- sev- merged commit 09433c1 into scummvm:master Nov 2, 2021
8 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants