MIDI import bug

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

MIDI import bug

midi-pascal
Hi,

There is a problem when importing some MIDI files:
If MIDI events notes are short, i.e. as in a drums track (~ 3 ticks)
they are not imported at all.
They do not make any sound and do not appear in the piano roll view either.

I will try to figure what the problem is since I have a full debug build
at hand (Ubuntu 12.04).
I noticed this issue long time ago (in version 0.9x) but it is still
there in the master branch (up to date).

midi-pascal

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

Raine M. Ekman
Quoting midi-pascal <[hidden email]>:

> There is a problem when importing some MIDI files:
> If MIDI events notes are short, i.e. as in a drums track (~ 3 ticks)
> they are not imported at all.
> They do not make any sound and do not appear in the piano roll view either.
>
> I will try to figure what the problem is since I have a full debug build
> at hand (Ubuntu 12.04).

This PR should fix it: https://github.com/LMMS/lmms/pull/2006

--
[hidden email]
softrabbit on #lmms



------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

midi-pascal
On 15-05-13 04:13 AM, Raine M. Ekman wrote:

> Quoting midi-pascal <[hidden email]>:
>
>> There is a problem when importing some MIDI files:
>> If MIDI events notes are short, i.e. as in a drums track (~ 3 ticks)
>> they are not imported at all.
>> They do not make any sound and do not appear in the piano roll view either.
>>
>> I will try to figure what the problem is since I have a full debug build
>> at hand (Ubuntu 12.04).
> This PR should fix it: https://github.com/LMMS/lmms/pull/2006
>
Applied this PR.
The good news is the short notes issue is solved!

The idea of naming the imported tracks and CC is a very nice feature too!
However the tracks name import does not work as expected:
The tracks are named Track 1, Track2... Track n, even if they have a
name in the original MIDI file.

This issue comes from two minor bugs:
In MidiImport.cpp, the condition:
if( attr == "tracknames" && evt->get_update_type() == 'a' ) {
should be:
if( attr == "tracknames" && evt->get_update_type() == 's' ) {
since the track name is a string, and
trackName = evt->get_atom_value();
should be:
trackName = evt->get_string_value();

But in allegro.cpp, the function get_string_value() has a bug since it
returns the name of the event ("Trackname") instead of its contents.
In get_string_value(), the line:
return update->parameter.attr_name();
should be:
return update->parameter.a;

I fixed these two functions and the result is now what was expected.
The tracks are now displayed with their orginal name and the automation
tracks have the same name
For example, a drums track and its automations now display as:
Drums
Drums CC 7
Drums CC 10

This feature could be enhanced a bit for the standard MIDI controls,
i.e. "CC 7" could be displayed as "Volume" , "CC 10" as "Panning" etc.
like in the automation editor window title.
This would make tracks more readable since not everyone can remember
which CC number correspond to which MIDI control.

Regards,
midi-pascal


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

midi-pascal
On 15-05-13 10:36 AM, midi-pascal wrote:

> On 15-05-13 04:13 AM, Raine M. Ekman wrote:
>> Quoting midi-pascal <[hidden email]>:
>>
>>> There is a problem when importing some MIDI files:
>>> If MIDI events notes are short, i.e. as in a drums track (~ 3 ticks)
>>> they are not imported at all.
>>> They do not make any sound and do not appear in the piano roll view either.
>>>
>>> I will try to figure what the problem is since I have a full debug build
>>> at hand (Ubuntu 12.04).
>> This PR should fix it: https://github.com/LMMS/lmms/pull/2006
>>
> Applied this PR.
> The good news is the short notes issue is solved!
>
> The idea of naming the imported tracks and CC is a very nice feature too!
> However the tracks name import does not work as expected:
> The tracks are named Track 1, Track2... Track n, even if they have a
> name in the original MIDI file.
>
> This issue comes from two minor bugs:
> In MidiImport.cpp, the condition:
> if( attr == "tracknames" && evt->get_update_type() == 'a' ) {
> should be:
> if( attr == "tracknames" && evt->get_update_type() == 's' ) {
> since the track name is a string, and
> trackName = evt->get_atom_value();
> should be:
> trackName = evt->get_string_value();
>
> But in allegro.cpp, the function get_string_value() has a bug since it
> returns the name of the event ("Trackname") instead of its contents.
> In get_string_value(), the line:
> return update->parameter.attr_name();
> should be:
> return update->parameter.a;
>
> I fixed these two functions and the result is now what was expected.
> The tracks are now displayed with their orginal name and the automation
> tracks have the same name
> For example, a drums track and its automations now display as:
> Drums
> Drums CC 7
> Drums CC 10
>
> This feature could be enhanced a bit for the standard MIDI controls,
> i.e. "CC 7" could be displayed as "Volume" , "CC 10" as "Panning" etc.
> like in the automation editor window title.
> This would make tracks more readable since not everyone can remember
> which CC number correspond to which MIDI control.
>
> Regards,
> midi-pascal
>
>
> ------------------------------------------------------------------------------
> One dashboard for servers and applications across Physical-Virtual-Cloud
> Widest out-of-the-box monitoring support with 50+ applications
> Performance metrics, stats and reports that give you Actionable Insights
> Deep dive visibility with transaction tracing using APM Insight.
> http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
> _______________________________________________
> LMMS-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/lmms-devel
>
I forgot to mention that the fixes I did in MidiImport.cpp and and
allegro.cpp cannot have any side effect since the function
get_string_value() is not used anywhere else in lmms.


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

Spekular R

This is pretty incredible, imo. Thanks a ton!


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

midi-pascal
On 15-05-13 11:53 AM, Spekular R wrote:
>
> This is pretty incredible, imo. Thanks a ton!
>
I think there is a small glitch in the PR 2006:
The word "Track" in the default track name is hardcoded so
internationalization will not work for it.
May I suggest to add tr() around the litteral?
i.e.         QString trackName = QString(tr("Track %1")).arg(t);

:-)

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

Tres Finocchiaro
i.e.         QString trackName = QString(tr("Track %1")).arg(t);

Probably best to use tr("Track") + "%1" moving forward so that translators don't try to fix percent signs. :)

-Tres


On Wed, May 13, 2015 at 1:05 PM, midi-pascal <[hidden email]> wrote:
On 15-05-13 11:53 AM, Spekular R wrote:
>
> This is pretty incredible, imo. Thanks a ton!
>
I think there is a small glitch in the PR 2006:
The word "Track" in the default track name is hardcoded so
internationalization will not work for it.
May I suggest to add tr() around the litteral?
i.e.         QString trackName = QString(tr("Track %1")).arg(t);

:-)

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

Raine M. Ekman
In reply to this post by midi-pascal
Quoting midi-pascal <[hidden email]>:

> The idea of naming the imported tracks and CC is a very nice feature too!
> However the tracks name import does not work as expected:
> The tracks are named Track 1, Track2... Track n, even if they have a
> name in the original MIDI file.

It did work back when I first made the PR :) PR #1984 changed stuff,  
and I didn't check after rebasing #2006... my mistake.


> This issue comes from two minor bugs:
> In MidiImport.cpp, the condition:
> if( attr == "tracknames" && evt->get_update_type() == 'a' ) {
> should be:
> if( attr == "tracknames" && evt->get_update_type() == 's' ) {
> since the track name is a string, and
> trackName = evt->get_atom_value();
> should be:
> trackName = evt->get_string_value();
>
> But in allegro.cpp, the function get_string_value() has a bug since it
> returns the name of the event ("Trackname") instead of its contents.
> In get_string_value(), the line:
> return update->parameter.attr_name();
> should be:
> return update->parameter.a;

My solution to that was to just use get_atom_value() and call it a  
day, as that seemed to do what was needed. But your reasoning does  
make sense.



> This feature could be enhanced a bit for the standard MIDI controls,
> i.e. "CC 7" could be displayed as "Volume" , "CC 10" as "Panning" etc.

Sure.


Qoting Tres:
> Probably best to use tr("Track") + "%1" moving forward so that translators
> don't try to fix percent signs. :)

Translation suggestions noted, too.

--
[hidden email]
softrabbit on #lmms



------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

midi-pascal


On 15-05-14 03:45 AM, Raine M. Ekman wrote:

> Quoting midi-pascal <[hidden email]>:
>
>> The idea of naming the imported tracks and CC is a very nice feature too!
>> However the tracks name import does not work as expected:
>> The tracks are named Track 1, Track2... Track n, even if they have a
>> name in the original MIDI file.
> It did work back when I first made the PR :) PR #1984 changed stuff,
> and I didn't check after rebasing #2006... my mistake.
>
>
>> This issue comes from two minor bugs:
>> In MidiImport.cpp, the condition:
>> if( attr == "tracknames" && evt->get_update_type() == 'a' ) {
>> should be:
>> if( attr == "tracknames" && evt->get_update_type() == 's' ) {
>> since the track name is a string, and
>> trackName = evt->get_atom_value();
>> should be:
>> trackName = evt->get_string_value();
>>
>> But in allegro.cpp, the function get_string_value() has a bug since it
>> returns the name of the event ("Trackname") instead of its contents.
>> In get_string_value(), the line:
>> return update->parameter.attr_name();
>> should be:
>> return update->parameter.a;
> My solution to that was to just use get_atom_value() and call it a
> day, as that seemed to do what was needed. But your reasoning does
> make sense.
The tricky thing is get_atom_value() will not work for the trackname
event in debug mode
because there is an assert(get_update_type() == 'a')) in it.
For the trackname event, get_update_type() returns 's'.
So lmms crashes when debuging the MidiImport code.
As you may know, assert() is a no-op when the code is compiled in optimized
mode so the crash does not happen in this case.
This is the reason why I choosed to fix the code this way.

>
>> This feature could be enhanced a bit for the standard MIDI controls,
>> i.e. "CC 7" could be displayed as "Volume" , "CC 10" as "Panning" etc.
> Sure.
>
>
> Qoting Tres:
>> Probably best to use tr("Track") + "%1" moving forward so that translators
>> don't try to fix percent signs. :)
> Translation suggestions noted, too.
>
Will you modify the code in your PR or should I wait for it to be pulled
in the master branch?
Your choice :-)


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel
Reply | Threaded
Open this post in threaded view
|

Re: MIDI import bug

Raine M. Ekman
Quoting midi-pascal <[hidden email]>:
> Will you modify the code in your PR or should I wait for it to be pulled
> in the master branch?
> Your choice :-)

Will push some changes to the PR soon-ish.

--
[hidden email]
softrabbit on #lmms



------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
LMMS-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/lmms-devel