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 Export feature (issue #258) #1686

Merged
merged 6 commits into from Feb 7, 2015

Conversation

Projects
None yet
9 participants
@mohamed--abdel-maksoud

mohamed--abdel-maksoud commented Jan 24, 2015

Background: #258

This is an implementation of a basic MIDI export feature in LMMS.

Remarks:

  • only instrument tracks are considered.
  • LMMS tracks are mapped 1-to-1 to MIDI tracks.

Your feedback is welcome.

@diizy

This comment has been minimized.

Show comment
Hide comment
@diizy

diizy Jan 24, 2015

Contributor

Why is there python code in this commit?

Not every platform comes with python. That's an additional dependency that seems unnecessary to add. I don't think we can merge this as it is now...

It would be a better idea to implement this as a file-export plugin. At the very least, get rid of the python...

Contributor

diizy commented Jan 24, 2015

Why is there python code in this commit?

Not every platform comes with python. That's an additional dependency that seems unnecessary to add. I don't think we can merge this as it is now...

It would be a better idea to implement this as a file-export plugin. At the very least, get rid of the python...

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Jan 24, 2015

The python script is a standalone utility to convert mmp files to MIDI. The LMMS program has no dependency on it.

mohamed--abdel-maksoud commented Jan 24, 2015

The python script is a standalone utility to convert mmp files to MIDI. The LMMS program has no dependency on it.

@diizy

This comment has been minimized.

Show comment
Hide comment
@diizy

diizy Jan 24, 2015

Contributor

If it's standalone, then I'm not sure if it should be in our main source tree... maybe a separate repository for it would be a better idea.

Contributor

diizy commented Jan 24, 2015

If it's standalone, then I'm not sure if it should be in our main source tree... maybe a separate repository for it would be a better idea.

Show outdated Hide outdated include/MidiFile.hpp
@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Jan 24, 2015

cool, I'll make a new pull request with the script removed.

mohamed--abdel-maksoud commented Jan 24, 2015

cool, I'll make a new pull request with the script removed.

@diizy

This comment has been minimized.

Show comment
Hide comment
@diizy

diizy Jan 24, 2015

Contributor

Also, it'd be better to implement this functionality as a plugin. Look at how midi-import is implemented, IIRC it's done as a file-import plugin. Midi-export can be implemented as a file-export-plugin.

This improves modularity of code and makes it easier to change and develop things later...

Contributor

diizy commented Jan 24, 2015

Also, it'd be better to implement this functionality as a plugin. Look at how midi-import is implemented, IIRC it's done as a file-import plugin. Midi-export can be implemented as a file-export-plugin.

This improves modularity of code and makes it easier to change and develop things later...

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Jan 24, 2015

sounds good, yet I've no more time to invest in LMMS atm.

mohamed--abdel-maksoud commented Jan 24, 2015

sounds good, yet I've no more time to invest in LMMS atm.

@eagles051387

This comment has been minimized.

Show comment
Hide comment
@eagles051387

eagles051387 Jan 26, 2015

You also do not need to create another pull request. You can make the
changes that were suggested above and commit them and the pull request
automatically gets updated as well.

On Sun, Jan 25, 2015 at 12:41 AM, Mohamed Abdel Maksoud <
notifications@github.com> wrote:

sounds good, yet I've not more time to invest in LMMS atm.


Reply to this email directly or view it on GitHub
#1686 (comment).

Jonathan Aquilina

eagles051387 commented Jan 26, 2015

You also do not need to create another pull request. You can make the
changes that were suggested above and commit them and the pull request
automatically gets updated as well.

On Sun, Jan 25, 2015 at 12:41 AM, Mohamed Abdel Maksoud <
notifications@github.com> wrote:

sounds good, yet I've not more time to invest in LMMS atm.


Reply to this email directly or view it on GitHub
#1686 (comment).

Jonathan Aquilina

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Jan 26, 2015

Thanks eagles, I noticed my commits appear here.
I've already committed the changes I can do now.

mohamed--abdel-maksoud commented Jan 26, 2015

Thanks eagles, I noticed my commits appear here.
I've already committed the changes I can do now.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jan 29, 2015

Member

@mohamed--abdel-maksoud this feature would be tremendous to have ready for stable-1.2 testing.

A few things... Travis seems to have some win32 compilation errors as seen here: jobs/48204778#L4905.

As @diizy mentioned, we would also need the code moved to plugins/MidiExport to remain consistent with our plugins architecture. Here's an example: plugins/MidiImport

Member

tresf commented Jan 29, 2015

@mohamed--abdel-maksoud this feature would be tremendous to have ready for stable-1.2 testing.

A few things... Travis seems to have some win32 compilation errors as seen here: jobs/48204778#L4905.

As @diizy mentioned, we would also need the code moved to plugins/MidiExport to remain consistent with our plugins architecture. Here's an example: plugins/MidiImport

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Jan 29, 2015

@tresf the error on win32 is trivial, I just fixed it.
I'll start looking at refactoring code as a plugin from next week if it is too necessary.

mohamed--abdel-maksoud commented Jan 29, 2015

@tresf the error on win32 is trivial, I just fixed it.
I'll start looking at refactoring code as a plugin from next week if it is too necessary.

@dednikko

This comment has been minimized.

Show comment
Hide comment
@dednikko

dednikko Jan 29, 2015

I apologize if this is seen as too far off topic, but:

@mohamed, THANK YOU! I know that this can be a pain to redo, and I want to
thank you for working on it.

From what I see, it would be best implemented as a plugin.

I just want you to know I am very excited to see this feature happen. You
will make a lot of users extremely happy!

/tangent
On Jan 29, 2015 3:26 PM, "Mohamed Abdel Maksoud" notifications@github.com
wrote:

@tresf https://github.com/tresf the error on win32 is trivial, I just
fixed it.
I'll start looking at refactoring code as a plugin from next week if it is
too necessary.


Reply to this email directly or view it on GitHub
#1686 (comment).

dednikko commented Jan 29, 2015

I apologize if this is seen as too far off topic, but:

@mohamed, THANK YOU! I know that this can be a pain to redo, and I want to
thank you for working on it.

From what I see, it would be best implemented as a plugin.

I just want you to know I am very excited to see this feature happen. You
will make a lot of users extremely happy!

/tangent
On Jan 29, 2015 3:26 PM, "Mohamed Abdel Maksoud" notifications@github.com
wrote:

@tresf https://github.com/tresf the error on win32 is trivial, I just
fixed it.
I'll start looking at refactoring code as a plugin from next week if it is
too necessary.


Reply to this email directly or view it on GitHub
#1686 (comment).

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Jan 29, 2015

Thanks @dednikko (: I've been looking to see this feature implemented for some time, too.

mohamed--abdel-maksoud commented Jan 29, 2015

Thanks @dednikko (: I've been looking to see this feature implemented for some time, too.

@unfa

This comment has been minimized.

Show comment
Hide comment
@unfa

unfa Feb 1, 2015

Contributor

This is going to be a serious improvement to LMMS, thank you for getting
your hands dirty to do it, Mohamed! :)
24 sty 2015 23:59 "Mohamed Abdel Maksoud" notifications@github.com
napisał(a):

Background: #258 #258

This is an implementation of a basic MIDI export feature in LMMS.

Remarks:

  • only instrument tracks are considered.
  • LMMS tracks are mapped 1-to-1 to MIDI tracks.

Your feedback is welcome.

You can view, comment on, or merge this pull request online at:

#1686
Commit Summary

  • standalone utility to convert mmp to midi
  • gui change and stub for midi output
  • Merge remote-tracking branch 'upstream/master'
  • c++ header to abstract MIDI format
  • first working version: Export Midi feature maps instrument tracks to
    equivalent MIDI tracks
  • fixing output file name
  • Merge remote-tracking branch 'upstream/master'

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1686.

Contributor

unfa commented Feb 1, 2015

This is going to be a serious improvement to LMMS, thank you for getting
your hands dirty to do it, Mohamed! :)
24 sty 2015 23:59 "Mohamed Abdel Maksoud" notifications@github.com
napisał(a):

Background: #258 #258

This is an implementation of a basic MIDI export feature in LMMS.

Remarks:

  • only instrument tracks are considered.
  • LMMS tracks are mapped 1-to-1 to MIDI tracks.

Your feedback is welcome.

You can view, comment on, or merge this pull request online at:

#1686
Commit Summary

  • standalone utility to convert mmp to midi
  • gui change and stub for midi output
  • Merge remote-tracking branch 'upstream/master'
  • c++ header to abstract MIDI format
  • first working version: Export Midi feature maps instrument tracks to
    equivalent MIDI tracks
  • fixing output file name
  • Merge remote-tracking branch 'upstream/master'

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1686.

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 3, 2015

Contributor

Hi @mohamed--abdel-maksoud, it's not exactly required but could you try to squash some commits together and rename others?

Keeping our git history sane is very important so that we are able to track regressions in an easy way.

There is a nice tutorial on how to use git rebase -i to achieve this. Thanks for the contribution, let me know if you have any questions.

Some references:

Contributor

badosu commented Feb 3, 2015

Hi @mohamed--abdel-maksoud, it's not exactly required but could you try to squash some commits together and rename others?

Keeping our git history sane is very important so that we are able to track regressions in an easy way.

There is a nice tutorial on how to use git rebase -i to achieve this. Thanks for the contribution, let me know if you have any questions.

Some references:

Show outdated Hide outdated src/core/Song.cpp
@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 3, 2015

@badosu I just pushed a change where midi export is a plugin, I think this modularizes the part in question.
I agree, the commits are quite messy so far, I'll try to organize them.

mohamed--abdel-maksoud commented Feb 3, 2015

@badosu I just pushed a change where midi export is a plugin, I think this modularizes the part in question.
I agree, the commits are quite messy so far, I'll try to organize them.

qDebug() << "failed to load midi export filter!";
return;
}
exf->tryExport(tracks, Engine::getSong()->getTempo(), export_filename);

This comment has been minimized.

@badosu

badosu Feb 3, 2015

Contributor

Thanks! In case we need to implement single track MIDI export feature this will come handy.

@badosu

badosu Feb 3, 2015

Contributor

Thanks! In case we need to implement single track MIDI export feature this will come handy.

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 4, 2015

it builds fine now, I tried to organize the commits but I guess I didn't do it right. If this is an issue I'll create a new fork and commit the changes anew.

mohamed--abdel-maksoud commented Feb 4, 2015

it builds fine now, I tried to organize the commits but I guess I didn't do it right. If this is an issue I'll create a new fork and commit the changes anew.

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 4, 2015

Contributor

Hi mohamed, I noticed something has happened there and the history got all messed up.

So I took all your changes and created a patch file for them, it's here: https://gist.github.com/badosu/120eaf72dc3847049ce0.

I made some commands so that you are able to restore your changes to an incorrupt state:

git checkout upstream/master
git checkout -b add-midi-export
curl https://gist.githubusercontent.com/badosu/120eaf72dc3847049ce0/raw/35020a3c0d325d451864b5f68621ce9558419e16/gistfile1.diff | git apply

Now you should be able to add files and commit them the way you want and open a new PR with everything fixed. Or you can fix the current PR as well.

Contributor

badosu commented Feb 4, 2015

Hi mohamed, I noticed something has happened there and the history got all messed up.

So I took all your changes and created a patch file for them, it's here: https://gist.github.com/badosu/120eaf72dc3847049ce0.

I made some commands so that you are able to restore your changes to an incorrupt state:

git checkout upstream/master
git checkout -b add-midi-export
curl https://gist.githubusercontent.com/badosu/120eaf72dc3847049ce0/raw/35020a3c0d325d451864b5f68621ce9558419e16/gistfile1.diff | git apply

Now you should be able to add files and commit them the way you want and open a new PR with everything fixed. Or you can fix the current PR as well.

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 4, 2015

thanks @badosu, I applied the commands, merged them and pushed to master, I'm not sure though how to delete my prior commits (I'm used to svn, so git can be challenging at times)

mohamed--abdel-maksoud commented Feb 4, 2015

thanks @badosu, I applied the commands, merged them and pushed to master, I'm not sure though how to delete my prior commits (I'm used to svn, so git can be challenging at times)

@eagles051387

This comment has been minimized.

Show comment
Hide comment
@eagles051387

eagles051387 Feb 4, 2015

you can do a git reset which should clear out those other commits.

On Wed, Feb 4, 2015 at 1:50 PM, Mohamed Abdel Maksoud <
notifications@github.com> wrote:

thanks @badosu https://github.com/badosu, I applied the commands,
merged them and pushed to master, I'm not sure though how to delete my
prior commits (I'm used to svn, so git can be challenging at times)


Reply to this email directly or view it on GitHub
#1686 (comment).

Jonathan Aquilina

eagles051387 commented Feb 4, 2015

you can do a git reset which should clear out those other commits.

On Wed, Feb 4, 2015 at 1:50 PM, Mohamed Abdel Maksoud <
notifications@github.com> wrote:

thanks @badosu https://github.com/badosu, I applied the commands,
merged them and pushed to master, I'm not sure though how to delete my
prior commits (I'm used to svn, so git can be challenging at times)


Reply to this email directly or view it on GitHub
#1686 (comment).

Jonathan Aquilina

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 4, 2015

thanks @eagles051387 , git reset seems to do the trick

mohamed--abdel-maksoud commented Feb 4, 2015

thanks @eagles051387 , git reset seems to do the trick

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 4, 2015

Contributor

Really nice work @mohamed--abdel-maksoud, let me test this feature.

Contributor

badosu commented Feb 4, 2015

Really nice work @mohamed--abdel-maksoud, let me test this feature.

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu
Contributor

badosu commented Feb 4, 2015

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 4, 2015

@badosu sorry, the cmake cache deceived me, the latest push should restore the changes

mohamed--abdel-maksoud commented Feb 4, 2015

@badosu sorry, the cmake cache deceived me, the latest push should restore the changes

{
QDomElement note = nn.toElement();
if (note.attribute("len", "0") == "0" || note.attribute("vol", "0") == "0") continue;
#if 0

This comment has been minimized.

@badosu

badosu Feb 4, 2015

Contributor

@mohamed--abdel-maksoud Another one here...

@badosu

badosu Feb 4, 2015

Contributor

@mohamed--abdel-maksoud Another one here...

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 4, 2015

Contributor

@mohamed--abdel-maksoud I am seeing some seemingly debug messages, is it related to this PR?

2015-02-04-201819_151x320_scrot

Contributor

badosu commented Feb 4, 2015

@mohamed--abdel-maksoud I am seeing some seemingly debug messages, is it related to this PR?

2015-02-04-201819_151x320_scrot

void MidiExport::error()
{
//qDebug() << "MidiExport error: " << m_error ;

This comment has been minimized.

@badosu

badosu Feb 4, 2015

Contributor

@mohamed--abdel-maksoud I guess you wanted to implement something here?

@badosu

badosu Feb 4, 2015

Contributor

@mohamed--abdel-maksoud I guess you wanted to implement something here?

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 4, 2015

@badosu yes, I cleaned that locally, let me know if there're more of such small fixes before I group them in one commit

mohamed--abdel-maksoud commented Feb 4, 2015

@badosu yes, I cleaned that locally, let me know if there're more of such small fixes before I group them in one commit

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 4, 2015

Contributor

So, I just exported a big track to MIDI and it looks like it works! (https://github.com/SecondFlight/lmms/raw/master/data/projects/CoolSongs/Greippi%20-%20Krem%20Kaakkuja%20%28Second%20Flight%20Remix%29.mmpz)

There are some possible improvements though, it looks like everything is Grand Piano and we may be able to extract some information from the tracks and changing their type, but should be a posterior improvement.

Also another possible improvement can be on how we show the "Export to MIDI" button, we can show this as a new option on the already existent Export Panel instead of a File menu option, that's arguable though.

I tried opening the generated MIDI file on Drumstick (http://sourceforge.net/projects/drumstick/) and it's crashing, I don't know if this is a problem with the file exactly.

Opening on a normal audio player with MIDI support works.

Contributor

badosu commented Feb 4, 2015

So, I just exported a big track to MIDI and it looks like it works! (https://github.com/SecondFlight/lmms/raw/master/data/projects/CoolSongs/Greippi%20-%20Krem%20Kaakkuja%20%28Second%20Flight%20Remix%29.mmpz)

There are some possible improvements though, it looks like everything is Grand Piano and we may be able to extract some information from the tracks and changing their type, but should be a posterior improvement.

Also another possible improvement can be on how we show the "Export to MIDI" button, we can show this as a new option on the already existent Export Panel instead of a File menu option, that's arguable though.

I tried opening the generated MIDI file on Drumstick (http://sourceforge.net/projects/drumstick/) and it's crashing, I don't know if this is a problem with the file exactly.

Opening on a normal audio player with MIDI support works.

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 4, 2015

@badosu Thanks for trying out the feature Amadeus! I just tested with this song and it seems to trigger a bug (the output MIDI has some out of range notes), I'll look into this one of the following days.

mohamed--abdel-maksoud commented Feb 4, 2015

@badosu Thanks for trying out the feature Amadeus! I just tested with this song and it seems to trigger a bug (the output MIDI has some out of range notes), I'll look into this one of the following days.

@badosu

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 4, 2015

Contributor

Thanks for following up Mohamed!

Contributor

badosu commented Feb 4, 2015

Thanks for following up Mohamed!

@diizy

This comment has been minimized.

Show comment
Hide comment
@diizy

diizy Feb 7, 2015

Contributor

Things look good here, good work on modularizing the code

Contributor

diizy commented Feb 7, 2015

Things look good here, good work on modularizing the code

diizy added a commit that referenced this pull request Feb 7, 2015

@diizy diizy merged commit b1a007b into LMMS:master Feb 7, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@badosu

badosu Feb 7, 2015

Contributor

@diizy I was still expecting some feedback from @mohamed--abdel-maksoud, but I guess he can send a PR later to address these issues.

Contributor

badosu commented Feb 7, 2015

@diizy I was still expecting some feedback from @mohamed--abdel-maksoud, but I guess he can send a PR later to address these issues.

@mohamed--abdel-maksoud

This comment has been minimized.

Show comment
Hide comment
@mohamed--abdel-maksoud

mohamed--abdel-maksoud Feb 8, 2015

@diizy @badosu the latest couple of commits should fix the bug: for some reason the song contains notes with negative durations or volumes above the maximum allowed in MIDI, now such notes are trimmed or discarded.

mohamed--abdel-maksoud commented Feb 8, 2015

@diizy @badosu the latest couple of commits should fix the bug: for some reason the song contains notes with negative durations or volumes above the maximum allowed in MIDI, now such notes are trimmed or discarded.

@tresf tresf referenced this pull request Feb 23, 2015

Open

Better MIDI support #1472

2 of 19 tasks complete

@tresf tresf referenced this pull request Apr 4, 2015

Closed

Add MIDI export #258

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 3, 2015

Member

@mohamed--abdel-maksoud it seems that commits mohamed--abdel-maksoud@e048035 and mohamed--abdel-maksoud@9a40363 were added to your branch after this PR was merged and closed, so they aren't in the upstream master branch. Could you issue a new PR that adds these commits?

Member

Umcaruje commented Jul 3, 2015

@mohamed--abdel-maksoud it seems that commits mohamed--abdel-maksoud@e048035 and mohamed--abdel-maksoud@9a40363 were added to your branch after this PR was merged and closed, so they aren't in the upstream master branch. Could you issue a new PR that adds these commits?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jun 10, 2017

Member

@mohamed--abdel-maksoud it seems that commits mohamed--abdel-maksoud/lmms@e048035 and mohamed--abdel-maksoud/lmms@9a40363 were added to your branch after this PR was merged and closed, so they aren't in the upstream master branch. Could you issue a new PR that adds these commits?

@mohamed--abdel-maksoud @Umcaruje These commits seem to be unmerged still. I can't test them right now but I cherry-picked them and they compile fine.

Member

zonkmachine commented Jun 10, 2017

@mohamed--abdel-maksoud it seems that commits mohamed--abdel-maksoud/lmms@e048035 and mohamed--abdel-maksoud/lmms@9a40363 were added to your branch after this PR was merged and closed, so they aren't in the upstream master branch. Could you issue a new PR that adds these commits?

@mohamed--abdel-maksoud @Umcaruje These commits seem to be unmerged still. I can't test them right now but I cherry-picked them and they compile fine.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jun 12, 2017

Member

@Umcaruje @mohamed--abdel-maksoud
I tested the export function with and without these forgotten commits.

Three tracks exported and the resulting track imported below. The output is the correct notes
but piled together in one track.

midiscrambled

Here is the same project exported with commits commits mohamed--abdel-maksoud/lmms@e048035 and mohamed--abdel-maksoud/lmms@9a40363.
The notes are somewhat scambled and there seem to be duplicate patterns in there.

midi

Member

zonkmachine commented Jun 12, 2017

@Umcaruje @mohamed--abdel-maksoud
I tested the export function with and without these forgotten commits.

Three tracks exported and the resulting track imported below. The output is the correct notes
but piled together in one track.

midiscrambled

Here is the same project exported with commits commits mohamed--abdel-maksoud/lmms@e048035 and mohamed--abdel-maksoud/lmms@9a40363.
The notes are somewhat scambled and there seem to be duplicate patterns in there.

midi

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