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

MIDI/SCI/KYRA: Add support for Roland GS drumkits #2059

Merged
merged 5 commits into from Feb 16, 2020

Conversation

@NMIError
Copy link
Contributor

NMIError commented Feb 13, 2020

MIDI/SCI/KYRA: Add support for Roland GS drumkits

This PR adds support for the alternate drumkits in Roland GS devices (also present in various GM devices). These are used in the MIDI data of Quest for Glory 3, Space Quest 5, Pepper's Adventures in Time and Lands of Lore.

Most of these games contain invalid drumkit numbers; they relied on a feature of the Roland SC-55 which corrected this. Later devices do not include this feature and drumkit selection would not work. This PR contains code to emulate the correction feature to make the drumkit selections work on later devices.

SCI did not send program changes on the rhythm channel. This has been added as well as the invalid drumkit number correction.
KYRA did already send program changes on the rhythm channel; invalid drumkit correction has been added.

I've tested this on a Roland SC-55mkII as well as FluidSynth with a GS compatible soundfont.

NMIError added 4 commits Jan 27, 2020
This adds support for the Roland GS drumkits used by SQ5 and QfG3.
The original Sierra GM driver does not pass the drumkit select MIDI
messages to the GM device, but they do exist in the MIDI data.

Another issue is that the drumkit numbers used are incorrect. This
does work on the original Roland SC-55 devices because they correct
the drumkit numbers. Later devices do not do this. Code has been
added to correct these wrong drumkit numbers.
- Move the GS drumkit fallback map to generic audio code for re-use
- Added comments and debug message
Lands of Lore sends several drumkit changes to Roland GS
devices with incorrect drumkit numbers. This would work
on some earlier devices like the SC-55, because they correct
these invalid selections. This change emulates the
corrections of the SC-55, so that the drumkit changes will
work on later Roland GS devices.
// Program change on rhythm channel (drumkit selection)
else if (!_isMT32 && channel == 0x09) {
// Apply GS drumkit fallback to correct invalid drumkit numbers.
param1 = MidiDriver::_gsDrumkitFallbackMap[param1];
param1ToSend = MidiDriver::_gsDrumkitFallbackMap[param1];
debugC(kDebugLevelSound, "[Midi] Selected drumkit %i (requested %i)", param1ToSend, param1);
}

This comment has been minimized.

Copy link
@athrxx

athrxx Feb 13, 2020

Member

Would you please change that into debugC(7, kDebugLevelSound, ....)?
All Kyra debug messages have debug levels, so this should have one, too.

This comment has been minimized.

Copy link
@NMIError

NMIError Feb 13, 2020

Author Contributor

done

@athrxx

This comment has been minimized.

Copy link
Member

athrxx commented Feb 13, 2020

Thanks. The changes to KYRA seems to be trivial enough, so I am okay with these . Are there any good test case tracks for the Lands of Lore music?

I can't comment on the SCI changes. Someone from the SCI team will review those...

@NMIError

This comment has been minimized.

Copy link
Contributor Author

NMIError commented Feb 13, 2020

I haven't played much of the game yet, so I haven't found any yet. The character selection screen after the intro switches to the Room drumkit, but right before the drums are actually used it switches back to Standard... There is probably more later in the game.

The GM sound effects also use alternate drumkits, but ScummVM doesn't seem to use those.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 16, 2020

Nice work! The SCI changes look good. Since the KYRA changes are OK as well, this can be merged.

@bluegr bluegr merged commit c646bda into scummvm:master Feb 16, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.