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

Spinback fix #4708

Merged
merged 9 commits into from
Apr 6, 2022
Merged

Spinback fix #4708

merged 9 commits into from
Apr 6, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Mar 29, 2022

Fixes a few spinback "regressions" noticed in #4705

  • fix infinite acceleration (never comes to halt currently)
  • spinback when interrupting softStart (was spinning forward)
  • allow spinback on stopped deck

The first two are fixed by a single - 7fed042 (after hours trying to understand the alpha-beta scratch filter).

Allowing spinback on stopped deck (IIRC that was working a few years back) was more work as it required cleaning up brake(), and I hope someone has motivation to review it so we can merge it to "stable" 2.3

  • remove debug output / use kLogger?

@ronso0
Copy link
Member Author

ronso0 commented Mar 29, 2022

still draft because I'd like to keep the debug output 87c23c2 until review is finished.

@ronso0
Copy link
Member Author

ronso0 commented Mar 30, 2022

Also: merging this to main requires manual editing since there are conflicts with renamed and moved files ( + clang-format, no actual behaviour or syntax changes in the functions I touched).
git diff upstream/2.3:src/controllers/controllerengine.cpp upstream/main:src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp

Please ping me before merging so I can make sure I have time to merge to main !!

* allow spinback when deck is stopped
* use -kBrakeRampToRate for spinback to avoid long, inaudible run out
* don't allow brake to interrupt spinback
* add stopScratchTimer() to remove redundant code
@ronso0 ronso0 force-pushed the spinback-fix branch 2 times, most recently from 984b893 to 011a747 Compare March 30, 2022 21:58
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Some more comments. I hope I will find time for a manual test soon.

src/controllers/controllerengine.cpp Outdated Show resolved Hide resolved
src/controllers/controllerengine.cpp Outdated Show resolved Hide resolved
src/controllers/controllerengine.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Mar 30, 2022

Some more comments. I hope I will find time for a manual test soon.

I am testing with the GUI and a MIDI Through mapping. It uses callbacks of toggles on deck2 to init effects on deck1:
https://gist.github.com/ronso0/70bc239c14c1de491dbb87dc303a789f
Eject = init brake
Repeat = init spinback
Slip mode = init softstart

@ronso0
Copy link
Member Author

ronso0 commented Mar 30, 2022

Thanks, I pushed th small fixes.

@daschuer
Copy link
Member

daschuer commented Apr 3, 2022

I have tested it and I think it works like desired.
So I think it is ready to polish for merge.

I must admit hat I have never used the backspin effect before and I think it is implemented unnaturally.
My expectation was that it sound like pushing a vinyl backward with a sharp acceleration an a spin down phase afterwards. Something like demonstrated here: https://www.youtube.com/watch?v=dzEJoSkwvnI at 0:20
Is this a topic of this PR?

@ronso0
Copy link
Member Author

ronso0 commented Apr 3, 2022

Exactly, that's how a backspin is supposed to sound like.
I am sure it was initially implemented like this (I simply doubt someone would implement it in such a weird way as it is now), so I suspect something broke since then, but I didn't figure out when with a quick git search.
Edit I missed "sharp acceleration". Yes, currently the backspin kicks in instantly, no accel. phase.

Actually I think some of the debug output is quite helpful, so I'll remove some of it and put the helpful part behind
#ifdef SCRATCH_DEBUG
Okay?

@ronso0
Copy link
Member Author

ronso0 commented Apr 3, 2022

Thanks for testing btw, appreciated!

@daschuer
Copy link
Member

daschuer commented Apr 4, 2022

Yes OK, thank you.

@daschuer
Copy link
Member

daschuer commented Apr 4, 2022

I will file a bug for the implementation issue. Maybe we find a GSoC contributor taking care with @ywwg.

@daschuer
Copy link
Member

daschuer commented Apr 4, 2022

@ronso0
Copy link
Member Author

ronso0 commented Apr 5, 2022

Last commit disables the debug output in real-time code.
Ready for merge.

@ronso0 ronso0 marked this pull request as ready for review April 5, 2022 22:40

// Give the filter a data point:

// If we're ramping to end scratching and the wheel hasn't been turned very
// recently (spinback after lift-off,) feed fixed data
qDebug() << ".";
Copy link
Member

Choose a reason for hiding this comment

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

Her is a leftover

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

damn, not fixed, didn't git add it proeprly :|
I pushed the fix to 2.3
045ce55

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM

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.

2 participants