Looping: update loopsize spinbox when recalling loop with any size #12509

ronso0 · 2024-01-04T12:29:21Z

The loop size is now correctly detected and updated, both when setting a loop with a size not in the preset list (0.03125, 0.0625, 0.125, 0.25, 0.5, 1, 2, 4, 8 ... 512) by setting the size with the beatloop size spinbox and when re-activating such a saved loop.
Done with new Beats::numFractionalBeatsInRange(start, end).

Note:
Unfortunately this does not work for loops set with IN/OUT buttons (intentionally, see my comment below).
edit: addressed in #12522

daschuer

Thank you. I have left suggestions.

daschuer · 2024-01-04T13:44:03Z

src/engine/controls/loopingcontrol.cpp

-    return -1;
+
+    // No hit. Calculate the fractional beat length
+    return pBeats->numFractionalBeatsInRange(startPosition, endPosition);


I wonder why numBeatsInRange() does not returns factions in the first place.
It could be also required here:

mixxx/src/engine/controls/loopingcontrol.cpp

Line 768 in fe0c530

m_pCOBeatLoopSize->setAndConfirm(pBeats->numBeatsInRange(

This branch is for when quantize is enabled, we don't need fractional lengths then.

daschuer · 2024-01-04T13:53:43Z

src/track/beats.cpp

+
+    // maybe shift range to start at first beat
+    audio::FrameDiff_t firstBeatOffset = firstBeatPos - startPos;
+    endPos += firstBeatOffset;


This only works if the beats have an equal length. For details around a position you may use:
BpmControl::getBeatContext(). This way you get the beat fraction from before and after the fist and last beat.
We may move this function to this class?

That function is helpful, thanks!
Yes, that (and BpmControl::getBeatContextNoLookup) should be in the Beats class.

Anyhow, the current implementation works well with constant BPM.
For tracks with variable BPM, the fractional size is just an estimation. Though IMO that doesn't matter since with var. BPM tracks loop_halve/_double aren't working as expected anyway AFAICT: the loop is simply scaled based on loop_in position and number of beats, i.e. it doesn't consider shorter/longer beats inside or after the loop.

Ok I understand.

What could be a real live use case?
A beat loop with an integer number moved by a half beat and a tempo change inside a loop.

Which beat length is used for moving? Probably the first beat? After moving, the loop will start at the middle of the fist beat and end somewhere else in the last beat.
I think the loop size should be still without a fraction, right?

What happens if the loop is moved fully in the new tempo region?

We need to test this.

Loops that contain tempo changes are moved correctly: both loop_in and loop_out are moved to the correct fractional position. This works with all loop sizes. 👍
I tested with 4 and 3.5 beats loop moved by 1/2 and 8 beats several times while the tempo changed from ~110 to ~140.

ronso0 · 2024-01-04T13:58:24Z

Thanks for you review!

This is appearantly not ready since
a) there is some issue with loop_remove when a track is loaded to multiple players
b) I ran into a debug assert in CueControl after seeking and reactivating a saved loop

ronso0 · 2024-01-04T20:50:54Z

b) I ran into a debug assert in CueControl after seeking and reactivating a saved loop

This happens when re-activating a saved loop for example, and it's a round-trip that becomes visible with this fix.
It seems avoiding the round-trip offset is close to impossible without a major refactoring, but it can be dealt with by using LoopingControl::positionNear(a, b) in the DEBUG_ASSERT (like in other places) instead of exact ==.
Puuh, I already feared I was poking into a rats nest with this small fix.

The loop_remove issue however is still unclear to me, I don't understand why my changes cause that.

ronso0 · 2024-01-05T18:55:58Z

Alright, I moved some helpers to more suitable classes and picked 1dafd34.

The actual loop size fix is on top of it and it's now much simpler and more accurate using Beats::getContext(). Thanks @daschuer for the hint!

"Good" news: the loop_remove issue exists also in 2.4 so that's not blocking this PR.
I'll look into it (I fear it never worked as intended in the first place..)

Though
this fix reveals an inconsistency:

set a loop manually with the IN/OUT buttons
- loop size spinbox is not updated
- halve/double don't work
- save that loop to a hotcue
  = halve/double work

and

set a loop manually with the IN/OUT buttons
- halve/double don't work
- eject track, reload
- loop is restored
  = halve/double work

I'm not sure why it was decided to not update the spinbox with the manual loop's length and thereby not make halve/double work, but there are even tests for that
LoopDoubleButton_DoesNotResizeManualLoop
LoopHalveButton_DoesNotResizeManualLoop

After skimming #1187 I assume this behaviour should preserve (standart pre-set) loop sizes so beatloop_activate can be used for new loops right away, without having to reset the loop size to some default 2^n size? Same when loading a new track.
One way around this (for new tracks) would be to reset the loop size to the default 4 beats if the track has no loop set. However, this seems complicated considering how loops and cues are read when loading a track.

From the top of my head I have two real-life use case where it would have been helpful:
1) loop constant BPM sections of tracks where you can't use integer/preset loops because beats are wrong, e.g. for long mixes with variable BPM but constant section.
2) Mix a part of a track with no/useless beats (vocals, noise ...) with track2 by pressing IN and OUT in sync with the beats of track2. Admittedly, this will be off beat soon but it's good enough for short transitions.

Forgive me opening this can of worms so soon before the release ; )

daschuer

Thank you for the update.
I need time for testing.

daschuer · 2024-01-10T08:12:43Z

src/audio/frame.h

+        VERIFY_OR_DEBUG_ASSERT(isValid()) {
+            return false;
+        }
+        return target.isValid() &&


Can this isValid() also an debug assert?

IMO the caller is responsible for testing the position. It may be possible this is called with an invalid FramePos and in that case we simply return false.

ronso0

reminder

ronso0 · 2024-01-10T13:41:57Z

src/audio/frame.h

@@ -91,6 +91,16 @@ class FramePos final {
        return util_isfinite(m_framePosition);
    }

+    // returns true if a is valid and is fairly close to target (within +/- 1 frame).


Suggested change

// returns true if a is valid and is fairly close to target (within +/- 1 frame).

// returns true if this and target are valid and target is fairly close (within +/- 1 frame).

daschuer · 2024-01-11T07:06:18Z

I have tested with a changing tempo track and have these issues:

the spin box jumps to rather random value like 4.01 or 5.07 after loading track, even there is no loop set. (Reloop does nothing)
When I manual edit the value, and than press the beat loop button the old value is applied, because the beatloop still has the input focus. The Enter key works.
The same issue is applies also to the beat jump box.
When I change the loop length using loop-out the spin-box is not updated. (intended?)
This manual edited loop changes length but not beat size when beat moved, OK.
Reloop cycle does not change the loop size. (intended?)
New loops with Loop-In and Out do not change the loop size box (intended?)

ronso0 · 2024-01-11T14:06:36Z

the spin box jumps to rather random value like 4.01 or 5.07 after loading track, even there is no loop set. (Reloop does nothing)

This may be due to invalid loop cues. Can you verify it occurs with new tracks? (new in the db, for example a track copy).
edit: I can not confirm with unplayed tracks / tracks that never had a loop set.

ronso0 · 2024-01-11T14:15:29Z

When I manual edit the value, and than press the beat loop button the old value is applied, because the beatloop still has the input focus. The Enter key works.
The same issue is applies also to the beat jump box.

It is required to press Return to confirm the new value.
I stumble over this repeatedly and find this not intuitive and quite annoying tbh. I took a look at getting the unconfirmed value from the deck's (loop) WBeatSpinbox via signals/slots, and while it seems possible it is quite complicated (or at least not time-safe since it's not syncronous).

ronso0 · 2024-01-11T14:16:18Z

When I change the loop length using loop-out the spin-box is not updated. (intended?)

New loops with Loop-In and Out do not change the loop size box

yes, see #12522.

ronso0 · 2024-01-11T14:17:26Z

Reloop cycle does not change the loop size. (intended?)

Not sure what you mean.

ronso0 · 2024-02-05T12:38:24Z

@daschuer, quoting you from #10511

I am afraid this too complex to slip in to 2.4.0 We can push a fix whenever this is ready.

Sure.
I'm not for rushing this either at this point, but I disagree it's complex. Essentially it's only 1a7af25, and that's a low risk commit compared to the currently broken beatsize behaviour.
That plus the last commit from #12522 da73fec, and it's fixed entirely.

All we have to do is decide about the UX, i.e. how to reset fractional sizes when a new track is loaded so that beatloop has a reasonable default to work with.

daschuer · 2024-02-06T07:16:52Z

Here my retest results:

the spin box jumps to rather random value like 4.01 or 5.07 after loading track, even there is no loop set. (Reloop does nothing)

Not able to reproduce it exactly. Currently the spinbox does not change. If this is a fractional position from the previous deck, you have hard times to use the double/half buttons in a reasonable way. How about just resetting it to a reasonable default?

When I manual edit the value, and than press the beat loop button the old value is applied, because the beatloop still has the input focus. The Enter key works.

The same issue is applies also to the beat jump box.

Known issues . do we have a bug for it?

When I change the loop length using loop-out the spin-box is not updated. (intended?)

Reloop cycle does not change the loop size. (intended?)

Yes, both seems to be the desired behaviour. Unfortunately it is inconsistent with the "feature" of showing the loop size at start up. Did you consider to not adopt non integer beat fractions after loading? This will also solve the first bullet point would feel more consistent.

New loops with Loop-In and Out do not change the loop size box (intended?)

Yes.

Discovered another issue:

Load a track with a stored loop in hot-cue, but no loop in marker set.
Reloop button starts from the very beginning.
This was one time issue.
Reproducible:
Remove a loop
Seek behind a stored loop
Press loop out -> nothing happens = OK
Eject the track
Reload the track
Seek behind a stored loop
Press loop out -> track is looping from the stored loop-in poition.

Conclusion: There is something fishy with reloading a track after a loop has been removed before. Probably thecause of the glitches I have experienced earlier.

ronso0 · 2024-02-06T10:49:44Z

If this is a fractional position from the previous deck, you have hard times to use the double/half buttons in a reasonable way. How about just resetting it to a reasonable default?

Yes, this is part of #12522

It seems there are a few issues with loop reset that were noticeable preiously, for example 5606098

Did you consider to not adopt non integer beat fractions after loading? This will also solve the first bullet point would feel more consistent.

I'd even vote for not adopting any loop size after track load. I don't understand why loop halve/double should immediatly affect the existing loop (temp or saved), that's unhandy if I just want to preprare a new loop.
We can keep the previous loop size if it is in the list of 2^n sizes, else reset to the default value (4).

daschuer · 2024-02-07T07:14:31Z

Sounds good.

daschuer · 2024-04-22T20:04:31Z

How is the state here? I will put it to draft. Please adjust if this is wrong.

ronso0 · 2024-04-23T14:21:26Z

Yeah I've put this on hold already anyway, currently I'm lacking the motivation to get this ready for 2.5
Also I have the impression (vaguely remember) that we did not yet come to a conclusion regarding the optimal UX, related: #12522

I'll pick this up at some point after 2.5 is out.

github-actions · 2024-08-07T00:08:31Z

This PR is marked as stale because it has been open 90 days with no activity.

…et length

github-actions bot added the engine label Jan 4, 2024

ronso0 force-pushed the loopcue-beatloop-size branch from 794caed to eda03ac Compare January 4, 2024 12:30

ronso0 linked an issue Jan 4, 2024 that may be closed by this pull request

setting or activating loop cue of 6 beats sets beatloop_size to -1 #10511

Open

ronso0 force-pushed the loopcue-beatloop-size branch from eda03ac to 9289597 Compare January 4, 2024 12:32

ronso0 added this to the 2.4.0 milestone Jan 4, 2024

ronso0 added the looping label Jan 4, 2024

ronso0 force-pushed the loopcue-beatloop-size branch 2 times, most recently from a197db1 to ce02c52 Compare January 4, 2024 13:25

daschuer requested changes Jan 4, 2024

View reviewed changes

ronso0 force-pushed the loopcue-beatloop-size branch from ce02c52 to 3ab9370 Compare January 5, 2024 01:19

github-actions bot added the code quality label Jan 5, 2024

ronso0 force-pushed the loopcue-beatloop-size branch from 3ab9370 to f7bcc28 Compare January 5, 2024 19:15

ronso0 mentioned this pull request Jan 6, 2024

update beatloop size for loops set manually with IN/OUT #12522

Draft

ronso0 modified the milestones: 2.4.0, 2.4.1 Jan 6, 2024

daschuer reviewed Jan 10, 2024

View reviewed changes

ronso0 commented Jan 10, 2024

View reviewed changes

ronso0 removed the code quality label Feb 29, 2024

ronso0 modified the milestones: 2.4.1, 2.5-beta Mar 20, 2024

daschuer marked this pull request as draft April 22, 2024 20:04

daschuer modified the milestones: 2.5-beta, 2.6-beta May 8, 2024

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Aug 7, 2024

ronso0 added 5 commits August 7, 2024 13:58

move positioNear(FramePos, FramePos) to FramePos::isNear(Framepos)

28327e7

move getBeatContext() and ~NoLookup() to Beats class

533ccdc

CueControl: separate debug assertions

9175fec

Beats: add helper to get fractional beats in frame range

9884336

Looping: get correct size when saving/reactivating loop with non-pres…

feb5fea

…et length

ronso0 changed the base branch from 2.4 to main August 7, 2024 12:27

ronso0 force-pushed the loopcue-beatloop-size branch from f7bcc28 to feb5fea Compare August 7, 2024 12:27

github-actions bot added the code quality label Aug 7, 2024

ronso0 removed code quality stale Stale issues that haven't been updated for a long time. labels Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Looping: update loopsize spinbox when recalling loop with any size #12509

Looping: update loopsize spinbox when recalling loop with any size #12509

ronso0 commented Jan 4, 2024 •

edited

Loading

daschuer left a comment

daschuer Jan 4, 2024

ronso0 Jan 4, 2024

daschuer Jan 4, 2024

ronso0 Jan 4, 2024

daschuer Jan 4, 2024

ronso0 Jan 6, 2024

ronso0 commented Jan 4, 2024

ronso0 commented Jan 4, 2024

ronso0 commented Jan 5, 2024

daschuer left a comment

daschuer Jan 10, 2024

ronso0 Jan 10, 2024

ronso0 left a comment

ronso0 Jan 10, 2024

daschuer commented Jan 11, 2024

ronso0 commented Jan 11, 2024 •

edited

Loading

ronso0 commented Jan 11, 2024

ronso0 commented Jan 11, 2024 •

edited

Loading

ronso0 commented Jan 11, 2024

ronso0 commented Feb 5, 2024

daschuer commented Feb 6, 2024

ronso0 commented Feb 6, 2024

daschuer commented Feb 7, 2024

daschuer commented Apr 22, 2024

ronso0 commented Apr 23, 2024

github-actions bot commented Aug 7, 2024

	// returns true if a is valid and is fairly close to target (within +/- 1 frame).
	// returns true if this and target are valid and target is fairly close (within +/- 1 frame).

Looping: update loopsize spinbox when recalling loop with any size #12509

Are you sure you want to change the base?

Looping: update loopsize spinbox when recalling loop with any size #12509

Conversation

ronso0 commented Jan 4, 2024 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Jan 4, 2024

ronso0 commented Jan 4, 2024

ronso0 commented Jan 5, 2024

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jan 11, 2024

ronso0 commented Jan 11, 2024 • edited Loading

ronso0 commented Jan 11, 2024

ronso0 commented Jan 11, 2024 • edited Loading

ronso0 commented Jan 11, 2024

ronso0 commented Feb 5, 2024

daschuer commented Feb 6, 2024

ronso0 commented Feb 6, 2024

daschuer commented Feb 7, 2024

daschuer commented Apr 22, 2024

ronso0 commented Apr 23, 2024

github-actions bot commented Aug 7, 2024

ronso0 commented Jan 4, 2024 •

edited

Loading

ronso0 commented Jan 11, 2024 •

edited

Loading

ronso0 commented Jan 11, 2024 •

edited

Loading