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

Fix MIDI export #3733

Merged
merged 8 commits into from Sep 13, 2017

Conversation

Projects
None yet
5 participants
@PhysSong
Member

PhysSong commented Jul 28, 2017

Fixes #2384.
Zero-length note support needs some work, per #2074. See #2074 (comment).
Also MidiFile.hpp needs correct copyright information. See #2384 (comment).

@PhysSong PhysSong added this to the 1.2.0 milestone Jul 29, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 29, 2017

Member

Wonderful!

Issues:
An exported track opens in MusE transposed down 2 octaves. The demo project c3.mmp has one note, c3. It will export as midi note 36 (c3.mid) which, by the most adopted standard, translates to c1. c3 should probably export as note 60. The c3 in c3.mmp seem to be the correct pitch ~130Hz.

c3.mmp.zip c3.mid.zip

http://computermusicresource.com/midikeys.html
Related: #1857

Member

zonkmachine commented Jul 29, 2017

Wonderful!

Issues:
An exported track opens in MusE transposed down 2 octaves. The demo project c3.mmp has one note, c3. It will export as midi note 36 (c3.mid) which, by the most adopted standard, translates to c1. c3 should probably export as note 60. The c3 in c3.mmp seem to be the correct pitch ~130Hz.

c3.mmp.zip c3.mid.zip

http://computermusicresource.com/midikeys.html
Related: #1857

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 30, 2017

Member

#1857 (comment)
Possible (temporary) fix: add 12 to every note pitch.

Another issue of this PR: actually master pitch is ignored now. I'll put master pitch support if needed.

Member

PhysSong commented Jul 30, 2017

#1857 (comment)
Possible (temporary) fix: add 12 to every note pitch.

Another issue of this PR: actually master pitch is ignored now. I'll put master pitch support if needed.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 30, 2017

Member

Possible (temporary) fix: add 12 to every note pitch.

👍

Another issue of this PR: actually master pitch is ignored now. I'll put master pitch support if needed.

Yes. I think master pitch needs to go in there.

Member

zonkmachine commented Jul 30, 2017

Possible (temporary) fix: add 12 to every note pitch.

👍

Another issue of this PR: actually master pitch is ignored now. I'll put master pitch support if needed.

Yes. I think master pitch needs to go in there.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 31, 2017

Member

I tried automation of master pitch. Maybe this isn't intended to work yet but here goes...

Automation of master pitch: midimasterpitch.mmp.zip
Note pitch comes out with the same pitch modulation as is presently when you press export MIDI.
To reproduce. Play and pause at random positions. Right click on master pitch fader and read off the value copy value(..). Export MIDI...
Expected: modulated from original C4 over a larger range, same result every time and not depending of start position.

Member

zonkmachine commented Jul 31, 2017

I tried automation of master pitch. Maybe this isn't intended to work yet but here goes...

Automation of master pitch: midimasterpitch.mmp.zip
Note pitch comes out with the same pitch modulation as is presently when you press export MIDI.
To reproduce. Play and pause at random positions. Right click on master pitch fader and read off the value copy value(..). Export MIDI...
Expected: modulated from original C4 over a larger range, same result every time and not depending of start position.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 31, 2017

Member

Yes. Processing automation is difficult, so current method(based on previous one) only exports notes.
I'll try to include these automations or merge it without master pitch support.

Member

PhysSong commented Jul 31, 2017

Yes. Processing automation is difficult, so current method(based on previous one) only exports notes.
I'll try to include these automations or merge it without master pitch support.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 31, 2017

Member

or merge it without master pitch support.

Maybe that's a good idea actually. The basic functions are there and midi export works.

Member

zonkmachine commented Jul 31, 2017

or merge it without master pitch support.

Maybe that's a good idea actually. The basic functions are there and midi export works.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 2, 2017

Member

I need to change some things:

  • Instrument pitch should be exported as a MIDI CC.
  • It is not clear whether MIDI export should transpose notes according to base notes, or not.
  • MidiFile.hpp doesn't support MIDI CC. In order to export automation patterns, I should modify this file a lot, or add some more functions.
  • Moreover, exporting automation patterns is complicated and difficult. For complete MIDI export, such things must be implemented.

So there's two choices about automation:

  1. Merge this PR without automation support and implement it later.
  2. Postpone merging until full automation support is implemented.
Member

PhysSong commented Aug 2, 2017

I need to change some things:

  • Instrument pitch should be exported as a MIDI CC.
  • It is not clear whether MIDI export should transpose notes according to base notes, or not.
  • MidiFile.hpp doesn't support MIDI CC. In order to export automation patterns, I should modify this file a lot, or add some more functions.
  • Moreover, exporting automation patterns is complicated and difficult. For complete MIDI export, such things must be implemented.

So there's two choices about automation:

  1. Merge this PR without automation support and implement it later.
  2. Postpone merging until full automation support is implemented.
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Aug 2, 2017

Member
  1. Merge this PR without automation support and implement it later.

I vote this. With 'later' here meaning in master.

Member

zonkmachine commented Aug 2, 2017

  1. Merge this PR without automation support and implement it later.

I vote this. With 'later' here meaning in master.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Aug 4, 2017

Member

Merge this PR without automation support and implement it later.

I vote on this too, midi automation can happen when we expose the midi CC messages in the instruments #1159 (part of #1472)

It is not clear whether MIDI export should transpose notes according to base notes, or not.

That's a tricky one, I usually use the base note to tune the actual instrument, but I also sometimes use it to actually shift an instrument a couple of octaves up or down, so I don't really know about this, we could do it as an option, but midi export doesn't have any dialog so far.

Member

Umcaruje commented Aug 4, 2017

Merge this PR without automation support and implement it later.

I vote on this too, midi automation can happen when we expose the midi CC messages in the instruments #1159 (part of #1472)

It is not clear whether MIDI export should transpose notes according to base notes, or not.

That's a tricky one, I usually use the base note to tune the actual instrument, but I also sometimes use it to actually shift an instrument a couple of octaves up or down, so I don't really know about this, we could do it as an option, but midi export doesn't have any dialog so far.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 5, 2017

Member

I'm trying to implement automation export. If it is finished before rc4 is released, I'll merge this PR with automation support.
If not, this PR should be merged without automation support.

I vote this. With 'later' here meaning in master.

It should be. However, MIDI export without automation support will be buggy for some projects.
When it have been tested enough and works well, we can(and should, in my opinion) backport it later.

Member

PhysSong commented Aug 5, 2017

I'm trying to implement automation export. If it is finished before rc4 is released, I'll merge this PR with automation support.
If not, this PR should be merged without automation support.

I vote this. With 'later' here meaning in master.

It should be. However, MIDI export without automation support will be buggy for some projects.
When it have been tested enough and works well, we can(and should, in my opinion) backport it later.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 10, 2017

Member

It is not clear whether MIDI export should transpose notes according to base notes, or not.

That's a tricky one

InstrumentTrack::processOutEvent() applies both base note and master pitch. According to this, I think the answer should be "yes".

I want to remove MidiFile.hpp from this project and write a new class for this purpose. That file doesn't support MIDI CC and have some bugs. I want to implement a better exporting plug-in :)
If the schedule matters, I'll instrument pitch support and merge it.

Member

PhysSong commented Aug 10, 2017

It is not clear whether MIDI export should transpose notes according to base notes, or not.

That's a tricky one

InstrumentTrack::processOutEvent() applies both base note and master pitch. According to this, I think the answer should be "yes".

I want to remove MidiFile.hpp from this project and write a new class for this purpose. That file doesn't support MIDI CC and have some bugs. I want to implement a better exporting plug-in :)
If the schedule matters, I'll instrument pitch support and merge it.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Aug 17, 2017

Member

I want to remove MidiFile.hpp from this project and write a new class for this purpose. That file doesn't support MIDI CC and have some bugs. I want to implement a better exporting plug-in :)
If the schedule matters, I'll instrument pitch support and merge it.

I think you should solve this the simplest possible way and leave rewrites to a later release.

Member

zonkmachine commented Aug 17, 2017

I want to remove MidiFile.hpp from this project and write a new class for this purpose. That file doesn't support MIDI CC and have some bugs. I want to implement a better exporting plug-in :)
If the schedule matters, I'll instrument pitch support and merge it.

I think you should solve this the simplest possible way and leave rewrites to a later release.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 24, 2017

Member

Do I need to update README.md in this PR?

Member

PhysSong commented Aug 24, 2017

Do I need to update README.md in this PR?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 24, 2017

Member

Do I need to update README.md in this PR?

Good catch. Sure.

Member

tresf commented Aug 24, 2017

Do I need to update README.md in this PR?

Good catch. Sure.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 3, 2017

Member

Is there anything to do?

Member

PhysSong commented Sep 3, 2017

Is there anything to do?

@Delta-Psi

This comment has been minimized.

Show comment
Hide comment
@Delta-Psi

Delta-Psi Sep 5, 2017

A way to choose the exported instrument for each track would be really useful. It could be selected in the misc section, below the master pitch toggle, along with a check for the track to be exported in the first place.

Delta-Psi commented Sep 5, 2017

A way to choose the exported instrument for each track would be really useful. It could be selected in the misc section, below the master pitch toggle, along with a check for the track to be exported in the first place.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 5, 2017

Member

It will be a good feature, but won't be in this PR because it is a fix for broken MIDI export, not a enhancement. Furthermore, it will be merged into stable-1.2, which is on a feature freeze now.

However it can be done in master later.

Member

PhysSong commented Sep 5, 2017

It will be a good feature, but won't be in this PR because it is a fix for broken MIDI export, not a enhancement. Furthermore, it will be merged into stable-1.2, which is on a feature freeze now.

However it can be done in master later.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 7, 2017

Member

Is there anything to do?

I think it's good enough to merge.

Just one observation when exporting shorties/Crunk(Demo).
You may want to take a look at the note length of the last beat note in a pattern. It gets a length of one whole measure. It would probably make more sense to have them cut off at the end of the measure.

Tracks Kick 01 and Clap 04 (Imported intor MusE) extends to measure 9.

crunklmms

Member

zonkmachine commented Sep 7, 2017

Is there anything to do?

I think it's good enough to merge.

Just one observation when exporting shorties/Crunk(Demo).
You may want to take a look at the note length of the last beat note in a pattern. It gets a length of one whole measure. It would probably make more sense to have them cut off at the end of the measure.

Tracks Kick 01 and Clap 04 (Imported intor MusE) extends to measure 9.

crunklmms

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 7, 2017

Member

Yes. I will take a look at it and fix it soon.

Member

PhysSong commented Sep 7, 2017

Yes. I will take a look at it and fix it soon.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 9, 2017

Member

This is the only thing left for rc4 now. I will fix the issue above and merge it tomorrow. No objection?

Member

PhysSong commented Sep 9, 2017

This is the only thing left for rc4 now. I will fix the issue above and merge it tomorrow. No objection?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 9, 2017

Member

This is the only thing left for rc4 now. I will fix the issue above and merge it tomorrow. No objection?

No objection.

There is also the issue with recording single stream instruments.

Member

zonkmachine commented Sep 9, 2017

This is the only thing left for rc4 now. I will fix the issue above and merge it tomorrow. No objection?

No objection.

There is also the issue with recording single stream instruments.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 11, 2017

Member

As a simple workaround, I'm considering cutting notes at the end of last BBTCO, or something similar.

Member

PhysSong commented Sep 11, 2017

As a simple workaround, I'm considering cutting notes at the end of last BBTCO, or something similar.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 11, 2017

Member

As a simple workaround, I'm considering cutting notes at the end of last BBTCO, or something similar.

I'm not that bothered by it. I think we can ship RC4 without a fix for this. MIDI export is a complex function and there will be a bunch of things that needs to be fixed anyway.

Member

zonkmachine commented Sep 11, 2017

As a simple workaround, I'm considering cutting notes at the end of last BBTCO, or something similar.

I'm not that bothered by it. I think we can ship RC4 without a fix for this. MIDI export is a complex function and there will be a bunch of things that needs to be fixed anyway.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 11, 2017

Member

I'm considering cutting notes at the end of last BBTCO

... Sounds like a good idea. 👍

Member

zonkmachine commented Sep 11, 2017

I'm considering cutting notes at the end of last BBTCO

... Sounds like a good idea. 👍

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 11, 2017

Member

there will be a bunch of things that needs to be fixed anyway.

Yes, maybe. Anyway, cutting notes at the end of last BBTCO or whole song will be much better. More things should be done in master and can be backported whenever needed.

Member

PhysSong commented Sep 11, 2017

there will be a bunch of things that needs to be fixed anyway.

Yes, maybe. Anyway, cutting notes at the end of last BBTCO or whole song will be much better. More things should be done in master and can be backported whenever needed.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 13, 2017

Member

May I merge it tomorrow?

Member

PhysSong commented Sep 13, 2017

May I merge it tomorrow?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 13, 2017

Member

If you are happy with the last commit I think you should just go ahead and merge it now.

Member

zonkmachine commented Sep 13, 2017

If you are happy with the last commit I think you should just go ahead and merge it now.

@PhysSong PhysSong merged commit c0682c9 into LMMS:stable-1.2 Sep 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PhysSong PhysSong deleted the PhysSong:midiexport branch Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment