Quantcast

I pulled three of Pokechu22's 12 commits...

classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

I pulled three of Pokechu22's 12 commits...

Paul Licameli
... but with some amandment of one of them.

These are commits for fixing conditional compilations and preferring wxASSERT to assert.

In one place I made compilation fixes by making arguments const-reference instead of value.  I found the commit did not completely fix the compilation for me because a variable was still named wrong.

Thank you for breaking your work into many little commits, each making a sensible step by itself that is meant to keep the project in some meaningful state.  I strive to do similar with big projects.

PRL



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli
Also I committed 5badb91 which was needed to complete compilation fixes for fix-midi-output on Mac, though I am not sure if the fix is correct.

With those changes I can compile and link fix-midi-output; whereas, I can compile but not link master with EXPERIMENTAL_MIDI_OUT.

I want to figure out where the fix for linkage happens in the fix-midi-output branch and cherry-pick that part of it onto master too.

PRL



On Mon, Mar 27, 2017 at 1:31 AM, Paul Licameli <[hidden email]> wrote:
... but with some amandment of one of them.

These are commits for fixing conditional compilations and preferring wxASSERT to assert.

In one place I made compilation fixes by making arguments const-reference instead of value.  I found the commit did not completely fix the compilation for me because a variable was still named wrong.

Thank you for breaking your work into many little commits, each making a sensible step by itself that is meant to keep the project in some meaningful state.  I strive to do similar with big projects.

PRL




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Pokechu22-2
First, thanks for the partial merge; those are some of the more useful
base commits.

However... 5badb91 shouldn't have been needed.  I didn't add that
call; it's been there since 2010 according to git blame (and none of
the pulled commits modified UpdateGain).  I don't know how that would
have worked before my changes, weird.

Assuming that the link error you're referring to is due to a missing
TrackInfo::GainSlider(int) (and not due to PortMidi not existing), the
fix for it occurs in the huge TrackPanel commit (69b30e2), and more or
less has to occur there unfortunately.

To explain what's going on here (since the need for a slider isn't
obvious if you haven't gotten it to work), here's a quick explanation
of the more subtle EXPERIMENTAL_MIDI_OUT changes.

Note tracks gain 2 (technically 1) new properties.  Here's a
screenshot: http://i.imgur.com/OofBNcm.png.  The more obvious one is
the toggle for individual channels, which actually exists even without
EXPERIMENTAL_MIDI_OUT, in a broken way - on master, you can toggle
channels by clicking, even though the buttons aren't visible.
Enabling EXPERIMENTAL_MIDI_OUT lets you see the buttons.

But that's not not the one that causes link problems.  There's also a
slider, which is used to control the volume of note tracks.  For wave
tracks, this slider controls gain; for note tracks, it controls
relative velocity of notes (mVelocity).  To be confusing, the getters
and setters for that called it Gain (perhaps to be the same as wave
tracks), but the ranges on it are very different - gain on wave tracks
ranges from -36 dB to +36 dB, while velocity goes from -50
(http://i.imgur.com/zuWEFXN.png) to +50
(http://i.imgur.com/1yRxHp9.png).  (Actual velocity in midi files
ranges from 0 to 127 if I recall correctly).  There's also a different
style for the slider.

In the past, both the gain slider for wave tracks and the velocity
slider for note tracks were put in an array, and GainSlider(int) was
used to index that array.  It then changed the style of the slider to
a gain or velocity slider (which changed the range and the tooltip) as
needed.  Oh, and it generated and stored a bogus offset rectangle
because GainSlider used a different location than the velocity slider
needed, which makes hit tests complicated.

The array was removed in 8f773342 because it really didn't help
anything, and GainSlider(int) was replaced with a variant that took a
Track and generated a new slider.  However, that only was done for
wave tracks; the velocity slider code wasn't touched (and still used
SetStyle).  Far later, in b28ec295, GainSlider(int) was re-declared
(but not defined) to get EXPERIMENTAL_MIDI_OUT to compile (but not
link).  What _should_ have happened was a dedicated
VelocitySlider(NoteTrack*) method, but that didn't happen.

So, to get everything to work, either a horribly hacky
reimplementation of GainSlider(int) needs to be re-added (including
the offset rectangle), or dedicated VelocitySlider code needs to be
created.  I chose the latter, for obvious reasons.  That does mean a
lot of changes to TrackPanel, but it's needed to get things to compile
in a way that isn't a total hack.

Unfortunately it's not something that can be easily isolated and
cherry-picked; if it were I'd have done it that way.

--Poke

On Mon, Mar 27, 2017 at 1:44 AM, Paul Licameli <paul.licameli@...> wrote:

> Also I committed 5badb91 which was needed to complete compilation fixes for
> fix-midi-output on Mac, though I am not sure if the fix is correct.
>
> With those changes I can compile and link fix-midi-output; whereas, I can
> compile but not link master with EXPERIMENTAL_MIDI_OUT.
>
> I want to figure out where the fix for linkage happens in the
> fix-midi-output branch and cherry-pick that part of it onto master too.
>
> PRL

I'm glad that the individual commits are useful; it's a good habit
I've gotten into.  (I do squash fixes into one commit, but I try to
keep the changes mostly broken down).

The naming actually was correct... when the commit was made.  But it
depended on a commit before that changed the names from r to rect (in
addition to some other changes there, such as making the method return
void), which don't work in isolation.  Leaving the return type as int
and keeping the name as 'r' is correct for the commit in isolation,
and I've already rebased to apply the renaming afterwards.

--Poke

On Mon, Mar 27, 2017 at 1:31 AM, Paul Licameli <paul.licameli@...> wrote:

> ... but with some amandment of one of them.
>
> These are commits for fixing conditional compilations and preferring
> wxASSERT to assert.
>
> In one place I made compilation fixes by making arguments const-reference
> instead of value.  I found the commit did not completely fix the
> compilation for me because a variable was still named wrong.
>
> Thank you for breaking your work into many little commits, each making a
> sensible step by itself that is meant to keep the project in some
> meaningful state.  I strive to do similar with big projects.
>
> PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli


On Mon, Mar 27, 2017 at 1:13 PM, Pokechu22 <[hidden email]> wrote:
First, thanks for the partial merge; those are some of the more useful
base commits.

However... 5badb91 shouldn't have been needed.  I didn't add that
call; it's been there since 2010 according to git blame (and none of
the pulled commits modified UpdateGain).  I don't know how that would
have worked before my changes, weird.

The explanation is that your commit "Split MixerTrackCluster into subclasses" moved UpdateGain() into a proper subclass of MixerTrackCluster but this call remains, though only for Mac build, in the constructor of the base class.

As I improve my understanding, I have reservations about this commit.  It will be very conflicting with other things I have prepared to make common base classes for NoteTrack and WaveTrack.  If followed a suggestion in the comments to split MixerTrackCluster, but I disagree with that suggestion.  Is it really essential for this project?  Can you rebase -i and resequence this commit last so that the rest builds without it?

Is that really all that this commit does?  Why must it also change CommandFlag.h?
 

Assuming that the link error you're referring to is due to a missing
TrackInfo::GainSlider(int) (and not due to PortMidi not existing), the
fix for it occurs in the huge TrackPanel commit (69b30e2), and more or
less has to occur there unfortunately.

To explain what's going on here (since the need for a slider isn't
obvious if you haven't gotten it to work), here's a quick explanation
of the more subtle EXPERIMENTAL_MIDI_OUT changes.

Note tracks gain 2 (technically 1) new properties.  Here's a
screenshot: http://i.imgur.com/OofBNcm.png.  The more obvious one is
the toggle for individual channels, which actually exists even without
EXPERIMENTAL_MIDI_OUT, in a broken way - on master, you can toggle
channels by clicking, even though the buttons aren't visible.
Enabling EXPERIMENTAL_MIDI_OUT lets you see the buttons.

But that's not not the one that causes link problems.  There's also a
slider, which is used to control the volume of note tracks.  For wave
tracks, this slider controls gain; for note tracks, it controls
relative velocity of notes (mVelocity).  To be confusing, the getters
and setters for that called it Gain (perhaps to be the same as wave
tracks), but the ranges on it are very different - gain on wave tracks
ranges from -36 dB to +36 dB, while velocity goes from -50
(http://i.imgur.com/zuWEFXN.png) to +50
(http://i.imgur.com/1yRxHp9.png).  (Actual velocity in midi files
ranges from 0 to 127 if I recall correctly).  There's also a different
style for the slider.

In the past, both the gain slider for wave tracks and the velocity
slider for note tracks were put in an array, and GainSlider(int) was
used to index that array.  It then changed the style of the slider to
a gain or velocity slider (which changed the range and the tooltip) as
needed.  Oh, and it generated and stored a bogus offset rectangle
because GainSlider used a different location than the velocity slider
needed, which makes hit tests complicated.

The array was removed in 8f773342 because it really didn't help
anything, and GainSlider(int) was replaced with a variant that took a
Track and generated a new slider.  However, that only was done for
wave tracks; the velocity slider code wasn't touched (and still used
SetStyle).  Far later, in b28ec295, GainSlider(int) was re-declared
(but not defined) to get EXPERIMENTAL_MIDI_OUT to compile (but not
link).  What _should_ have happened was a dedicated
VelocitySlider(NoteTrack*) method, but that didn't happen.

So, to get everything to work, either a horribly hacky
reimplementation of GainSlider(int) needs to be re-added (including
the offset rectangle), or dedicated VelocitySlider code needs to be
created.  I chose the latter, for obvious reasons.  That does mean a
lot of changes to TrackPanel, but it's needed to get things to compile
in a way that isn't a total hack.

Unfortunately it's not something that can be easily isolated and
cherry-picked; if it were I'd have done it that way.

--Poke

Thank you for this explanation.

Should I be able to build your branch and play a midi file?  No such luck for me yet, on Mac anyway.

 PRL


On Mon, Mar 27, 2017 at 1:44 AM, Paul Licameli <paul.licameli@...> wrote:
> Also I committed 5badb91 which was needed to complete compilation fixes for
> fix-midi-output on Mac, though I am not sure if the fix is correct.
>
> With those changes I can compile and link fix-midi-output; whereas, I can
> compile but not link master with EXPERIMENTAL_MIDI_OUT.
>
> I want to figure out where the fix for linkage happens in the
> fix-midi-output branch and cherry-pick that part of it onto master too.
>
> PRL

I'm glad that the individual commits are useful; it's a good habit
I've gotten into.  (I do squash fixes into one commit, but I try to
keep the changes mostly broken down).

The naming actually was correct... when the commit was made.  But it
depended on a commit before that changed the names from r to rect (in
addition to some other changes there, such as making the method return
void), which don't work in isolation.  Leaving the return type as int
and keeping the name as 'r' is correct for the commit in isolation,
and I've already rebased to apply the renaming afterwards.

--Poke

On Mon, Mar 27, 2017 at 1:31 AM, Paul Licameli <paul.licameli@...> wrote:
> ... but with some amandment of one of them.
>
> These are commits for fixing conditional compilations and preferring
> wxASSERT to assert.
>
> In one place I made compilation fixes by making arguments const-reference
> instead of value.  I found the commit did not completely fix the
> compilation for me because a variable was still named wrong.
>
> Thank you for breaking your work into many little commits, each making a
> sensible step by itself that is meant to keep the project in some
> meaningful state.  I strive to do similar with big projects.
>
> PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Pokechu22-2
On Mon, Mar 27, 2017 at 6:28 PM, Paul Licameli <[hidden email]> wrote:

>
>
> On Mon, Mar 27, 2017 at 1:13 PM, Pokechu22 <[hidden email]> wrote:
>>
>> First, thanks for the partial merge; those are some of the more useful
>> base commits.
>>
>> However... 5badb91 shouldn't have been needed.  I didn't add that
>> call; it's been there since 2010 according to git blame (and none of
>> the pulled commits modified UpdateGain).  I don't know how that would
>> have worked before my changes, weird.
>
>
> The explanation is that your commit "Split MixerTrackCluster into
> subclasses" moved UpdateGain() into a proper subclass of MixerTrackCluster
> but this call remains, though only for Mac build, in the constructor of the
> base class.

Ah, makes sense.  So if I understand you right, it _isn't_ needed to
build the current master, but it will be needed to build my code on
mac eventually.  That makes more sense.  (And, yes, the call doesn't
seem to be needed; UpdateGain is already called earlier in both cases
and I don't think sizing would matter there).

> As I improve my understanding, I have reservations about this commit.  It
> will be very conflicting with other things I have prepared to make common
> base classes for NoteTrack and WaveTrack.  If followed a suggestion in the
> comments to split MixerTrackCluster, but I disagree with that suggestion.
> Is it really essential for this project?  Can you rebase -i and resequence
> this commit last so that the rest builds without it?

It's partially needed.  Without the commit, things build but there are
some invalid casts that are used to support the mixerboard which can
sometimes cause odd behavior (including crashes like "this was
nullptr" and fun stuff like http://i.imgur.com/kQ019Bd.png when you
try to pan a non-pannable notetrack... yea.) when it is opened with a
NoteTrack (though, it also can function entirely normally; it's very
much undefined behavior).  Also, the commit allows me to remove
SetStyle for sliders (that's not an essential change, but I still
think it's a good idea).

Both can be done other ways, so if you've got a cleaner approach, I'd
like to hear it.  Certainly if there is a general AudioTrack, the
existing approach would be nicer.

I've rebased it (and the SetStyle change) to be the last commits; it
does build without it (though, without it you still get the broken
behavior due to invalid casts).

> Is that really all that this commit does?  Why must it also change
> CommandFlag.h?

It needs to change CommandFlag.h and add a new flag because otherwise,
the mixerboard is only enabled when wave tracks exist, meaning if you
only have a note track, it previously wouldn't be enabled.  As far as
I know (but I'm not experienced with wxWidgets) it's not possible to
enable a menu item when either of 2 flags are set, only when both are
set.

>> Assuming that the link error you're referring to is due to a missing
>> TrackInfo::GainSlider(int) (and not due to PortMidi not existing), the
>> fix for it occurs in the huge TrackPanel commit (69b30e2), and more or
>> less has to occur there unfortunately.
>>
>> To explain what's going on here (since the need for a slider isn't
>> obvious if you haven't gotten it to work), here's a quick explanation
>> of the more subtle EXPERIMENTAL_MIDI_OUT changes.
>>
>> Note tracks gain 2 (technically 1) new properties.  Here's a
>> screenshot: http://i.imgur.com/OofBNcm.png.  The more obvious one is
>> the toggle for individual channels, which actually exists even without
>> EXPERIMENTAL_MIDI_OUT, in a broken way - on master, you can toggle
>> channels by clicking, even though the buttons aren't visible.
>> Enabling EXPERIMENTAL_MIDI_OUT lets you see the buttons.
>>
>> But that's not not the one that causes link problems.  There's also a
>> slider, which is used to control the volume of note tracks.  For wave
>> tracks, this slider controls gain; for note tracks, it controls
>> relative velocity of notes (mVelocity).  To be confusing, the getters
>> and setters for that called it Gain (perhaps to be the same as wave
>> tracks), but the ranges on it are very different - gain on wave tracks
>> ranges from -36 dB to +36 dB, while velocity goes from -50
>> (http://i.imgur.com/zuWEFXN.png) to +50
>> (http://i.imgur.com/1yRxHp9.png).  (Actual velocity in midi files
>> ranges from 0 to 127 if I recall correctly).  There's also a different
>> style for the slider.
>>
>> In the past, both the gain slider for wave tracks and the velocity
>> slider for note tracks were put in an array, and GainSlider(int) was
>> used to index that array.  It then changed the style of the slider to
>> a gain or velocity slider (which changed the range and the tooltip) as
>> needed.  Oh, and it generated and stored a bogus offset rectangle
>> because GainSlider used a different location than the velocity slider
>> needed, which makes hit tests complicated.
>>
>> The array was removed in 8f773342 because it really didn't help
>> anything, and GainSlider(int) was replaced with a variant that took a
>> Track and generated a new slider.  However, that only was done for
>> wave tracks; the velocity slider code wasn't touched (and still used
>> SetStyle).  Far later, in b28ec295, GainSlider(int) was re-declared
>> (but not defined) to get EXPERIMENTAL_MIDI_OUT to compile (but not
>> link).  What _should_ have happened was a dedicated
>> VelocitySlider(NoteTrack*) method, but that didn't happen.
>>
>> So, to get everything to work, either a horribly hacky
>> reimplementation of GainSlider(int) needs to be re-added (including
>> the offset rectangle), or dedicated VelocitySlider code needs to be
>> created.  I chose the latter, for obvious reasons.  That does mean a
>> lot of changes to TrackPanel, but it's needed to get things to compile
>> in a way that isn't a total hack.
>>
>> Unfortunately it's not something that can be easily isolated and
>> cherry-picked; if it were I'd have done it that way.
>>
>> --Poke
>
>
> Thank you for this explanation.
>
> Should I be able to build your branch and play a midi file?  No such luck
> for me yet, on Mac anyway.

Yes.  If the play button isn't enabled/clicking it does nothing, it's
probably a configuration error on your end (or on the build script),
but if you click it and get silence or a crash, then it's my fault
probably.

--Poke

>
>  PRL
>
>>
>> On Mon, Mar 27, 2017 at 1:44 AM, Paul Licameli <paul.licameli@...> wrote:
>> > Also I committed 5badb91 which was needed to complete compilation fixes
>> > for
>> > fix-midi-output on Mac, though I am not sure if the fix is correct.
>> >
>> > With those changes I can compile and link fix-midi-output; whereas, I
>> > can
>> > compile but not link master with EXPERIMENTAL_MIDI_OUT.
>> >
>> > I want to figure out where the fix for linkage happens in the
>> > fix-midi-output branch and cherry-pick that part of it onto master too.
>> >
>> > PRL
>>
>> I'm glad that the individual commits are useful; it's a good habit
>> I've gotten into.  (I do squash fixes into one commit, but I try to
>> keep the changes mostly broken down).
>>
>> The naming actually was correct... when the commit was made.  But it
>> depended on a commit before that changed the names from r to rect (in
>> addition to some other changes there, such as making the method return
>> void), which don't work in isolation.  Leaving the return type as int
>> and keeping the name as 'r' is correct for the commit in isolation,
>> and I've already rebased to apply the renaming afterwards.
>>
>> --Poke
>>
>> On Mon, Mar 27, 2017 at 1:31 AM, Paul Licameli <paul.licameli@...> wrote:
>> > ... but with some amandment of one of them.
>> >
>> > These are commits for fixing conditional compilations and preferring
>> > wxASSERT to assert.
>> >
>> > In one place I made compilation fixes by making arguments
>> > const-reference
>> > instead of value.  I found the commit did not completely fix the
>> > compilation for me because a variable was still named wrong.
>> >
>> > Thank you for breaking your work into many little commits, each making a
>> > sensible step by itself that is meant to keep the project in some
>> > meaningful state.  I strive to do similar with big projects.
>> >
>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli
Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which, when undefined, will cause no changes at all in appearance and behavior?  Then please put this conditional compilation in from the beginning of the list of commits.  I am trying to understand "switch to a dedicated velocity slider..." which does not obect this.  I don't know if the later commit completely corrects it.

I see this idiom which I greatly dislike because it confuses the auto-matching of parentheses in my editor:

#ifdef EXPERIMENTAL_MIDI_CONTROLS

   if (((mNextEventTrack->IsVisibleChan(channel)) &&

#else

   if ((

#endif


Do no replicate parentheses with #ifdefs.  Do this instead.

   if ((

#ifdef EXPERIMENTAL_MIDI_CONTROLS

        (mNextEventTrack->IsVisibleChan(channel)) &&

#endif


Thus everything just inside the conditional compilation leg has balanced parentheses.

PRL



On Mon, Mar 27, 2017 at 11:42 PM, Pokechu22 <[hidden email]> wrote:
On Mon, Mar 27, 2017 at 6:28 PM, Paul Licameli <[hidden email]> wrote:
>
>
> On Mon, Mar 27, 2017 at 1:13 PM, Pokechu22 <[hidden email]> wrote:
>>
>> First, thanks for the partial merge; those are some of the more useful
>> base commits.
>>
>> However... 5badb91 shouldn't have been needed.  I didn't add that
>> call; it's been there since 2010 according to git blame (and none of
>> the pulled commits modified UpdateGain).  I don't know how that would
>> have worked before my changes, weird.
>
>
> The explanation is that your commit "Split MixerTrackCluster into
> subclasses" moved UpdateGain() into a proper subclass of MixerTrackCluster
> but this call remains, though only for Mac build, in the constructor of the
> base class.

Ah, makes sense.  So if I understand you right, it _isn't_ needed to
build the current master, but it will be needed to build my code on
mac eventually.  That makes more sense.  (And, yes, the call doesn't
seem to be needed; UpdateGain is already called earlier in both cases
and I don't think sizing would matter there).

> As I improve my understanding, I have reservations about this commit.  It
> will be very conflicting with other things I have prepared to make common
> base classes for NoteTrack and WaveTrack.  If followed a suggestion in the
> comments to split MixerTrackCluster, but I disagree with that suggestion.
> Is it really essential for this project?  Can you rebase -i and resequence
> this commit last so that the rest builds without it?

It's partially needed.  Without the commit, things build but there are
some invalid casts that are used to support the mixerboard which can
sometimes cause odd behavior (including crashes like "this was
nullptr" and fun stuff like http://i.imgur.com/kQ019Bd.png when you
try to pan a non-pannable notetrack... yea.) when it is opened with a
NoteTrack (though, it also can function entirely normally; it's very
much undefined behavior).  Also, the commit allows me to remove
SetStyle for sliders (that's not an essential change, but I still
think it's a good idea).

Both can be done other ways, so if you've got a cleaner approach, I'd
like to hear it.  Certainly if there is a general AudioTrack, the
existing approach would be nicer.

I've rebased it (and the SetStyle change) to be the last commits; it
does build without it (though, without it you still get the broken
behavior due to invalid casts).

> Is that really all that this commit does?  Why must it also change
> CommandFlag.h?

It needs to change CommandFlag.h and add a new flag because otherwise,
the mixerboard is only enabled when wave tracks exist, meaning if you
only have a note track, it previously wouldn't be enabled.  As far as
I know (but I'm not experienced with wxWidgets) it's not possible to
enable a menu item when either of 2 flags are set, only when both are
set.

>> Assuming that the link error you're referring to is due to a missing
>> TrackInfo::GainSlider(int) (and not due to PortMidi not existing), the
>> fix for it occurs in the huge TrackPanel commit (69b30e2), and more or
>> less has to occur there unfortunately.
>>
>> To explain what's going on here (since the need for a slider isn't
>> obvious if you haven't gotten it to work), here's a quick explanation
>> of the more subtle EXPERIMENTAL_MIDI_OUT changes.
>>
>> Note tracks gain 2 (technically 1) new properties.  Here's a
>> screenshot: http://i.imgur.com/OofBNcm.png.  The more obvious one is
>> the toggle for individual channels, which actually exists even without
>> EXPERIMENTAL_MIDI_OUT, in a broken way - on master, you can toggle
>> channels by clicking, even though the buttons aren't visible.
>> Enabling EXPERIMENTAL_MIDI_OUT lets you see the buttons.
>>
>> But that's not not the one that causes link problems.  There's also a
>> slider, which is used to control the volume of note tracks.  For wave
>> tracks, this slider controls gain; for note tracks, it controls
>> relative velocity of notes (mVelocity).  To be confusing, the getters
>> and setters for that called it Gain (perhaps to be the same as wave
>> tracks), but the ranges on it are very different - gain on wave tracks
>> ranges from -36 dB to +36 dB, while velocity goes from -50
>> (http://i.imgur.com/zuWEFXN.png) to +50
>> (http://i.imgur.com/1yRxHp9.png).  (Actual velocity in midi files
>> ranges from 0 to 127 if I recall correctly).  There's also a different
>> style for the slider.
>>
>> In the past, both the gain slider for wave tracks and the velocity
>> slider for note tracks were put in an array, and GainSlider(int) was
>> used to index that array.  It then changed the style of the slider to
>> a gain or velocity slider (which changed the range and the tooltip) as
>> needed.  Oh, and it generated and stored a bogus offset rectangle
>> because GainSlider used a different location than the velocity slider
>> needed, which makes hit tests complicated.
>>
>> The array was removed in 8f773342 because it really didn't help
>> anything, and GainSlider(int) was replaced with a variant that took a
>> Track and generated a new slider.  However, that only was done for
>> wave tracks; the velocity slider code wasn't touched (and still used
>> SetStyle).  Far later, in b28ec295, GainSlider(int) was re-declared
>> (but not defined) to get EXPERIMENTAL_MIDI_OUT to compile (but not
>> link).  What _should_ have happened was a dedicated
>> VelocitySlider(NoteTrack*) method, but that didn't happen.
>>
>> So, to get everything to work, either a horribly hacky
>> reimplementation of GainSlider(int) needs to be re-added (including
>> the offset rectangle), or dedicated VelocitySlider code needs to be
>> created.  I chose the latter, for obvious reasons.  That does mean a
>> lot of changes to TrackPanel, but it's needed to get things to compile
>> in a way that isn't a total hack.
>>
>> Unfortunately it's not something that can be easily isolated and
>> cherry-picked; if it were I'd have done it that way.
>>
>> --Poke
>
>
> Thank you for this explanation.
>
> Should I be able to build your branch and play a midi file?  No such luck
> for me yet, on Mac anyway.

Yes.  If the play button isn't enabled/clicking it does nothing, it's
probably a configuration error on your end (or on the build script),
but if you click it and get silence or a crash, then it's my fault
probably.

--Poke

>
>  PRL
>
>>
>> On Mon, Mar 27, 2017 at 1:44 AM, Paul Licameli <paul.licameli@...> wrote:
>> > Also I committed 5badb91 which was needed to complete compilation fixes
>> > for
>> > fix-midi-output on Mac, though I am not sure if the fix is correct.
>> >
>> > With those changes I can compile and link fix-midi-output; whereas, I
>> > can
>> > compile but not link master with EXPERIMENTAL_MIDI_OUT.
>> >
>> > I want to figure out where the fix for linkage happens in the
>> > fix-midi-output branch and cherry-pick that part of it onto master too.
>> >
>> > PRL
>>
>> I'm glad that the individual commits are useful; it's a good habit
>> I've gotten into.  (I do squash fixes into one commit, but I try to
>> keep the changes mostly broken down).
>>
>> The naming actually was correct... when the commit was made.  But it
>> depended on a commit before that changed the names from r to rect (in
>> addition to some other changes there, such as making the method return
>> void), which don't work in isolation.  Leaving the return type as int
>> and keeping the name as 'r' is correct for the commit in isolation,
>> and I've already rebased to apply the renaming afterwards.
>>
>> --Poke
>>
>> On Mon, Mar 27, 2017 at 1:31 AM, Paul Licameli <paul.licameli@...> wrote:
>> > ... but with some amandment of one of them.
>> >
>> > These are commits for fixing conditional compilations and preferring
>> > wxASSERT to assert.
>> >
>> > In one place I made compilation fixes by making arguments
>> > const-reference
>> > instead of value.  I found the commit did not completely fix the
>> > compilation for me because a variable was still named wrong.
>> >
>> > Thank you for breaking your work into many little commits, each making a
>> > sensible step by itself that is meant to keep the project in some
>> > meaningful state.  I strive to do similar with big projects.
>> >
>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Pokechu22
EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
midi channels.  When it's defined/not defined, it behaves slightly
differently from before it was introduced, since in current master the
channel toggles are half-enabled (you can blindly click to toggle
them).  The intention is that it is stable enough to have enabled by
default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
commit does is change where it's defined so that it's controlled via
EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
doesn't add anything new.

"Switch to a dedicated velocity slider" could probably be split up a
bit, since that commit actually does more than it says (including
_fixing_ the midi controls -- but not putting them behind their own
define).  A lot of those changes are interconnected though (getting
rid of the offset rect and such).  Should I try to split out the midi
controls fixes and bring both them and the define to the start?

I'll change those ifdefs tomorrow.  Should I do the same with method
headers (and brackets on them), if there are any like that, or do
those not cause issues with your editor?  (I vaguely remember some
being that way though I'm not sure if I changed it).

--Poke

On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]> wrote:

> Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which, when
> undefined, will cause no changes at all in appearance and behavior?  Then
> please put this conditional compilation in from the beginning of the list of
> commits.  I am trying to understand "switch to a dedicated velocity
> slider..." which does not obect this.  I don't know if the later commit
> completely corrects it.
>
> I see this idiom which I greatly dislike because it confuses the
> auto-matching of parentheses in my editor:
>
> #ifdef EXPERIMENTAL_MIDI_CONTROLS
>
>    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>
> #else
>
>    if ((
>
> #endif
>
>
> Do no replicate parentheses with #ifdefs.  Do this instead.
>
>    if ((
>
> #ifdef EXPERIMENTAL_MIDI_CONTROLS
>
>         (mNextEventTrack->IsVisibleChan(channel)) &&
>
> #endif
>
>
> Thus everything just inside the conditional compilation leg has balanced
> parentheses.
>
> PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli
I think for my next move I want to cherry pick "Rename gain to velocity in note tracks" and make that part of a branch I want to push, which will introduce new common base classes of WaveTrack and NoteTrack.  This branch will also greatly reduce the number of #ifdef tests of USE_MIDI and EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may have some nontrivial rebasing work to do onto it.

About not playing MIDI, I observe in the debugger on my mac that there was an attempt to open a midi stream in AudioIO.cpp, but it failed and returned an error code.  Maybe I am missing something in expected third-party dynamic libraries or environment variables?  Still it's true at least for me that this is not working out of the box to make midi play work.  So it's not yet a feature.

I am trying to determine what parts of your work are harmless and could be separated from it and merged without #ifdef guards.  But it seems there are changes to the appearance of note tracks that are unconditionally compiled.  If this project does not advance past experimental for this release, then those changes in drawing should not be done.  They should be disabled by some EXPERIMENTAL switch if they are merged at all.

But the simplification of ASlider by removing SetStyle, which depends on the new members of TrackInfo -- that I might merge in, and take it on myself to reconcile my track-panel-refactor with that.

PRL



On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22 <[hidden email]> wrote:
EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
midi channels.  When it's defined/not defined, it behaves slightly
differently from before it was introduced, since in current master the
channel toggles are half-enabled (you can blindly click to toggle
them).  The intention is that it is stable enough to have enabled by
default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
commit does is change where it's defined so that it's controlled via
EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
doesn't add anything new.

"Switch to a dedicated velocity slider" could probably be split up a
bit, since that commit actually does more than it says (including
_fixing_ the midi controls -- but not putting them behind their own
define).  A lot of those changes are interconnected though (getting
rid of the offset rect and such).  Should I try to split out the midi
controls fixes and bring both them and the define to the start?

I'll change those ifdefs tomorrow.  Should I do the same with method
headers (and brackets on them), if there are any like that, or do
those not cause issues with your editor?  (I vaguely remember some
being that way though I'm not sure if I changed it).

--Poke

On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]> wrote:
> Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which, when
> undefined, will cause no changes at all in appearance and behavior?  Then
> please put this conditional compilation in from the beginning of the list of
> commits.  I am trying to understand "switch to a dedicated velocity
> slider..." which does not obect this.  I don't know if the later commit
> completely corrects it.
>
> I see this idiom which I greatly dislike because it confuses the
> auto-matching of parentheses in my editor:
>
> #ifdef EXPERIMENTAL_MIDI_CONTROLS
>
>    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>
> #else
>
>    if ((
>
> #endif
>
>
> Do no replicate parentheses with #ifdefs.  Do this instead.
>
>    if ((
>
> #ifdef EXPERIMENTAL_MIDI_CONTROLS
>
>         (mNextEventTrack->IsVisibleChan(channel)) &&
>
> #endif
>
>
> Thus everything just inside the conditional compilation leg has balanced
> parentheses.
>
> PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli
Poke, please see my latest pushes to master at 8c796e2

You made me accelerate some other saved up work I had wanted to do, making a common base class of WaveTrack and NoteTrack but not of the other kinds of track.  Mute and Solo state should be in this class, not in Track itself. 

This may cause you a bit of grief reworking the MixerBoard, sorry about that.

You will observe far fewer #ifdefs, even fewer than in your version.

I don't think I would make two subclasses of MixerTrackCluster if I were doing it, rather I would switch where needed on track type, but I will leave those details to you.

One small note of style, is that I noticed that you used C-style pointer casts in places.  I prefer not to do that.  I would prefer to see static_cast, because that discipline never allows implicit casting away of const.

Your list of still outstanding commits would shrink by one when you rebase, because I cherry-picked "Rename gain to velocity on note tracks", except partial reversion of it was needed to compile it.  Some things involving mGainPlacementRect belonged in a different commit, I think.

PRL


On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]> wrote:
I think for my next move I want to cherry pick "Rename gain to velocity in note tracks" and make that part of a branch I want to push, which will introduce new common base classes of WaveTrack and NoteTrack.  This branch will also greatly reduce the number of #ifdef tests of USE_MIDI and EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may have some nontrivial rebasing work to do onto it.

About not playing MIDI, I observe in the debugger on my mac that there was an attempt to open a midi stream in AudioIO.cpp, but it failed and returned an error code.  Maybe I am missing something in expected third-party dynamic libraries or environment variables?  Still it's true at least for me that this is not working out of the box to make midi play work.  So it's not yet a feature.

I am trying to determine what parts of your work are harmless and could be separated from it and merged without #ifdef guards.  But it seems there are changes to the appearance of note tracks that are unconditionally compiled.  If this project does not advance past experimental for this release, then those changes in drawing should not be done.  They should be disabled by some EXPERIMENTAL switch if they are merged at all.

But the simplification of ASlider by removing SetStyle, which depends on the new members of TrackInfo -- that I might merge in, and take it on myself to reconcile my track-panel-refactor with that.

PRL



On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22 <[hidden email]> wrote:
EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
midi channels.  When it's defined/not defined, it behaves slightly
differently from before it was introduced, since in current master the
channel toggles are half-enabled (you can blindly click to toggle
them).  The intention is that it is stable enough to have enabled by
default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
commit does is change where it's defined so that it's controlled via
EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
doesn't add anything new.

"Switch to a dedicated velocity slider" could probably be split up a
bit, since that commit actually does more than it says (including
_fixing_ the midi controls -- but not putting them behind their own
define).  A lot of those changes are interconnected though (getting
rid of the offset rect and such).  Should I try to split out the midi
controls fixes and bring both them and the define to the start?

I'll change those ifdefs tomorrow.  Should I do the same with method
headers (and brackets on them), if there are any like that, or do
those not cause issues with your editor?  (I vaguely remember some
being that way though I'm not sure if I changed it).

--Poke

On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]> wrote:
> Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which, when
> undefined, will cause no changes at all in appearance and behavior?  Then
> please put this conditional compilation in from the beginning of the list of
> commits.  I am trying to understand "switch to a dedicated velocity
> slider..." which does not obect this.  I don't know if the later commit
> completely corrects it.
>
> I see this idiom which I greatly dislike because it confuses the
> auto-matching of parentheses in my editor:
>
> #ifdef EXPERIMENTAL_MIDI_CONTROLS
>
>    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>
> #else
>
>    if ((
>
> #endif
>
>
> Do no replicate parentheses with #ifdefs.  Do this instead.
>
>    if ((
>
> #ifdef EXPERIMENTAL_MIDI_CONTROLS
>
>         (mNextEventTrack->IsVisibleChan(channel)) &&
>
> #endif
>
>
> Thus everything just inside the conditional compilation leg has balanced
> parentheses.
>
> PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Pokechu22-2
I've split the EXPERIMENTAL_MIDI_CONTROLS and velocity slider portions
of my changes, and made a few other tweaks to that code (to make its
rect more like the rects for other controls).

For not playing, it seems like there's a complete lack of handling for
when PM returns an error, but the same is also true for PA - it checks
if an error occurred, but doesn't do anything useful with the error
code.  I've added a temporary commit to display a message box with
that error, but it's not an elegant solution and that should be
handled better with both portmidi and portaudio.  I'm not sure what
the best way of doing that is, though (but it's definitely not a
message box in the middle).

Some of the EXPERIMENTAL_MIDI_CONTROLS changes remove things that are
already present... but _shouldn't_ be present by default.  Basically
the feature as-is is half enabled (you can click, but you can't see),
when it should either be fully enabled or not enabled at all.

static_cast is a good idea; I'll switch to that as needed.

I'm not sure why there was rect stuff in "Rename gain to velocity on
note tracks"; it doesn't look like that in a diff on my end (but, the
rect changes happened before that change, so it might be a side-effect
of merging).

--Poke

On Tue, Mar 29, 2017 at 6:40 AM, Paul Licameli <paul.licameli@...> wrote:

> Poke, please see my latest pushes to master at 8c796e2
> You made me accelerate some other saved up work I had wanted to do, making
> a common base class of WaveTrack and NoteTrack but not of the other kinds
> of track.  Mute and Solo state should be in this class, not in Track
> itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will
> leave those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL

On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <paul.licameli@...> wrote:

> I think for my next move I want to cherry pick "Rename gain to velocity in
> note tracks" and make that part of a branch I want to push, which will
> introduce new common base classes of WaveTrack and NoteTrack.  This branch
> will also greatly reduce the number of #ifdef tests of USE_MIDI and
> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
> have some nontrivial rebasing work to do onto it.
>
> About not playing MIDI, I observe in the debugger on my mac that there was
> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
> an error code.  Maybe I am missing something in expected third-party
> dynamic libraries or environment variables?  Still it's true at least for
> me that this is not working out of the box to make midi play work.  So it's
> not yet a feature.
>
> I am trying to determine what parts of your work are harmless and could be
> separated from it and merged without #ifdef guards.  But it seems there are
> changes to the appearance of note tracks that are unconditionally
> compiled.  If this project does not advance past experimental for this
> release, then those changes in drawing should not be done.  They should be
> disabled by some EXPERIMENTAL switch if they are merged at all.
>
> But the simplification of ASlider by removing SetStyle, which depends on
> the new members of TrackInfo -- that I might merge in, and take it on
> myself to reconcile my track-panel-refactor with that.
>
> PRL
>
> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22 <pokechu022+sf-audacity-devel@...> wrote:
>
> > EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
> > midi channels.  When it's defined/not defined, it behaves slightly
> > differently from before it was introduced, since in current master the
> > channel toggles are half-enabled (you can blindly click to toggle
> > them).  The intention is that it is stable enough to have enabled by
> > default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
> > commit does is change where it's defined so that it's controlled via
> > EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
> > doesn't add anything new.
> >
> > "Switch to a dedicated velocity slider" could probably be split up a
> > bit, since that commit actually does more than it says (including
> > _fixing_ the midi controls -- but not putting them behind their own
> > define).  A lot of those changes are interconnected though (getting
> > rid of the offset rect and such).  Should I try to split out the midi
> > controls fixes and bring both them and the define to the start?
> >
> > I'll change those ifdefs tomorrow.  Should I do the same with method
> > headers (and brackets on them), if there are any like that, or do
> > those not cause issues with your editor?  (I vaguely remember some
> > being that way though I'm not sure if I changed it).

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Pokechu22
In reply to this post by Paul Licameli
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:

> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

My foolish mistake, corrected now in master.
 

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

Don't worry about what's already there, just avoid making new C style casts.
 

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

I'm out of my depth with the midi libraries libraries: Roger may be of more help.  My concern is to be sure this development harmonizes with other code reorganizations I want to accomplish.

What I know is that USE_MIDI is defined in 2.1.3 and EXPERIMENTAL_MIDI_OUT is not.  USE_MIDI controls whether the class NoteTrack exists at all.  In 2.1.3, MIDI data can be imported and visualized on a musical staff of sorts, and some editing can be done, and there is even some code to apply time-warping effects, see SoundTouchEffect.cpp.  But there is no playback.

There is also the code for "stretching" a MIDI track.  When you make a selection in a MIDI track, and move the cursor inside the selection to middle height, there is a changed cursor.  You can click and drag left and right.  This does some sort of time warp to the data.  But it behaves oddly, and I think not as intended, but I am not sure what the intent was.  You might like to attempt to figure this code out too.

It should be possible to disable all user-visible changes in your project by compiling with appropriate defines, so I think that means that all changes you make to appearance of note tracks should be controlled by EXPERIMENTAL_MIDI_OUT, or by a new symbol.

Unless you believe you are fixing a bug even in the limited MIDI track features that are already in 2.1.3, such as that strangeness in the stretch tool.

PRL
 

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli
In reply to this post by Pokechu22
I built your changes, and the dialog box I see when I attempt play on my MacBook says

PortMidi: `Invalid device ID'

I haven't tried it on my Windows box yet.

What does the Velocity slider do?  I can't hear it.  Is it redundant with the play-at-speed ("transcription") toolbar?

I do now see the colored buttons and I do see how they show and hide channels.

You say you have not got scrubbing to work.  Have you also tried MIDI playback with the other play commands, such as looping play, cut preview, play-one-second, play-at-speed?

Now that I am displaying MIDI tracks more often, I can think of a few improvements you could attempt, that could be controlled by USE_MIDI, not by EXPERIMENTAL_MIDI_OUT.
  1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave tracks.
  2. Scrolling and magnifying the keyboard scale should work by means of mouse scrollwheel, as with wave tracks.
PRL


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli


On Fri, Mar 31, 2017 at 10:56 AM, Paul Licameli <[hidden email]> wrote:
I built your changes, and the dialog box I see when I attempt play on my MacBook says

PortMidi: `Invalid device ID'

I haven't tried it on my Windows box yet.

What does the Velocity slider do?  I can't hear it.  Is it redundant with the play-at-speed ("transcription") toolbar?

Answering my own question, I see that it relates to the attack of percussive notes, and has nothing to do with tempo.  Ok then.

PRL
 

I do now see the colored buttons and I do see how they show and hide channels.

You say you have not got scrubbing to work.  Have you also tried MIDI playback with the other play commands, such as looping play, cut preview, play-one-second, play-at-speed?

Now that I am displaying MIDI tracks more often, I can think of a few improvements you could attempt, that could be controlled by USE_MIDI, not by EXPERIMENTAL_MIDI_OUT.
  1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave tracks.
  2. Scrolling and magnifying the keyboard scale should work by means of mouse scrollwheel, as with wave tracks.
PRL


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
rbd
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

rbd



On 3/31/17 11:41 AM, Paul Licameli wrote:


On Fri, Mar 31, 2017 at 10:56 AM, Paul Licameli <[hidden email]> wrote:
I built your changes, and the dialog box I see when I attempt play on my MacBook says

PortMidi: `Invalid device ID'
PortMidi is modeled on PortAudio. Devices have ID's that are assigned when PortMidi is initialized. If you saved an ID in preferences and try to reuse it after devices have been added or unplugged, the ID could become invalid. Also, input and output have different ID's - you can't open an input ID for output. - I hope this helps.

I haven't tried it on my Windows box yet.

What does the Velocity slider do?  I can't hear it.  Is it redundant with the play-at-speed ("transcription") toolbar?

Answering my own question, I see that it relates to the attack of percussive notes, and has nothing to do with tempo.  Ok then.

PRL
 

I do now see the colored buttons and I do see how they show and hide channels.

You say you have not got scrubbing to work.  Have you also tried MIDI playback with the other play commands, such as looping play, cut preview, play-one-second, play-at-speed?

Now that I am displaying MIDI tracks more often, I can think of a few improvements you could attempt, that could be controlled by USE_MIDI, not by EXPERIMENTAL_MIDI_OUT.
  1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave tracks.
  2. Scrolling and magnifying the keyboard scale should work by means of mouse scrollwheel, as with wave tracks.
PRL


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot


_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli
At startup, inside pm_macosxcm_init, numDevices does get 3, but numInputs and numOutputs are 0.

In pm_init, default input and output device ids both get set to -1.

That is at startup.  But these functions are visited again when opening preferences.  Pm_Terminate is called, then Pm_Initialize again.  The second time, pm_macosxcm_init returns an error, but this is not returned to the caller of Pm_initialize.

Pm_CountDevices then returns zero.  MidiIOPrefs::mHostNames remains empty.  This causes assertion errors.

I pushed to master a provisional fix for those assertion errors.

The Host drop-down is too narrow, as seen in the attached.

The drop-downs tell me "No MIDI interfaces" and "No devices found".  This happens even if I remove the calls to Pm_Terminate and Pm_Initialize to avoid the re-initialization error.

So is there something lacking in my System preferences or other environment?

The few MIDI files I have can open and play with no problem in GarageBand.

PRL



On Fri, Mar 31, 2017 at 12:08 PM, Roger Dannenberg <[hidden email]> wrote:



On 3/31/17 11:41 AM, Paul Licameli wrote:


On Fri, Mar 31, 2017 at 10:56 AM, Paul Licameli <[hidden email]> wrote:
I built your changes, and the dialog box I see when I attempt play on my MacBook says

PortMidi: `Invalid device ID'
PortMidi is modeled on PortAudio. Devices have ID's that are assigned when PortMidi is initialized. If you saved an ID in preferences and try to reuse it after devices have been added or unplugged, the ID could become invalid. Also, input and output have different ID's - you can't open an input ID for output. - I hope this helps.


I haven't tried it on my Windows box yet.

What does the Velocity slider do?  I can't hear it.  Is it redundant with the play-at-speed ("transcription") toolbar?

Answering my own question, I see that it relates to the attack of percussive notes, and has nothing to do with tempo.  Ok then.

PRL
 

I do now see the colored buttons and I do see how they show and hide channels.

You say you have not got scrubbing to work.  Have you also tried MIDI playback with the other play commands, such as looping play, cut preview, play-one-second, play-at-speed?

Now that I am displaying MIDI tracks more often, I can think of a few improvements you could attempt, that could be controlled by USE_MIDI, not by EXPERIMENTAL_MIDI_OUT.
  1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave tracks.
  2. Scrolling and magnifying the keyboard scale should work by means of mouse scrollwheel, as with wave tracks.
PRL


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot


_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel

Screen Shot 2017-03-31 at 1.07.22 PM.png (58K) Download Attachment
rbd
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

rbd

That seems odd to me.

The reason for calling Pm_Terminate, then Pm_Initialize is that if you plugged in a USB device, you need to re-initialize PortMidi because that's where it discovers the available devices and gives them device numbers.

The odd part is pm_macosxcm_init returning an error. At least I can say I'm using PortMidi on a Mac and I have applications that do this -- re-initialize, notice new devices, and open them -- so I don't know what's going on in this case.

It does make sense that if there are initialization errors, PortMidi would report that it cannot find any devices (device count is 0). It sounds like a bug if that caused an assertion failure. It's possible to have no midi devices, so that should be communicated to the user somehow without causing a failure.


On 3/31/17 1:11 PM, Paul Licameli wrote:
At startup, inside pm_macosxcm_init, numDevices does get 3, but numInputs and numOutputs are 0.

In pm_init, default input and output device ids both get set to -1.

That is at startup.  But these functions are visited again when opening preferences.  Pm_Terminate is called, then Pm_Initialize again.  The second time, pm_macosxcm_init returns an error, but this is not returned to the caller of Pm_initialize.

Pm_CountDevices then returns zero.  MidiIOPrefs::mHostNames remains empty.  This causes assertion errors.

I pushed to master a provisional fix for those assertion errors.

The Host drop-down is too narrow, as seen in the attached.

The drop-downs tell me "No MIDI interfaces" and "No devices found".  This happens even if I remove the calls to Pm_Terminate and Pm_Initialize to avoid the re-initialization error.

So is there something lacking in my System preferences or other environment?

The few MIDI files I have can open and play with no problem in GarageBand.

PRL



On Fri, Mar 31, 2017 at 12:08 PM, Roger Dannenberg <[hidden email]> wrote:



On 3/31/17 11:41 AM, Paul Licameli wrote:


On Fri, Mar 31, 2017 at 10:56 AM, Paul Licameli <[hidden email]> wrote:
I built your changes, and the dialog box I see when I attempt play on my MacBook says

PortMidi: `Invalid device ID'
PortMidi is modeled on PortAudio. Devices have ID's that are assigned when PortMidi is initialized. If you saved an ID in preferences and try to reuse it after devices have been added or unplugged, the ID could become invalid. Also, input and output have different ID's - you can't open an input ID for output. - I hope this helps.


I haven't tried it on my Windows box yet.

What does the Velocity slider do?  I can't hear it.  Is it redundant with the play-at-speed ("transcription") toolbar?

Answering my own question, I see that it relates to the attack of percussive notes, and has nothing to do with tempo.  Ok then.

PRL
 

I do now see the colored buttons and I do see how they show and hide channels.

You say you have not got scrubbing to work.  Have you also tried MIDI playback with the other play commands, such as looping play, cut preview, play-one-second, play-at-speed?

Now that I am displaying MIDI tracks more often, I can think of a few improvements you could attempt, that could be controlled by USE_MIDI, not by EXPERIMENTAL_MIDI_OUT.
  1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave tracks.
  2. Scrolling and magnifying the keyboard scale should work by means of mouse scrollwheel, as with wave tracks.
PRL


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ audacity-devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/audacity-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli


On Fri, Mar 31, 2017 at 3:35 PM, Roger Dannenberg <[hidden email]> wrote:

That seems odd to me.

The reason for calling Pm_Terminate, then Pm_Initialize is that if you plugged in a USB device, you need to re-initialize PortMidi because that's where it discovers the available devices and gives them device numbers.

The odd part is pm_macosxcm_init returning an error. At least I can say I'm using PortMidi on a Mac and I have applications that do this -- re-initialize, notice new devices, and open them -- so I don't know what's going on in this case.

It does make sense that if there are initialization errors, PortMidi would report that it cannot find any devices (device count is 0). It sounds like a bug if that caused an assertion failure. It's possible to have no midi devices, so that should be communicated to the user somehow without causing a failure.


On 3/31/17 1:11 PM, Paul Licameli wrote:
At startup, inside pm_macosxcm_init, numDevices does get 3, but numInputs and numOutputs are 0.

In pm_init, default input and output device ids both get set to -1.

That is at startup.  But these functions are visited again when opening preferences.  Pm_Terminate is called, then Pm_Initialize again.  The second time, pm_macosxcm_init returns an error, but this is not returned to the caller of Pm_initialize.

Pm_CountDevices then returns zero.  MidiIOPrefs::mHostNames remains empty.  This causes assertion errors.

I pushed to master a provisional fix for those assertion errors.

The Host drop-down is too narrow, as seen in the attached.

The drop-downs tell me "No MIDI interfaces" and "No devices found".  This happens even if I remove the calls to Pm_Terminate and Pm_Initialize to avoid the re-initialization error.

So is there something lacking in my System preferences or other environment?

The few MIDI files I have can open and play with no problem in GarageBand.

PRL

The assertion failure was a simply corrected matter of the MIDI preference dialog code wrongly assuming an array was non-empty.

MIDI playback fails for me with Pokechu22's changes, even if I do not first open the preferences dialog.

Do I need to install something in MacOS before PortMidi can discover it?

PRL
 


On Fri, Mar 31, 2017 at 12:08 PM, Roger Dannenberg <[hidden email]> wrote:



On 3/31/17 11:41 AM, Paul Licameli wrote:


On Fri, Mar 31, 2017 at 10:56 AM, Paul Licameli <[hidden email]> wrote:
I built your changes, and the dialog box I see when I attempt play on my MacBook says

PortMidi: `Invalid device ID'
PortMidi is modeled on PortAudio. Devices have ID's that are assigned when PortMidi is initialized. If you saved an ID in preferences and try to reuse it after devices have been added or unplugged, the ID could become invalid. Also, input and output have different ID's - you can't open an input ID for output. - I hope this helps.


I haven't tried it on my Windows box yet.

What does the Velocity slider do?  I can't hear it.  Is it redundant with the play-at-speed ("transcription") toolbar?

Answering my own question, I see that it relates to the attack of percussive notes, and has nothing to do with tempo.  Ok then.

PRL
 

I do now see the colored buttons and I do see how they show and hide channels.

You say you have not got scrubbing to work.  Have you also tried MIDI playback with the other play commands, such as looping play, cut preview, play-one-second, play-at-speed?

Now that I am displaying MIDI tracks more often, I can think of a few improvements you could attempt, that could be controlled by USE_MIDI, not by EXPERIMENTAL_MIDI_OUT.
  1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave tracks.
  2. Scrolling and magnifying the keyboard scale should work by means of mouse scrollwheel, as with wave tracks.
PRL


On Thu, Mar 30, 2017 at 11:29 PM, Pokechu22 <[hidden email]> wrote:
OK, I've rebased off of the latest master.

I chose to not split MixerTrackCluster into subclasses for now, and
instead have both a velocity slider and gain slider and work off of
whether GetNote() and GetWave() are present.  I'm not completely sure
it's the best way, but it wasn't tooo hard to implement this way.

One thing that I noticed: GetRight() always returns the left track -
this means that changing gain doesn't actually affect both tracks.
That should probably be corrected.

I haven't yet gotten to handling the C-Style casts unfortunately.
Most of the other EXPERIMENTAL_MIDI_OUT code uses it, so it'll take a
while to figure out where all of it is (I haven't found a good regex
to match them).

A consideration for the future: what conditionals should the midi code
be put behind?  Right now it's USE_MIDI and EXPERIMENTAL_MIDI_OUT, but
in the future, that should change. USE_MIDI currently means "do we
have allegro/portSMF", and doesn't necessarily mean PortMIDI is
present, so just depending on that won't necessarily satisfy the
conditions (unless the requirements for USE_MIDI are changed).  It's
something to think about.

--Poke

On Wed, Mar 29, 2017 at 11:12 AM, Paul Licameli <[hidden email]> wrote:
> Poke, please see my latest pushes to master at 8c796e2
>
> You made me accelerate some other saved up work I had wanted to do, making a
> common base class of WaveTrack and NoteTrack but not of the other kinds of
> track.  Mute and Solo state should be in this class, not in Track itself.
>
> This may cause you a bit of grief reworking the MixerBoard, sorry about
> that.
>
> You will observe far fewer #ifdefs, even fewer than in your version.
>
> I don't think I would make two subclasses of MixerTrackCluster if I were
> doing it, rather I would switch where needed on track type, but I will leave
> those details to you.
>
> One small note of style, is that I noticed that you used C-style pointer
> casts in places.  I prefer not to do that.  I would prefer to see
> static_cast, because that discipline never allows implicit casting away of
> const.
>
> Your list of still outstanding commits would shrink by one when you rebase,
> because I cherry-picked "Rename gain to velocity on note tracks", except
> partial reversion of it was needed to compile it.  Some things involving
> mGainPlacementRect belonged in a different commit, I think.
>
> PRL
>
>
> On Tue, Mar 28, 2017 at 1:40 AM, Paul Licameli <[hidden email]>
> wrote:
>>
>> I think for my next move I want to cherry pick "Rename gain to velocity in
>> note tracks" and make that part of a branch I want to push, which will
>> introduce new common base classes of WaveTrack and NoteTrack.  This branch
>> will also greatly reduce the number of #ifdef tests of USE_MIDI and
>> EXPERIMENTAL_MIDI_OUT.  I will want to push this branch, and then you may
>> have some nontrivial rebasing work to do onto it.
>>
>> About not playing MIDI, I observe in the debugger on my mac that there was
>> an attempt to open a midi stream in AudioIO.cpp, but it failed and returned
>> an error code.  Maybe I am missing something in expected third-party dynamic
>> libraries or environment variables?  Still it's true at least for me that
>> this is not working out of the box to make midi play work.  So it's not yet
>> a feature.
>>
>> I am trying to determine what parts of your work are harmless and could be
>> separated from it and merged without #ifdef guards.  But it seems there are
>> changes to the appearance of note tracks that are unconditionally compiled.
>> If this project does not advance past experimental for this release, then
>> those changes in drawing should not be done.  They should be disabled by
>> some EXPERIMENTAL switch if they are merged at all.
>>
>> But the simplification of ASlider by removing SetStyle, which depends on
>> the new members of TrackInfo -- that I might merge in, and take it on myself
>> to reconcile my track-panel-refactor with that.
>>
>> PRL
>>
>>
>>
>> On Tue, Mar 28, 2017 at 1:08 AM, Pokechu22
>> <[hidden email]> wrote:
>>>
>>> EXPERIMENTAL_MIDI_CONTROLS controls the buttons to toggle individual
>>> midi channels.  When it's defined/not defined, it behaves slightly
>>> differently from before it was introduced, since in current master the
>>> channel toggles are half-enabled (you can blindly click to toggle
>>> them).  The intention is that it is stable enough to have enabled by
>>> default (even if the rest of EXPERIMENTAL_MIDI_OUT isn't).  All that
>>> commit does is change where it's defined so that it's controlled via
>>> EXPERIMENTAL_MIDI_CONTROLS instead of EXPERIMENTAL_MIDI_OUT; it
>>> doesn't add anything new.
>>>
>>> "Switch to a dedicated velocity slider" could probably be split up a
>>> bit, since that commit actually does more than it says (including
>>> _fixing_ the midi controls -- but not putting them behind their own
>>> define).  A lot of those changes are interconnected though (getting
>>> rid of the offset rect and such).  Should I try to split out the midi
>>> controls fixes and bring both them and the define to the start?
>>>
>>> I'll change those ifdefs tomorrow.  Should I do the same with method
>>> headers (and brackets on them), if there are any like that, or do
>>> those not cause issues with your editor?  (I vaguely remember some
>>> being that way though I'm not sure if I changed it).
>>>
>>> --Poke
>>>
>>> On Mon, Mar 27, 2017 at 9:37 PM, Paul Licameli <[hidden email]>
>>> wrote:
>>> > Is EXPERIMENTAL_MIDI_CONTROLS a conditional compilation switch, which,
>>> > when
>>> > undefined, will cause no changes at all in appearance and behavior?
>>> > Then
>>> > please put this conditional compilation in from the beginning of the
>>> > list of
>>> > commits.  I am trying to understand "switch to a dedicated velocity
>>> > slider..." which does not obect this.  I don't know if the later commit
>>> > completely corrects it.
>>> >
>>> > I see this idiom which I greatly dislike because it confuses the
>>> > auto-matching of parentheses in my editor:
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >    if (((mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #else
>>> >
>>> >    if ((
>>> >
>>> > #endif
>>> >
>>> >
>>> > Do no replicate parentheses with #ifdefs.  Do this instead.
>>> >
>>> >    if ((
>>> >
>>> > #ifdef EXPERIMENTAL_MIDI_CONTROLS
>>> >
>>> >         (mNextEventTrack->IsVisibleChan(channel)) &&
>>> >
>>> > #endif
>>> >
>>> >
>>> > Thus everything just inside the conditional compilation leg has
>>> > balanced
>>> > parentheses.
>>> >
>>> > PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ audacity-devel mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/audacity-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Pokechu22-2
In reply to this post by Paul Licameli
On Fri, Mar 31, 2017 at 7:56 AM, Paul Licameli <[hidden email]> wrote:
> I built your changes, and the dialog box I see when I attempt play on my
> MacBook says
>
> PortMidi: `Invalid device ID'
>
> I haven't tried it on my Windows box yet.

I don't have any advice for this; I don't use a mac and don't know
enough about portmidi to get it to work.  In some cases you definitely
do need another midi synth, such as timidity++, but it sounds like
this is a more complex issue.

> What does the Velocity slider do?  I can't hear it.  Is it redundant with
> the play-at-speed ("transcription") toolbar?

You've already figured this out, but it's basically the
loudness/forcefulness of notes (see
https://en.wikipedia.org/wiki/File:Dynamic%27s_Note_Velocity.svg).

> I do now see the colored buttons and I do see how they show and hide
> channels.
>
> You say you have not got scrubbing to work.  Have you also tried MIDI
> playback with the other play commands, such as looping play, cut preview,
> play-one-second, play-at-speed?

I tested the behavior of these:
* Play one second works fine.
* Play at speed and time tracks both mostly in terms of playback, but
when there's only a note track and no wave tracks, the playback
indicator is in the wrong place as I mentioned earlier.  This is
probably my bug, since I attempted to add time track support to midi
output (previously, it had a single time multiplier instead).
* Looping play loops, but skips the first few notes (perhaps, it
doesn't play notes in the first second / in the midi latency time).
Looping play at speed also loops, but skips the _last_ few notes
(perhaps the same way).
* Scrubbing, as I previously mentioned, plays 1 or 2 notes near the
point you first use it, and then refuses to play any other notes (but
it will happily replay the notes you played the first time again)
* I don't know where cut preview is, so I've been unable to test it.

The timing code is somewhat complicated and I haven't entirely figured it out.

> Now that I am displaying MIDI tracks more often, I can think of a few
> improvements you could attempt, that could be controlled by USE_MIDI, not by
> EXPERIMENTAL_MIDI_OUT.
>
> 1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave
> tracks.
> 2. Scrolling and magnifying the keyboard scale should work by means of mouse
> scrollwheel, as with wave tracks.
>
> PRL

These are definitely good ideas; I'll look into it.  I'm going to be
going on a trip in a day or two and probably won't be able to work on
it until I get back, though.

--Poke

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: I pulled three of Pokechu22's 12 commits...

Paul Licameli


On Fri, Mar 31, 2017 at 8:01 PM, Pokechu22 <[hidden email]> wrote:
On Fri, Mar 31, 2017 at 7:56 AM, Paul Licameli <[hidden email]> wrote:
> I built your changes, and the dialog box I see when I attempt play on my
> MacBook says
>
> PortMidi: `Invalid device ID'
>
> I haven't tried it on my Windows box yet.

I don't have any advice for this; I don't use a mac and don't know
enough about portmidi to get it to work.  In some cases you definitely
do need another midi synth, such as timidity++, but it sounds like
this is a more complex issue.

> What does the Velocity slider do?  I can't hear it.  Is it redundant with
> the play-at-speed ("transcription") toolbar?

You've already figured this out, but it's basically the
loudness/forcefulness of notes (see
https://en.wikipedia.org/wiki/File:Dynamic%27s_Note_Velocity.svg).

> I do now see the colored buttons and I do see how they show and hide
> channels.
>
> You say you have not got scrubbing to work.  Have you also tried MIDI
> playback with the other play commands, such as looping play, cut preview,
> play-one-second, play-at-speed?

I tested the behavior of these:
* Play one second works fine.
* Play at speed and time tracks both mostly in terms of playback, but
when there's only a note track and no wave tracks, the playback
indicator is in the wrong place as I mentioned earlier.  This is
probably my bug, since I attempted to add time track support to midi
output (previously, it had a single time multiplier instead).
* Looping play loops, but skips the first few notes (perhaps, it
doesn't play notes in the first second / in the midi latency time).
Looping play at speed also loops, but skips the _last_ few notes
(perhaps the same way).
* Scrubbing, as I previously mentioned, plays 1 or 2 notes near the
point you first use it, and then refuses to play any other notes (but
it will happily replay the notes you played the first time again)
* I don't know where cut preview is, so I've been unable to test it.

By default it is bound to the C key.  It plays some seconds before the selection, skips the selection, and plays some seconds after.  Playback preferences affect how much.

PRL
 

The timing code is somewhat complicated and I haven't entirely figured it out.

> Now that I am displaying MIDI tracks more often, I can think of a few
> improvements you could attempt, that could be controlled by USE_MIDI, not by
> EXPERIMENTAL_MIDI_OUT.
>
> 1. The Ctrl+Shift+F command should re-fit all audio tracks, not only wave
> tracks.
> 2. Scrolling and magnifying the keyboard scale should work by means of mouse
> scrollwheel, as with wave tracks.
>
> PRL

These are definitely good ideas; I'll look into it.  I'm going to be
going on a trip in a day or two and probably won't be able to work on
it until I get back, though.

--Poke

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
12
Loading...