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

Performance: Slow MIDI import due to message being shown repeatedly #2028

Closed
michaelgregorius opened this Issue Apr 30, 2015 · 2 comments

Comments

Projects
None yet
3 participants
@michaelgregorius
Contributor

michaelgregorius commented Apr 30, 2015

The MIDI import is quite slow due to TextFloat::displayMessage being called repeatedly in AutomationPattern::addObject.

An example MIDI that can be used to reproduce the issue can be found at http://www.vgmusic.com/music/console/nintendo/wii/mkwii-koopacape.mid (please also refer to #1971 for further sources). If you comment out the call to TextFloat::displayMessage performance improves quite a lot.

I assume that the current implementation of AutomationPattern::addObject was originally meant to be used in interactive cases where the user wants to create a connection that is already there and that it was not meant to be called from "non-interactive" code like the MIDI import.

The simplest solution might be to remove the message altogether. In my opinion even in an interactive case the information provided by the displayed message is not really useful to the user. If the user connects something that's already connected the system should simply do nothing because in the end it would still have the same effect that the user intended.

If the message should be kept for interactive cases there are two ways I can think of to improve the performance for the MIDI import case:

  1. Let AutomationPattern::addObject return a boolean that indicates whether the model was added and remove the call to TextFloat::displayMessage. In case the model is not added the caller may decide what to do. In an interactive context the message could still be shown whereas in an non-interactive context, e.g. the MIDI import, the result is just ignored and no message is shown.
  2. Add a boolean parameter to AutomationPattern::addObject which indicates whether it's called from an interactive context which means that the message should be shown or a non-interactive context which means that no messages are shown.

I would prefer the first solution because it keeps the usage of AutomationPattern::addObject more flexible, i.e. it would be simpler to implement some third, forth, etc. kind of behaviours in other callers. It also provides a better separation between presentation and functionality.

Any thoughts?

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Apr 30, 2015

Member

I don't deal with midi files enough to know if it's worth leaving the message in under interactive mode or not, but if you do leave the message in interactive modes, I agree with your decision to use option 1 over option 2.

Especially seeing as how AutomationPattern is in the core and not the gui, AutomationPattern should really be totally unaware of the existence of the gui's textFloat class.

Member

Wallacoloo commented Apr 30, 2015

I don't deal with midi files enough to know if it's worth leaving the message in under interactive mode or not, but if you do leave the message in interactive modes, I agree with your decision to use option 1 over option 2.

Especially seeing as how AutomationPattern is in the core and not the gui, AutomationPattern should really be totally unaware of the existence of the gui's textFloat class.

michaelgregorius added a commit to michaelgregorius/lmms that referenced this issue May 3, 2015

Solves issue #2028 (Slow MIDI import due to repeated message)
AutomationPattern::addObject now returns a boolean which indicates
whether the object was added or not. This change enables the removal of
the error message that is shown in the case that a model is already
connected from AutomationPattern::addObject. Instead all interactive
callers now check for the return value and show the message in case it
is needed.

This change set improves the import of MIDI files significantly. These
have been slowed down quite a lot due to the message being shown
repeatedly during the MIDI import.

michaelgregorius added a commit to michaelgregorius/lmms that referenced this issue May 3, 2015

tresf added a commit that referenced this issue May 5, 2015

Merge pull request #2033 from michaelgregorius/2028-slow_midi_import
Solves issue #2028 (Slow MIDI import due to repeated message)
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jun 17, 2015

Member

Closed via #2033

Member

tresf commented Jun 17, 2015

Closed via #2033

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