AFP Selection Bug

[tresf]

Since the AFP loop code has been added to the AudioFileProcessor instrument, the wave selection behavior has become non-ideal.

The issue is that by default, the start-point of the wave is no longer adjustable with the knobs. Instead, the loop-points must be adjusted first to allow the start-point to be adjusted.

Ideally, one of two scenarios would happen:

• The start-point would be adjustable and would nudge the loop-point marker automatically.
  -- OR --
• The AFP would default to having the loop-point start and loop-point end values disabled by default. When disabled, the loop-point markers would have no bearing on the start-point and end-point of the wav.

Possibly related to #927.

Code:

https://github.com/LMMS/lmms/blob/stable-1.1/plugins/audio_file_processor/audio_file_processor.cpp#L373

[diizy]

On 09/07/2014 06:42 PM, Tres Finocchiaro wrote:

Since the AFP loop code has been added to the AudioFileProcessor
instrument, the wave selection behavior has become non-ideal.

The issue is that by default, the start-point of the wave is no longer
adjustable with the knobs. Instead, the loop-points must be adjusted
first to allow the start-point to be adjusted.

Actually, this depends on the direction. I can't remember which way it
was, but when you move the loop point to one direction, it pushes the
startpoint with it, and when you move the loop point to another
direction, it moves freely.

The problem is that we have to make sure that the loop point doesn't get
outside the area between start/end points, so we're doing a check that
checks if the loop point is outside the area and if so, it moves the
start/end points to accommodate.

The best fix would be to save the "last moved point" in some variable,
then use that to consider that point the "priority" and move the other
points accordingly at conflict situations. This would be the most
intuitive for the user, as the point currently being moved would always
move freely.

[DeRobyJ]

I may not have too much knowledge of C++, but what I see here is that

WHEN loopPoint is over Start/End point -> change Start/End point
So loopPoint is not moved when not touched.
This is no fix in that using the same function for the 3 knobs.

connect( &m_startPointModel, SIGNAL( dataChanged() ),
this, SLOT( loopPointChanged() ) );
connect( &m_endPointModel, SIGNAL( dataChanged() ),
this, SLOT( loopPointChanged() ) );
connect( &m_loopPointModel, SIGNAL( dataChanged() ),
this, SLOT( loopPointChanged() ) );

Instead, we should have three different functions (loopPointChanged, startPointChanged, endPointChanged)
In each of these, we should write that if they are overlapping, the others will move!

Right now loopPointChanged (which applies for the three of them indifferetly) is able to move start and end points, while it can't move loop point.

startPointChanged and endPointChanged should be able to tackle loopPoint.

This of course leads to the fact that when you move the start point, you may move the loop point, and this will turn loopPointChanged on: two fuctions dealing with the same case and with opposite instructions. Then which function has priority? We should have a "priority" variable that can be 1, 2 and 3.

The knob moved by the user is 3, and if it is startPoint:
loopPoint is 2
endPoint is 1
(because you may touch the endPoint by pushing the loopPoint, and in this case you want loopPointChanged to give intructions)

If it is endPoint
loopPoint is 2
startPoint is 1

If it is loopPoint the problem is less complicated, as we'll have max two functions active at the same time.
start and end are 2.

[tresf]

So I examined the code and it seems it was written to nudge the start and end points with the loop markers, which is backwards. It makes the start and end points greater than loop boundries ignore value changes unless initiated by a loop marker. As you've stated, this is partly because the function has no memory of what the value was before, so detecting direction is much more difficult.

But on top of this, when the loop markers are disabled, the loop markers should have no effect on the sample's start and end, period. A disabled feature shouldn't have any bearing on an enabled feature. For this reason I feel this feature is incomplete and should be pushed to master.

I'll still try to dive in and fix it, but it's behavior is broken in ways that I don't feel justify inclusion into a stable build.

Recommendations:

1. Global Flag for that AFP instance to disable enhanced loop functionality when disabled.
2. Make "power button" have inverted functionality (turn on enhanced loop functionality rather than turn off).
3. Make loop markers and start-end markers behave identically to each-other, nudging when necessary but never blocking each-other with the exception of hard-limits on start/end points.

[diizy]

On 09/07/2014 07:46 PM, DeRobyJ wrote:

I may not have too much knowledge of C++, but what I see here is that

WHEN loopPoint is over Start/End point -> change Start/End point
So loopPoint is not moved when not touched.
This is no fix in that using the same function for the 3 knobs.

connect( &m_startPointModel, SIGNAL( dataChanged() ),
this, SLOT( loopPointChanged() ) );
connect( &m_endPointModel, SIGNAL( dataChanged() ),
this, SLOT( loopPointChanged() ) );
connect( &m_loopPointModel, SIGNAL( dataChanged() ),
this, SLOT( loopPointChanged() ) );

Instead, we should have three different functions (loopPointChanged,
startPointChanged, endPointChanged)
In each of these, we should write that if they are overlapping, the
others will move!

Right now loopPointChanged (which applies for the three of them
indifferetly) is able to move start and end points, while it can't
move loop point.

startPointChanged and endPointChanged should be able to tackle loopPoint.

This of course leads to the fact that when you move the start point,
you may move the loop point, and this will turn loopPointChanged on:
two fuctions dealing with the same case and with opposite
instructions. Then which function has priority? We should have a
"priority" variable that can be 1, 2 and 3.

The knob moved by the user is 3, and if it is startPoint:
loopPoint is 2
endPoint is 1
(because you may touch the endPoint by pushing the loopPoint, and in
this case you want loopPointChanged to give intructions)

If it is endPoint
loopPoint is 2
startPoint is 1

If it is loopPoint the problem is less complicated, as we'll have max
two functions active at the same time.
start and end are 2.

You're making this way more complicated than it needs to be...

[diizy]

On 09/07/2014 07:54 PM, Tres Finocchiaro wrote:

So I examined the code and it seems it was written to nudge the start
and end points with the loop markers, which is backwards. It makes the
start and end points ignore value changes unless initiated by a loop
marker. As you've stated, this is partly because the function has no
memory of what the value was before, so detecting direction is much
more difficult.

But on top of this, when the loop markers are disabled, the loop
markers should have no affect on the sample's start and end, period. A
disabled feature shouldn't have any bearing on an enabled feature. For
this reason I feel this feature is incomplete and should be pushed to
master.

I think you're exaggerating the severity of the issue by quite a bit...
sure, it's a flaw in functionality, but it's not something that prevents
the user from doing anything, it doesn't affect the stability of the
program - none of the functionality is affected... if anything, it makes
usage slightly more inconvenient, but doesn't actually prevent or
disable any functionality.

No one says stable needs to be perfect... it just needs to be stable.

Recommendations:

1. Global to disable enhanced loop functionality when disabled.

I don't think we should start making global settings for every possible
feature. This quickly leads to a situation where the user has to spend
30 minutes with a new build to get LMMS to behave the way they want...

I don't think a global setting is even justified in this case. Not to
mention the compatibility problems it would lead to... what to do when
you open a project that uses loop points, but you've disabled loop
points in settings? the project will not play correctly.

1. Make "power button" have inverted functionality (turn on enhanced
   loop functionality rather than turn off).

Problem here is, we have 3 modes: Loop off, Loop on (forwards), Loop on
(ping-pong). We can't have one button enable the looping functionality,
because there's two modes where looping is enabled... and only one mode
where looping is disabled.

That aside, you're the one who came up with the symbols... If you want
to design a new symbol for the "loop off" mode, be my guest.

1. Make loop markers and start-end markers behave identically to
   each-other, nudging when necessary but never blocking each-other
   with the exception of hard-limits on start/end points.

This is what I suggested doing.

What we need is, simply, some mechanism to figure out which loop point
has been moved last. Because the "last moved" point is also always the
"currently being moved" point. There's several ways to do this... one
would be - like DeRobyJ suggested - to create discrete slots for each
knob, then have these simply set the variable and then call the generic
"loopPointChanged" function. Another would be to keep track of the last
positions of each model, and determine which one was changed this way.

In any case, when we know the "last moved knob", we can the prioritize
this loopPoint in the loopPointChanged function, and have the other
points always move away from the prioritized loopPoint.

[tresf]

@diizy, I don't think we're in disagreement with much.

First, my definition for global is probably misused in this context, so I'll explain.

Global to disable enhanced loop functionality when disabled.

I mean when the enhanced loop is turned off, turn off the knob, turn off the markers. flag for that AFP instance would have probably been a better term. I did not mean to introduce conversation about global settings.

No one says stable needs to be perfect... it just needs to be stable.

Well, this depends on who we trust with quality control. So far, we have a pretty decent team of testers, and I value their opinion quite a bit. If Stian and myself say this needs fixing, and we honor that opinion, we have two options, 1. Revert 2. Fix what we have. Since option 2 is still up in the air, it is not an exaggeration to say we should consider reverting.

We always have the option to ignore the opinions of our testers, but that brings us down a slippery slope.

That aside, you're the one who came up with the symbols...

I've intentionally made it a point not to point any fingers in this bug report. I hope you can be respectful and please do the same.

-Tres

[Sti2nd]

Hey, Diizy, LMMS 1.1.0 comes out when it is ready, remember? I felt it was ready months ago, you didn't, now I feel it isn't ready, you do. Point is we waited until now, we can probably wait a week and see what can be done with this bug.

[diizy]

On 09/07/2014 09:09 PM, Tres Finocchiaro wrote:

I don't think we're in disagreement with much.

First, my definition for |global| is probably misused in this context,
so I'll explain.

Global to disable enhanced loop functionality when disabled.

I mean when the enhanced loop is turned off, turn off the knob, turn
off the markers. |flag for that AFP instance| would have probably been
a better term. I did not mean to introduce conversation about |global
settings|.

Ok. I think that would still make things too complicated. There's the
issue again that implementing the logic for this would quite possibly be
more complex than just fixing the behaviour of the current widgets.

No one says stable needs to be perfect... it just needs to be stable.

Well, this depends on who we trust with quality control. So far, we
have a pretty decent team of testers, and I value their opinion quite
a bit. If Stian and myself says this needs fixing, and we honor that
opinion, we have two options, 1. Revert 2. Fix what we have. Since
option 2 is still up in the air, it is not an exaggeration to say we
should consider reverting.

Reverting would quite possibly also turn out to be more complicated than
fixing. What I'm saying is, good luck isolating all the code that
implements the loopback point, and disabling it in a way that does not
interfere with all the other changes and fixes made on the AFP since
then... You could of course just roll back all the AFP changes to a
state before this feature was implemented, but then you risk also
bringing in all the bugs fixed in AFP in the meanwhile, and also
possibly introducing whole new bugs, caused by incompatibilities of the
AFP back then and the current 1.1 codebase.

We always have the option to ignore the opinions of our testers, but
that brings us down a slippery slope.

No one's opinion is getting ignored here. We still have to consider what
is most feasible and most optimal usage of our current resources.

That aside, you're the one who came up with the symbols...

I've intentionally made it a point not to point any fingers in this
bug report. I hope you can be respectful and please do the same.

No fingers are being pointed.

You're still in the best position to change the icons, since you made
the current ones and would be best suited to do any new ones in the same
look and style.

[tresf]

Ok. I think that would still make things too complicated. There's the
issue again that implementing the logic for this would quite possibly be
more complex than just fixing the behaviour of the current widgets.

I hadn't realized until digging in now that this functionality is in addition to what we already had -- It's always on sorry for any confusion... oversight on my part.

You're still in the best position to change the icons, since you made
the current ones and would be best suited to do any new ones in the same
look and style.

Swapping on/off pixmaps I think would be the quickest fix.

Reverting would quite possibly also turn out to be more complicated than
fixing. What I'm saying is, good luck isolating all the code that
implements the loopback point, and disabling it in a way that does not
interfere with all the other changes and fixes made on the AFP since
then...

Understood, lets fix it then. I have some time now to stare at it... Will post any progress here.

-Tres

[diizy]

On 09/07/2014 09:36 PM, Tres Finocchiaro wrote:

Swapping on/off pixmaps I think would be the quickest fix.

Again, the thing is, there's 3 modes, not just on/off. Which ones are
you're suggesting switching? Off and Forwards? I'm not sure if that'd
make more sense conseptually...

Understood, lets fix it then. I have some time now to stare at it...
Will post any progress here.

Cool.

[tresf]

Again, the thing is, there's 3 modes, not just on/off. Which ones are
you're suggesting switching? Off and Forwards? I'm not sure if that'd
make more sense conceptually...

Well, there's three modes:

on - forward
on - ping-pong 
off

So the power button being lit when it's off I think is backward. It should be lit when it's on. Switching the power pixmaps achieves this, which was my original intention when they were made.

[diizy]

On 09/07/2014 09:52 PM, Tres Finocchiaro wrote:

Again, the thing is, there's 3 modes, not just on/off. Which ones are
you're suggesting switching? Off and Forwards? I'm not sure if that'd
make more sense conceptually...

Well, there's three modes:

|on - forward
on - ping-pong
off
|

So the power button being lit when it's off I think is backward. It
should be lit when it's on. Switching the power pixmaps achieves this,
which was my original intention when they were made.

Oh. I see your point, but also I think that might possibly be kind of
confusing? People are used to a certain way of multi-selection buttons
working, which is that one button in a group is lit at any one time...
just saying, deviating from this practice might be confusing some.

[tresf]

Please review the proposed changes. #1136

I think that might possibly be kind of confusing? People are used to a certain way of multi-selection buttons working

I think when you try it out it will make sense. It is placed over the top of the two buttons and I think it will be intuitive. If this pull request gets your blessing, I'll probably build an interim win build for @Sti2nd to make sure at least he can test drive the change prior to the 1.1 release.

-Tres

[musikBear]

one small comment - isent it just in one specific situation, that the loop-point need to be more 'enhanced' to the user. : When the user loads a new sample.
After this 'load'-event the user are in manual control of all three dials, and hence all three posistion-settings Imo, the user should at that point in time, have full insight in the existence of three points (start, loop, end), and that they cant overlab. Its only at the load-event the loop-point need to be moved, and the only reason is that the user need to spot the blue loop-bar. Right? -So in whitch function does a sample load? That is where the mid-pos-parking should be implemented.

(Besides that, speak nicely 💋

[tresf]

Sure, we can, but who's to say it should be in the middle? To do this proper, we'd support loop markers on the samples directly like most DAWs do and set it according to where the sample would want it.

I get your point, they can't "see" the left edge, but now I think we're nitpicking. 🐝

-Tres

[tresf]

This still needs some tweaking before it's closed.

Per @DeRobyJ

Moving startPoint is always ok, but some some reason if you move endPoint and you push both loopPoint and startPoint with it, LMMS will eventually crash D:
This seems to happen:

• Randomly while playing the instrument in loopModeEnabled
• Always while playing in loopModeEnabled and touching 0.001
  
  
  This don't seem to happen
• When not playing anything / not having enabled looping

[tresf]

I submitted another fix. Can you please retest here:
https://github.com/tresf/lmms/releases/tag/v1.0.95

[DeRobyJ]

Fixed
Tried with all the other buttons (reverse, stutter, both loop modes) and playing melodies and chords.

This all work fine now.

[tresf]

Thanks! 🍭