Fix midi export

[elpescado]

Some time ago I have changed how MIDI export works. Instead of using sequential numbers, I decided to use MIDI notes assigned by drum kit. I was aware that it could break MIDI export for some people, so I have updated drum kits included with Hydrogen to have meaningful MIDI settings.

Unfortunately, I missed some use cases. Instrument settings are saved in songs, so loading song created in previous versions may cause exported MIDI file have only hi bongo notes. The same may happen when using third party drum kit file.

I have created this branch to fix this. When loading song/drum kit, a check is made whether all instruments use the same note. If so, they are assigned sequential MIDI notes starting from 36. That way, exported MIDI file is the same as file exported from Hydrogen 0.9.6.

An automatic test case is included.

I hope this helps. I'd appreciate any help with testing this;)

[elpescado]

This issue is reported here:

Exported MIDI files contain only Hi Bongo note on/note off events
Does MIDI Export Work In Ver 9.7 ???

[mauser]

Hi @elpescado,

Thanks for looking into those midi issues..

i haven't tested this pull request yet, but after looking at the code, i'm thinking that it would be helpful to ask the user (with a dialog) before using setting new midi out values for the export.

Hydrogen is used in so many ways that i wouldn't be surprised if there is someone out there who wants to export a song with all instruments set to the same midi out note (maybe because he uses the piano roll editor a lot...).

[elpescado]

Hi,

I'd rather not show dialog. After all, it complicates code, which in turn complicates testing, and it gets in the way and confuses user. I'd like to think about better solution.

Thank you for your insights about piano roll. I tried to make this fix conservative (I don't want to break things even more;D), to trigger reassigning only if all notes are the same. I couldn't come up with scenario that setting same notes to all instruments would be desirable. I haven't been thinking about piano roll though, as I'm not familiar with that feature. But I imagine notes set in piano roll editor are separate from notes assigned to instrument, so that setting notes in instrument does not affect notes in piano roll. Or am I missing something?

Users can still work around this issue by adding dummy instrument with MIDI note set to some other value to prevent this fix from triggering. I agree that's ugly workaround, but is should work.

An alternative that I have considered was to add combo box during MIDI export. User could select whether use MIDI notes from drumkit or just use sequential numbers. I don't like that idea though, as user has to choose right option on every export which is easy to forget. Then user gets wrong output file, which they discover only after importing that file, and have to go back to hydrogen and repeat the whole procedure once again.

[elpescado]

I have looked into piano roll editor. I have to admit that I am confused.

It seems that "final" note number is calculated as:

(pNote->get_octave() +3 ) * 12 + pNote->get_key() + pNote->get_instrument()->get_midi_out_note() - 60

which is roughly equivalent to:

MIDDLE_C + note_own_value_relative_to_middle_c + instrument_note_relative_to_middle_c

With two remarks:

1. MIDI file export uses just Instrument::get_midi_out_note() whereas MIDI drivers (i.e. realtime MIDI output) use above formula.
2. CoreMIDI driver uses Note::get_midi_key() to perform that computation, whereas other drivers compute that value themselves, leading to code duplication.

Looking at the source code, it seems that piano roll and instrument notes are mutually exclusive - one of them has to be middle C in order to get meaningful output. For example setting note value to D in piano roll, and setting instrument note to E would result in F# note being played. I have to confirm this, but it seems that my patch indeed may break MIDI output.

[mauser]

Hi @elpescado ,

i can understand your concerns about the midi export dialog. Maybe we should introduce a "quick" and "advanced" export?

The quick export just uses the current midi out value (+/- the piano roll "offset") . The advanced export would offer possibilities like the re-adjusting the note values if all out values are equal or overwriting the midi channel for this particular export.

About confusing the user: you have a point when you say that a dialog may confuse the user, but re-assigning new midi values without a notice may also confuse the user, in all other other places, Hydrogen does no implicit adjusting of any values (at least to my knowledge).

About the general midi export / piano editor problem: Yes, it is quite likely that this has been broken by using the midi out values.. i haven't thought about that earlier :-/ The piano roll editor is not sth. that i'm using often..

[elpescado]

I'm afraid that there's no perfect solution.

Every option has the downside of upset some users. I still think reassigning note values has more benefits than hindrances. The only user group that would be negatively impacted that I can think of currently is users of piano roll that do not use MIDI export (as MIDI export doesn't support piano roll). But I may be missing something, so maybe we'll come up with even better solution.

(source: xkcd)

[pablote]

I think that if compatibility is broken in any way, there should be a way for the user to switch to the previous way of doing things, even if the new way is the better/right-way-to-do-it thing.

It could be something like a "Legacy export logic for instruments" checkbox on some oscure "advanced" tab, but as long it's there, people will have an option.

[thijz]

IMHO we should try to respect the 'GM MIDI' convention where possible, but currently there are very few drumkits that respect this midi drum standard.
IF all drumkits were GM MIDI compliant there would be no room for 'interpretation' or confusion

[elpescado]

What I got so far:

I'm fairly satisfied by how it looks, with couple of notes:

1. The warning icon (provided by Qt) doesn't match rest of application. Keep/remove it?
2. The close button is redundant. Keep/remove it?
3. Exact message has to be written;)
4. I thought of adding "Learn more" link to more verbose description of problem. I'm not sure, though, what should happen when user clicks on that link. Open website? Popup window? Maunal? Or is it unnecessary and remove it?

What do you think?

[mauser]

Hey!

That looks great to me! I suppose the close button can be removed.. About the link: We could open the html manual directly at the correspoding location. We haven't implemented sth. like that yet, but it should be possible, since we're shipping the documentation as HTML.

[elpescado]

HI!

I'm almost done. Here's (almost) final version:

I'd like to update manual, but otherwise I consider this ready for testing.

Are there any native English speakers? Does this message make sense?

Incorrect MIDI setup
MIDI out notes are not configured for this drumkit, so exporting this song to MIDI file may fail. Fix this by assigning default values?

[mikotoiii]

It looks good to me except the last line could be read a couple of ways. I think something like this might flow better: "Would you like Hydrogen to automatically fix this by assigning default values?"

…

On Apr 5, 2017 17:59, "elpescado" ***@***.***> wrote: HI! I'm almost done. Here's (almost) final version: [image: zrzut ekranu 2017-04-05 o 22 52 13] <https://cloud.githubusercontent.com/assets/1619113/24726410/9515177a-1a52-11e7-9866-a22dc69f1e44.png> I'd like to update manual, but otherwise I consider this ready for testing. Are there any native English speakers? Does this message make sense? *Incorrect MIDI setup* MIDI out notes are not configured for this drumkit, so exporting this song to MIDI file may fail. Fix this by assigning default values? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#450 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AL8r2NOl1Cq7h1YdcC11hcdfdqqFuN8Aks5rtACtgaJpZM4LDu6r> .

[elpescado]

Ok, I consider this issue ready for testing. Can I ask for code review?

"Learn more" button linking to manual proved to be more complicated than it seemed, as there are multiple language versions of manual, so I decided to leave it.

How to test:
Create song/get old song that has every instrument set to play MIDI note 60. Load that song - info bar should show. Clicking button should set MIDI note values to sequential numbers starting from 36.

Create song with MIDI notes set to some arbitraty values. Check that loading that song does not trigger info box.

BTW. I noticed that demo songs do not have MIDI notes set. Fix them or leave them as they are?

[mauser]

Hi @elpescado!

I've just tested it a little bit and i've stumbled over the following small issues:

• At the moment it is not possible to deactivate the warning completely for all songs (like the development warning). I suppose that most Hydrogen users are not using the midi export at all, so it might be interesting to have an "Do not show this warning again" button in the infobar. Otherwise it will be quite confusing for a lot of people.. Or deactivate the warning if the midi channel is set to -1 ?

• The infobar is quite nice! Thanks for this very good idea. Maybe we can find also other uses for the widget.

• If a song has only one instrument, the warning is always shown

About the wording of the message:

• It talks about the midi values of a drumkit, but what is important are the midi values of a song. So maybe exchange "drumkit" with "song"?
• What about using sth. like "Advice: midi setup" or "Midi setup advice" instead of "Incorrect midi setup" ?
• The text says that the midi values are "not configured". Wouldn't it be better to say that all instrmuments are using the same midi note? This makes it clearer for the user what really the problem is..

[mauser]

About the demo songs: I guess that they have been created before the midi out value has been introduced in hydrogen, so it might make sense to assign midi values in those songs. But maybe we should save those songs not with 0.9.7 or 1.0.0, but with an older version

[elpescado]

Hi,

I have fixed message showing when song has just one instrument.

The most difficult part of this task was to explain this fairly complex situation in short description, so I had to simplify the message. The situation is most likely caused by using default settings from previous versions, not by deliberately setting note values, so I think "not configured" fits here. "Midi setup advice" sounds good IMO. I'm unsure about "song" vs "drumkit". My line of thought was that "drumkit" is part of "song" do "drumkit" is more precise.

[mauser]

Hi,

the pulll request has been merged now. Probably i'm going to add a possibility to de-activate this warning for users who are not using the midi export.

[mauser]

Thanks a lot for the pull request!