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 import: various small improvements #2006

Merged
merged 7 commits into from Jun 17, 2015

Conversation

Projects
None yet
4 participants
@softrabbit
Member

softrabbit commented Apr 24, 2015

A little mixed bag of short modifications here, longer descriptions are with their respective commits.

  • Don't call AutomationPattern::addObject all the time
  • Ensure minimum note length 1 tick
  • Add naming of AutomationTracks
  • Call qApp->processEvents() before creating tracks (as @Wallacoloo hinted, #1971 (comment))
  • Set pitch bend range to 2 semitones on instrument load, GM compatible (half of issue #1967)
Show outdated Hide outdated plugins/MidiImport/MidiImport.cpp
@@ -394,8 +401,8 @@ bool MidiImport::readSMF( TrackContainer* tc )
{
smfMidiChannel * ch = chs[evt->chan].create( tc, trackName );
Alg_note_ptr noteEvt = dynamic_cast<Alg_note_ptr>( evt );
Note n( noteEvt->get_duration() * ticksPerBeat,
double ticks = MidiTime( noteEvt->get_duration() * ticksPerBeat );

This comment has been minimized.

@Wallacoloo

Wallacoloo Apr 27, 2015

Member

Why is ticks declared as double instead of int? MidiTime only offers an int() cast operator, so casting to double should not give any benefit.

@Wallacoloo

Wallacoloo Apr 27, 2015

Member

Why is ticks declared as double instead of int? MidiTime only offers an int() cast operator, so casting to double should not give any benefit.

This comment has been minimized.

@softrabbit

softrabbit Apr 27, 2015

Member

Aimed for the least possible change here. noteEvt->get_duration() returns a double, so there was a double going in to the constructor before.

@softrabbit

softrabbit Apr 27, 2015

Member

Aimed for the least possible change here. noteEvt->get_duration() returns a double, so there was a double going in to the constructor before.

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Apr 27, 2015

Member

Just built your branch, and attempting to import "Sylver - Love is an Angel" from http://www.eurokdj.com/ringtones/index_midis.php reliably results in a crash, whereas it works with only warnings in master from a few days ago (edit: it works in current master as well):

FlpImport::tryImport(): not a valid FL project

Error reading song: song node not found
MidiImport::tryImport(): found MThd
MISSING GLOBAL THINGY
lmms: /home/colin/proj/lmms/plugins/MidiImport/portsmf/allegro.cpp:555: char *Alg_event::get_atom_value(): Assertion `get_update_type() == 'a'' failed.
Aborted (core dumped)

v.s. expected output:

FlpImport::tryImport(): not a valid FL project

Error reading song: song node not found
MidiImport::tryImport(): found MThd
MISSING GLOBAL HANDLER
     Chn: -1, Type Code: 9, Time: 0.000000, Update Type: sysexs
fluidsynth: error: There is no preset with bank number 0 and preset number 38 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 80 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 64 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 81 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 119 in SoundFont 0
fluidsynth: error: There is no preset with bank number 128 and preset number 0 in SoundFont 0

Note that I don't have any soundfonts installed (I've never used midi in lmms before).

Member

Wallacoloo commented Apr 27, 2015

Just built your branch, and attempting to import "Sylver - Love is an Angel" from http://www.eurokdj.com/ringtones/index_midis.php reliably results in a crash, whereas it works with only warnings in master from a few days ago (edit: it works in current master as well):

FlpImport::tryImport(): not a valid FL project

Error reading song: song node not found
MidiImport::tryImport(): found MThd
MISSING GLOBAL THINGY
lmms: /home/colin/proj/lmms/plugins/MidiImport/portsmf/allegro.cpp:555: char *Alg_event::get_atom_value(): Assertion `get_update_type() == 'a'' failed.
Aborted (core dumped)

v.s. expected output:

FlpImport::tryImport(): not a valid FL project

Error reading song: song node not found
MidiImport::tryImport(): found MThd
MISSING GLOBAL HANDLER
     Chn: -1, Type Code: 9, Time: 0.000000, Update Type: sysexs
fluidsynth: error: There is no preset with bank number 0 and preset number 38 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 80 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 64 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 81 in SoundFont 0
fluidsynth: error: There is no preset with bank number 0 and preset number 119 in SoundFont 0
fluidsynth: error: There is no preset with bank number 128 and preset number 0 in SoundFont 0

Note that I don't have any soundfonts installed (I've never used midi in lmms before).

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Apr 27, 2015

Member

Also, I'd recommend that you add some inline documentation explaining why the code calls qApp->processEvents() if it's not too much trouble. Might be useful if imports are ever refactored to be threaded separately.

Member

Wallacoloo commented Apr 27, 2015

Also, I'd recommend that you add some inline documentation explaining why the code calls qApp->processEvents() if it's not too much trouble. Might be useful if imports are ever refactored to be threaded separately.

@softrabbit

This comment has been minimized.

Show comment
Hide comment
@softrabbit

softrabbit Apr 28, 2015

Member

@Wallacoloo, #1984 is the fix for that crash. Left my variant of that code out of this as that part was taken care of elsewhere. Guess I could rebase this now.

Member

softrabbit commented Apr 28, 2015

@Wallacoloo, #1984 is the fix for that crash. Left my variant of that code out of this as that part was taken care of elsewhere. Guess I could rebase this now.

softrabbit added some commits Apr 24, 2015

MIDI import: don't call AutomationPattern::addObject all the time
Should be enough to add the object to the automation pattern only once,
as the objModel variable that is added will always be the same for the
same value of ccid, which indexes the ccs array.

This will speed up creation of automation tracks for MIDI CCs and pitch
bend.
MIDI import: ensure minimum note length 1 tick
Too short notes had their duration rounded down to 0 on import,
as MIDI precision allows way shorter notes than LMMS, and
portsmf reports the duration as a double, where a beat == 1.0.

Was going to use ceil() first, but that might round some notes
up to a slightly longer duration.
MIDI import: add naming of AutomationTracks
Name the automation tracks like "[MIDI trackname] CC ##". And don't
call the automation track creation function for every CC event, as
it now will involve constructing a QString.
MIDI import: call qApp->processEvents(); before creating tracks
This should keep LMMS responsive from the window managers POV, unless
the user selects a huge enough default sound font that loading it
takes too long.
MIDI import: set default pitch bend range to +/-2 semitones
AFAIK, this is how the General MIDI standard says it should be.
Clarified the reason for qApp->processEvents(),
changed an odd type juggling into something slightly saner.
@softrabbit

This comment has been minimized.

Show comment
Hide comment
@softrabbit

softrabbit May 18, 2015

Member

Added a few tweaks suggested by @midi-pascal and @tresf on the lmms-dev mailing list.

Member

softrabbit commented May 18, 2015

Added a few tweaks suggested by @midi-pascal and @tresf on the lmms-dev mailing list.

@midi-pascal

This comment has been minimized.

Show comment
Hide comment
@midi-pascal

midi-pascal Jun 17, 2015

Contributor

This would be very nice to have this PR integrated to the master branch.
I use it a lot and it works like a charm.
@tresf Do you think it is feasible since this PR has no side effect?

Contributor

midi-pascal commented Jun 17, 2015

This would be very nice to have this PR integrated to the master branch.
I use it a lot and it works like a charm.
@tresf Do you think it is feasible since this PR has no side effect?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jun 17, 2015

Member

@tresf Do you think it is feasible since this PR has no side effect?

Absolutely, merged. Added to changelog.

FYI, @softrabbit has the ability to merge his own, so this is more a matter of waiting on peer review.

Member

tresf commented Jun 17, 2015

@tresf Do you think it is feasible since this PR has no side effect?

Absolutely, merged. Added to changelog.

FYI, @softrabbit has the ability to merge his own, so this is more a matter of waiting on peer review.

tresf added a commit that referenced this pull request Jun 17, 2015

Merge pull request #2006 from softrabbit/midi_import
Midi import: various small improvements

@tresf tresf merged commit 05d4b13 into LMMS:master Jun 17, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@softrabbit

softrabbit Jun 17, 2015

Member

FYI, @softrabbit has the ability to merge his own, so this is more a matter of waiting on peer review.

Are you sure? I always get the text "Only those with write access to this repository can merge pull requests." and no obvious way to merge, so I've assumed that's the way it's meant to be.

Member

softrabbit commented Jun 17, 2015

FYI, @softrabbit has the ability to merge his own, so this is more a matter of waiting on peer review.

Are you sure? I always get the text "Only those with write access to this repository can merge pull requests." and no obvious way to merge, so I've assumed that's the way it's meant to be.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jun 17, 2015

Member

Are you sure? I always get the text "Only those with write access to this repository can merge pull requests." and no obvious way to merge, so I've assumed that's the way it's meant to be.

Sorry. Fixed. :)

FYI, @softrabbit NOW has the ability to merge his own~~, so this is more a matter of waiting on peer review.~~

Member

tresf commented Jun 17, 2015

Are you sure? I always get the text "Only those with write access to this repository can merge pull requests." and no obvious way to merge, so I've assumed that's the way it's meant to be.

Sorry. Fixed. :)

FYI, @softrabbit NOW has the ability to merge his own~~, so this is more a matter of waiting on peer review.~~

@midi-pascal

This comment has been minimized.

Show comment
Hide comment
@midi-pascal

midi-pascal Jun 18, 2015

Contributor

Wonderfull 👍

Contributor

midi-pascal commented Jun 18, 2015

Wonderfull 👍

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