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) Waveform scratch crossing loop boundaries #13007

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Mar 27, 2024

Previously, scratching with the waveform and crossing loop boundaries at slow rates caused an infinite wrap-around 'loop' even after the mouse was stopped (while still pressed).
Probably affects Spinny scratching, too, but I didn't test it.

PositionScratchController::process() now knows if the play position was wrapped around and calculates the true sample distance travelled.
I notice this would also be helpful for and probably simplify #12951 hotcue_X_indicator control

TL;DR
PositionScratchController didn't know the play position was wrapped around, so it
interpreted the raw (huge) sample delta as play pos move and calculated a huge rate, which was
then applied by the engine, resulting in a (true) long sample distance in the next run,
which caused another loop wrap-around, and so on...

@ronso0 ronso0 changed the title Waveform scratch loop fix (fix) Waveform scratch crossing loop boundaries Mar 27, 2024
@ronso0 ronso0 added this to the 2.4.1 milestone Mar 27, 2024
@ronso0
Copy link
Member Author

ronso0 commented Mar 27, 2024

This is now cleaned up.
You may pick d4a0daa to add some debug output.

@ronso0 ronso0 marked this pull request as ready for review March 28, 2024 13:21
Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

I tested this on Windows and everything works as expected!

Comment on lines 622 to 624
// TODO FramePos only for easy validation.
// Check how to get kLegacyInvalidEnginePosition in order to use sample pos
// for the entire
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to switch from frame position to sample position?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's passed only to PositionScratchController and uses only sample positions for processing anyway, so I thought we'd better convert it early. No issue though, and getting kLegacyInvalidEnginePosition here requires more includes, so I can as well remove the TODO.

@Swiftb0y
Copy link
Member

I remember mentioning this in an issue or zulip post sometime ago, but I can't find it anymore

@ronso0
Copy link
Member Author

ronso0 commented Mar 29, 2024

yeah, we touched this topic sometime, but I can't find it anymore either.

@ronso0
Copy link
Member Author

ronso0 commented Mar 29, 2024

I tested this on Windows and everything works as expected!

An important test case, which failed with my previous attempts, is a fast throwback with a very short loop (< 1 beat), then hold the mouse button until scratching (inertia) stopped. This should perform exactly like a throwback with no loop.
And scracthing should also sound right, not just look reasonable on the waveforms.

@JoergAtGithub
Copy link
Member

I tested this on Windows and everything works as expected!

An important test case, which failed with my previous attempts, is a fast throwback with a very short loop (< 1 beat), then hold the mouse button until scratching (inertia) stopped. This should perform exactly like a throwback with no loop. And scracthing should also sound right, not just look reasonable on the waveforms.

I tested this, and it worked correct!

@ronso0
Copy link
Member Author

ronso0 commented Mar 29, 2024

Great. So I remove the TODO and this is ready?

PositionScratchController::process() now knows if the play position was wrapped around (and
how often) since the last processing. This avoids an infinite wrap-around 'loop', even after
the mouse didn't move.

Previously, PositionScratchController didn't know if the play position was wrapped around, so it
interpreted the raw (huge) sample delta as play pos move and calculated a huge rate, which was
then applied by the engine, resulting in a (now true) long sample distance in the next run,
which caused another loop wrap-around, and so on...
@ronso0 ronso0 force-pushed the waveform-scratch-loop-fix branch from 97b1c4a to 50e16c9 Compare April 4, 2024 09:09
@ronso0
Copy link
Member Author

ronso0 commented Apr 4, 2024

Done.

@JoergAtGithub
Copy link
Member

LGTM! Thank you!

@JoergAtGithub JoergAtGithub merged commit 7b19c11 into mixxxdj:2.4 Apr 5, 2024
14 checks passed
@ronso0 ronso0 deleted the waveform-scratch-loop-fix branch April 5, 2024 16:34
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.

3 participants