#8860 closed patch
GUI/LAUNCHER: Midi device selection
| Reported by: | athrxx | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This patch adds a popup widget to the launcher which allows selection of a specific midi device if the driver supports this and also device volume controls if the selected device supports this.
The main purpose for this addition is being able to switch between a MT-32 for older titles and a GM device for games like Sam & Max, HoF and IHNM.
The internal device detection and selection has to be done by MidiDriver. I implemented this for MidiDriver_WIN only (and of course the virtual stuff in MidiDriver). I don't know how to do this for other systems. It should be a simple enough matter though (at least it was for Windows).
Please check this out. I am open to all kinds of suggestions. The launcher controls could obviously be done in lots of different ways (I just put them in places where they'd fit and where they looked okay for me).
Florian
Ticket imported from: #1956501. Ticket imported from: patches/965.
Attachments (5)
Change History (46)
by , 16 years ago
| Attachment: | midicfg.patch added |
|---|
comment:1 by , 16 years ago
| Owner: | set to |
|---|
comment:2 by , 16 years ago
Cool. I can't apply the patch on my system right now (various other things are floating in my checkout around right now), but this is one of the features I wanted for quite some time.
From a brief review of the code, it seems like a good start. Eventually, though, I'd prefer if we could list all available devices in one go. That is, the average user should not have to first figure out the right driver, then select a device. Rather, he should get a single list of all available "devices", including supported "fake" devices (like adlib, pcspk, ...), single-device-drivers (MT-32 emu, fluidsynth, CoreAudio...), and finally a list of "real" MIDI devices (so there wouldn't be a "Windows MIDI driver" entry anymore, but rather multiple entries for each device supported by it, including any software emulation ones). I am not sure whether I succeed at explaining what I mean here, though :-). Nor do I necessarily say you have to do any of that, but I'd love to hear your opinion on this.
Small remark: you don't have to check for NULL before deleting something, so you can remove the "if" from the following if (_devs) delete [] _devs;
Eventually, I would like to make a bigger overhaul to the Music/MIDI devices system (like moving the code for querying available Music drivers & devices to the OSystem API. But that doesn't have to stop this patch :)
comment:3 by , 16 years ago
I did like that idea of yours a lot, so I made a new patch yesterday. Drivers like the "Windows Midi Driver" will be directly resolved into devices and shown in the same menu. Same goes for the entries in scummvm.ini. Device volume controls stay the same more or less (my only device that supports this seems to be the Microsoft GS softsynth that I don't really use, so this hasn't been tested too thoroughly).
Florian
File Added: midicfg_V2.patch
comment:4 by , 16 years ago
I have a patch hanging around to do a similar thing for ALSA, but it wasn't ready to be published (and I didn't spend too much time on the GUI side ;)
One question I'd like to arise is that just showing the device name could add some ambiguity, for example on systems where the same physical device is available via different drivers (like OSS/ALSA). Another case is when using Timidity through ALSA (it's what I do), which could be confused with the Timidity driver. I propose showing the list of devices with a format like "Driver: Device" or similar, like "Windows Midi: Microsoft GS softsynth" and "ALSA: Timidity", for example.
Other than that, I've started working on the MIDI plugins, and I think the device listing should be done from the plugin interface (like the game detection is done in the engine plugins), before creating the driver instance.
comment:5 by , 16 years ago
I agree with the first part. Here is a new version of the patch that takes care of the possible ambigouity problem. The driver name will be put stored at the end of the string in brackets (I like that one better than the other way round, but that could be changed if I am the only one who thinks like this :-) ).
I can't really say anything regarding the second part since I have no clue what this is about (what is a "MIDI plugin" ?). Maybe I missed something from devel here. But I can assure you "pro forma" that I wouldn't mind at all to move any parts of this code to other files/places, expecially if this is needed or useful for something you're planning to do. File Added: midicfg_V3.patch
comment:6 by , 16 years ago
The MIDI plugins aim to remove the hardcoded values and arrays in sound/mididrv.* by making each driver to describe itself. I've just committed in revision 31993 an initial interface. You should be able to easily add a getDevices method to the Windows MIDI plugin (I've also included the ALSA implementation). This information isn't used yet, but it's a start.
comment:7 by , 16 years ago
Would be really nice to get this into SVN eventually. Athrxx, would you be willing to use jvprat's new API? Or should I review the patch as is?
comment:8 by , 16 years ago
Okay, I assumed this item had been rendered sort of obsolete by jvprat's midi plugin project (wasn't midi device selection the whole point of this?).
Since you don't seem to think so ;-) I am going to update this patch tomorrow evening or Saturday as soon as I find the time for this.
comment:9 by , 16 years ago
Btw, the plugins aren't able to return MidiDriverType values atm. Wouldn't it make lots of sense to simply have the plugins return a MidiDriverDescription struct instead of all these getId(), getName(), getXX()? (Naming seems to be inconsistent with 'name', 'id', etc. from MidiDriverDescription btw.) Does anyone mind if I make that change?
comment:10 by , 16 years ago
Yes, I would mind!
The MidiDriverDescription is supposed to go! It is insufficient for our needs, which is one of the reasons for MidiPlugins in the first place.
comment:11 by , 16 years ago
In the current API you should use MidiDriverFlags instead of the MidiDriverType, and it's returned by getCapabilities() is the function you're looking for. All the devices will be considered having the same "capabilities".
You're right, it would make sense to return a structure for each device, and some way to reference them (to save the selected device to a file, for example), but it isn't done yet. The music plugins API isn't mature, and most things still need to be designed. I'll try to dedicate some time to it this weekend. I'm sorry for the inconvenience.
by , 16 years ago
| Attachment: | midicfg_V4.patch added |
|---|
comment:12 by , 16 years ago
Okay I have written a completely new patch, very small, just the basics. I have dropped device volume control for now. I can always add this later when I am less clueless about which parts of the midi code are to be removed or rewritten. This will also require some extension to the MusicDevice class.
The important changes are all in gui\options.cpp, sound\mididrv.h and backends\midi\windows.h.
The things I did to sound\mididrv.cpp, sound\midiplugin.h and sound\midiplugin.cpp are all temporary hacks sort of since I needed to somehow pass MidiDriver::detectMusicDriver() and again I was totally clueless whether this function and those MidiDriverFlags are meant to be removed or not. File Added: midicfg_V4.patch
comment:14 by , 14 years ago
I was looking into applying your patch, but today MidiDriverDescription is gone. Could you please bring the patch up to date?
comment:15 by , 14 years ago
Oh man, this is a really old patch. I'll try to look into it over the next days and try to do a rewrite.
comment:16 by , 14 years ago
Okay, here is what I came up with. This patch is somewhat larger than the others since it contains several general changes to the driver detection code. This is far from perfect (regarding cleanup etc. and is obviously not very much tested), but should be a good start. Writing an even larger patch doesn’t make too much sense atm, as long as I don’t know whether this is going in the right direction or not and whether this can be worked on in the repository or not.
Backend code is provided for Windows only since I am clueless about other platforms and can’t test these either. This part shouldn’t be too difficult to do, though. Backend authors might want to take a quick look at the changes in windows.h and they will surely know what to do ;-). This concerns only platforms which have devices anyway (although you might do things like having sound fonts as devices for fluidsynth if you wanted to get rid of that sound font button in the launcher).
In addition to selecting a specific audio device you can now select a default MT32 device and a default GM device in the options dialogue. Engines can pass MDT_PREFER_MT32 or MDT_PREFER_GM to detectMusicDriver() . If no specific device is selected detectMusicDriver() will select the default MT32 or GM device as a fall back. This allows most targets to be simply run on ‘<default>’ and still getting MT32 or GM sound as needed. I haven’t implemented the new flags for all engines, since I am not always sure which sort of device is needed. I just tried with Kyra, Scumm and Simon and these seem to work fine.
I have dropped ‘FM Towns” from the selection menu since this makes no sense. This is as specific driver for Loom FM-Towns and is useless for all other games. Loom Fm-Towns will use the correct driver automatically.
You can still select MIDI sound for Agi, Cine, Maniac Mansion etc. Maybe the engines should have the supported drivers hard coded in the detection tables (or something like that ). But this is really not the purpose of this patch.
Florian
comment:17 by , 14 years ago
Strange, some of my latest adds in options.cpp just got removed. Maybe TortoiseSVN did that. So, here is a version of the patch that should be okay.
comment:18 by , 14 years ago
I just put a typedef in MidiDriver and did some renaming to make thing a bit more transparent to engine authors. But I will now stop making changes until I know what you guys think about the way this patch is going.
comment:19 by , 14 years ago
Hmm, this still seems to have issues (mt-32 emulator music is somehow broken). I don't know the cause for this and have to look into it some more.
comment:20 by , 14 years ago
Okay that last one was just leaky in general. This is now a restructured and somewhat cleaned up version which tries to avoid messing around with the MusicPlugin interface.
comment:22 by , 14 years ago
Another update since the last changes in the M4 engine code seem to break the last version
comment:23 by , 14 years ago
Thanks, I plan to look at this now. However, the patch causes compile errors, because you seem to have removed the return type in some function declaration: - Common::Error createInstance(MidiDriver **mididriver) const; + createInstance(MidiDriver **mididriver, MidiDriver::DeviceHandle = 0) const;
Not sure how that even compiles for you, but it's easy enough for me to undo this, so I'll give it a try now :)
comment:24 by , 14 years ago
Some more things:
* In base/plugins.cpp you add LINK_PLUGIN(AUTO) twice -- a mistake?
* The patch contained some keycode changes in KyraEngine_v1::setupKeyMap -- again, a mistake?
* With the patch, it seems that now instead of MusicPluginObject::getId(), the code writes the result of getName() into the config file -- this is not how it is intended (the ID is meant to be for the computer, a unique identifier; the name is meant to be a human readable string). At least for me it wrote "CoreAudio" into the config file, instead of "core", which breaks compatibility with older ScummVM versions. This should be reverted to using the driver id, not the name. I guess you need a kFullDeviceId for that or something like that.
* Reverting this would probably also allow removing the "backward compatibility" code in _the code which sets up _midiPopUp around line 200
* There is some serious code duplication for the code which populates the now four MIDI device popups; and also for the code which reads the value in there. This could be reduced considerably by adding a method or function which takes the popup, the config domain name, and a "predicate" (i.e. a pointer to a function or a C++ functor, see common/func.h) which identifies the drivers based on some desired property.
Apart from this, the patch looks pretty good to me. Almost ready for inclusion. I understand that you are reluctant to put much further effort into this based on how long it took to review this properly. If you don't plan to continue working on this, I would try to fix the things i listed, but i have no idea when I would get around to that; reviewing an updated patch, though, should be relatively quick ;).
comment:25 by , 14 years ago
I admit that I was sort of reluctant when I got sev's message about this patch 3 weeks ago, since I had more or less forgotten about it. But now having done a complete rewrite and being more or less familiar with that stuff again I don't mind doing some more work here.
The reason why getCompleteName() and not getId() gets stored in the config file is that getCompleteName() returns not only the "driver type" but also the device name. With getId() we simply cannot save the name of the selected device, In the case of core audio you may not be aware about the device related stuff since device detection code for this is not implemented (if there are devices with core audio - I don't know anything about core audio).
If you're sure what you want/need here I can make the necessary changes and fix the other things you mentioned. (Everything compiles very well here ;-) I can only test with MSVC and MinGW both of which aren't too sensitive about anything).
comment:26 by , 14 years ago
Okay, this version of the patch should at least not have any mistakes/ errors in it. The missing return type functions were only contained in code portions that don't get compiled with windows (backends/midi/<non-windows>). This is the reason why I did not get any errors.
I'll try to reduce code duplication over the next couple of days / weekend.
comment:27 by , 14 years ago
Eugene just came back from his vacation and made some major changes to ScummVM, so it might be a tad *cough* more work to update the patch now. Sorry for that :-/.
As for getCompleteName() vs. getId() -- yeah, I understand that it does not suffice to just store the driver id in the config file. My point is that the current way to serialize the driver+device data is incorrect: It combines the human readable name of the driver with a device specifier; but it really should combined the machine readable ID of the driver with a device specifier.
getCompleteName() is only good for display to the user, but not for serialzing; a new method (e.g. getCompleteId() ) is needed, along with code to unserialize this modified format.
Also, I would expect that for drivers without devices, a plain driver id w/o a device specifier would be accepted; in fact, I'd hope that if the "default device" is selected for a driver (always the case for drivers without devices), then the serializing code would write only the driver id, without a device specifier, for backward compatibility. However, that is kind of optional.
comment:28 by , 14 years ago
That was really cruel ;-) Nonetheless I did manage an update for my code yesterday. I'll try some code reduction in options.cpp and will post an update here over the next couple of days.
comment:29 by , 14 years ago
Okay, here is a new version. I added translation tags to some devices like the null and the auto device ("no sound" / "<default>") and to some emulator devices since translation makes some sense here. I looked at the backend devices too and found that there ist not one of these that requires translation so I did not add anything there (if you feel that things like "tapwave zodiac", "windows midi", "stmidi" or "coreaudio" should be properly translated I can still add this, as long as I am not the poor guy doing the actual translation :-) ).
comment:30 by , 14 years ago
Hmm, I'm just testing with my other PC which has much more music devices and it seems that my latest efforts to reduce code size in options.cpp have broken the widgets. I'll have to look into this and then post an update.
comment:31 by , 14 years ago
Here is a fixed version. I really seem to have problem when I copy files from a svn repo to another place that sometimes my file changes somehow get reset. But I am not sure whether I can blame this on TortoiseSVN. Anyway, I hope I managed to get all the latest changes in this patch.
comment:32 by , 14 years ago
Looks pretty good to me!
There is one major issue remaining: The options dialog is messed up when using 1x mode, esp. with aspect ratio correction off -- 320x200 is just a tad too small ;). I'll try to improve it a little bit by reducing the overall padding / spacing, but I am afraid it looks as if that won't be enough. We may have to rearrange the options dialog or so.
Anyway, if you promise to stay around and help resolve that, if necessary, I'll be happy to commit the patch now :)
comment:33 by , 14 years ago
Hmpf, you're right. I never use 1x mode.. Since the theme seems to be off by only one half of a line we might be able to fix this by smaller spacing. Or we move 'sub title speed' to the graphics tab. Or we remove the 'Adlib Emulator' line (why do we need two emulators anyway).
Anyway, I'll be around if I am supposed to fix anything.
Btw (off topic): how do I set up address cloaking in IRC? Is that done via a Nickserv command?
comment:34 by , 14 years ago
It's only off by half a line when you are in 320x240 mode. If you disable aspect ratio correction, i.e., switch to 320x200 (by pressing ctrl-alt-A, for example), it's more like 2-3 lines missing.
However, I just played around a bit with the dialog layout and it is possible to cramp everything in, although the result looks rather ugly & crowded. Not ideal, but that means it is solvable. I'll look into this tonight, but you are of course also welcome to make a fix :)
As for the off-topic: Best to ask such things via email; please email Eugene and me about this.
comment:35 by , 14 years ago
BTW, I also committed this in my local git-svn tree, and will push it to the main repos tonight, when I am back home from work.
comment:36 by , 14 years ago
I committed your patch to SVN. However, this introduced a severe regression (which I should have noticed earlier, but didn't; luckily, Johannes paid close attention ;-). Namely, it is now impossible to select PC speaker or PCjr as output devices in the GUI, and using them via the command line apparently leads to crashes / endless loops. Unless you can promise a quick fix, I may have to back the change out again until this is resolvable. :-(
comment:37 by , 14 years ago
I'll have a look at this tonight after work. Shouldn't be too difficult to fix.
comment:38 by , 14 years ago
Okay, I hope the regression is fixed. At least Kyra seems to work now.
What we don't have atm is CMS / Creative music system support, because I am somewhat cluesless about this one. There seems to be neither a backend device nor an emulator device. So if I am supposed to do something about that one I'd need some directions ;-)
comment:40 by , 12 years ago
| Status: | new → closed |
|---|
comment:41 by , 5 years ago
| Component: | → GUI |
|---|
launcher patch