Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Loop_out not seeking back #12739

Merged
merged 4 commits into from Feb 6, 2024
Merged

fix Loop_out not seeking back #12739

merged 4 commits into from Feb 6, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Feb 5, 2024

This fixes seeking in the following situation I noticed in #12522 (comment)

  • enable quantize
  • move playhead slightly after a beat
  • set loop_in
    = snaps to previous beat
  • jump forward by 4 beats
  • set loop_out
    = snaps to previous beat, playhead moves 4 beats back inside the loop
  • deactivate loop
  • jump forward by 4 beats
  • set loop_out
    (= previously: snaps to prev. beat, playhead doesn't move, loop will never be reached)
    = FIX: snaps to previous beat, playhead moves 4 beats back inside the loop

Note:

As before, (re)setting loop_out with playpos exactly at loop_out of the inactive loop, playpos should stay at loop_out and not jump to loop_in.

See discussion on Zulip https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/2.2E4.20behaviour.20.2F.20failing.20test.20of.20loop_in.2F_out.20with.20quantize

TODO

  • Adjust LoopInOutButtons_QuantizeEnabled

@github-actions github-actions bot added the engine label Feb 5, 2024
@ronso0
Copy link
Member Author

ronso0 commented Feb 5, 2024

The test LoopInOutButtons_QuantizeEnabled should fail now.

@ronso0 ronso0 added the looping label Feb 5, 2024
@ronso0
Copy link
Member Author

ronso0 commented Feb 6, 2024

The test LoopInOutButtons_QuantizeEnabled should fail now.

It doesn't so it seems there is indeed a ProcessBuffer() call missing.

@ronso0
Copy link
Member Author

ronso0 commented Feb 6, 2024

Seems I misread the test procedure 🙄
The test is good, and it succeeds (for the initial loop_out setting).
edit no, I'm having trouble translating the framePos calculation.

I extended it to cover the case this PR fixes.

@ronso0 ronso0 force-pushed the loop-out-seek-fix branch 2 times, most recently from 83d31a3 to 30f9bfd Compare February 6, 2024 10:39
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ywwg
Copy link
Member

ywwg commented Feb 6, 2024

extremely annoyingly, the first time I hand-tested it appeared the fix did not work, but every time I tried afterward it did. Is there a chance that old_loop_info could be blank, but an out point is set?

@ywwg
Copy link
Member

ywwg commented Feb 6, 2024

Unless someone else can reproduce the failure, I think this is a Good Enough fix for the release. If we find a further edge case later we can fix it in 2.4.1

@ronso0 ronso0 marked this pull request as draft February 6, 2024 15:08
@ronso0
Copy link
Member Author

ronso0 commented Feb 6, 2024

Draft until the test is fixed.

@ronso0
Copy link
Member Author

ronso0 commented Feb 6, 2024

I disabled the test and filed #12741

@ronso0 ronso0 marked this pull request as ready for review February 6, 2024 16:37
@@ -322,31 +322,47 @@ TEST_F(LoopingControlTest, LoopOutButton_AdjustLoopOutPointInsideLoop) {
EXPECT_FRAMEPOS_EQ_CONTROL(mixxx::audio::FramePos{1500}, m_pLoopEndPoint);
}

TEST_F(LoopingControlTest, LoopInOutButtons_QuantizeEnabled) {
TEST_F(LoopingControlTest, DISABLED_LoopInOutButtons_QuantizeEnabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a short comment that links to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, done.

@ywwg ywwg merged commit 2562466 into mixxxdj:2.4 Feb 6, 2024
14 checks passed
@ronso0 ronso0 deleted the loop-out-seek-fix branch February 6, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants