Proposed fix 1527 Channel note on indicator doesn't fire when a very high note is played.

[curlymorphic]

Proposed fix 1527 Channel note on indicator doesn't fire when a very high note is played.

Now the Instrument activity indicator flashed when a note is played.

[diizy]

I'm a bit hesitant about this fix...

The check for the key range is there for a reason, by just bypassing it, we'll be emiting the newNote signal even if the note is invalid, and even if the instrument doesn't get passed a midi event.

I'm not sure if the indicator even should blink in this situation...

[curlymorphic]

I also had them concerns, but as master currenly stands the note is already playing before this emit newNote,and notes play that are outside of this range check.

TBH maybe the correct fix for this is to stop the playing of notes outside the range check?

[diizy]

On 12/30/2014 02:52 PM, Dave wrote:

I also had them concerns, but as master currenly stands the note is
already playing before this emit newNote,and notes play that are
outside of this range check.

On some instruments. Try it on eg. Vestige or Opulenz, and you'll see
that those notes won't play. Outgoing MIDI also won't play.

TBH maybe the correct fix for this is to stop the playing of notes
outside the range check?

Well that's not really a good solution either - we shouldn't limit our
instruments to MIDI standards just because some instruments constrain
themselves to MIDI.

Maybe we should first find out all the things the newNote signal is
connected to, then we can maybe figure out when exactly it should be
emitted, or if it should maybe be separated to multiple signals...

[curlymorphic]

The only place I can find that uses the newNote signal is InstrumentTrack.cpp ln 913

connect( _it, SIGNAL( newNote() ),
            m_activityIndicator, SLOT( activate() ) );

The only purpose of the newNote signal is for the indicator

[tresf]

The check for the key range is there for a reason, by just bypassing it, we'll be emiting the newNote signal even if the note is invalid, and even if the instrument doesn't get passed a midi event.

If you don't mind me asking, in what scenarios are the invalid check for? In several instruments, these notes still generate sound output, just no visual indicator on the FadeButton, so what type of scenario are we protecting from?

-Tres

[tresf]

The only purpose of the newNote signal is for the indicator

Perhaps we give it a more appropriate name then?

[curlymorphic]

Perhaps we give it a more appropriate name then?

I agree the name newNote is misleading

activateIndicator would be my suggestion

[diizy]

On 12/30/2014 03:24 PM, Dave wrote:

Perhaps we give it a more appropriate name then?

I agree the name newNote is misleading

activateIndicator would be my suggestion

If it's only used by one recipient, then we shouldn't be using a signal
at all, but instead call the function directly. Signals/Slots always
cause extra overhead compared to direct function calls.

[curlymorphic]

I can do that, the queston is where, inside or outside the range check?

[diizy]

On 12/30/2014 03:31 PM, Dave wrote:

I can do that, the queston is where, inside or outside the range check?

Hm, for now it's probably easiest to put it outside the range check,
this can then be handled more elegantly in the future with the API
changes...

[tresf]

If it's only used by one recipient, then we shouldn't be using a signal at all, but instead call the function directly. Signals/Slots always cause extra overhead compared to direct function calls.

Again, lack of knowledge question... but do emit calls fire non-blocking actions (similar to a new thread)? Probably over-analyzing here, but since my knowledge of slots is very limited, I wanted to make sure that calling the function directly wouldn't cause any unintended issues. 🐮

[diizy]

On 12/30/2014 04:08 PM, Tres Finocchiaro wrote:

If it's only used by one recipient, then we shouldn't be using a
signal at all, but instead call the function directly.
Signals/Slots always cause extra overhead compared to direct
function calls.

Again, lack of knowledge question... but do |emit| calls fire
non-blocking actions (similar to a new thread)?

No. emit calls fire Qt's metaobject system (it's what we need mocfiles
for). It's basically just abstraction that makes it easier to call
arbitrary functions from an arbitrary object. This abstraction of course
comes with overhead, because it has to first figure out what object
we're emitting from, then figure out what slots are connected to the
signal we're emitting, then figure out the function pointers to those
slots and call each of them individually.

The benefit from signals/slots is when we have a situation where we may
not always know all the functions we want to call, or want to modify
these connections in runtime.

If a signal is only ever connected to one slot, then it's a lot of
overhead for no gain - we can get rid of all that overhead by just
calling the function directly.

[curlymorphic]

Removed the Signal newNote

[diizy]

Looks good

[curlymorphic]

If a signal is only ever connected to one slot, then it's a lot of
overhead for no gain - we can get rid of all that overhead by just
calling the function directly.

If performance is the only factor we are measuring. The use of signals and slots removes dependencies for ui and mechanics, with signals and slots, if the ui breaks or changes the engine carries on.

This lack of dependency really shines because it is possible to change the ui with no changes needed to core code. From what i can see LMMS is written using the MVC pattern, putting these dependencies in is eroding that.

This is more of a general observation from me than a comment on performance, that i know is important in audio

[diizy]

This commit may be causing a crash in preset preview now...