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 brake function, add softStart (accelerate from zero) #1085

Merged
merged 11 commits into from Mar 2, 2017

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Dec 20, 2016

Continuing to fix https://bugs.launchpad.net/mixxx/+bug/1571442
"Brake effect initial speed and factor"Initial speed was fixed in #1072.
This PR makes engine.brake() actually take the deceleration factor into account by initializing AlphaBetaFilter with custom beta (How was that done originally at the time the docuwiki was written?).
Additionally, one can now call script.brake(..., factor) in controller script.

@ronso0
Copy link
Member Author

ronso0 commented Dec 20, 2016

Should I remove the whole m_rampFactor from brake() function?
I don't see how it affects the brake effect at all.

@ronso0
Copy link
Member Author

ronso0 commented Dec 20, 2016

..forget about that.
It can be used when ramping to 1 like with a softStart function to accelerate to 1, see
https://bugs.launchpad.net/mixxx/+bug/1643720
Although I would probably create a whole new function instead of making brake() more complex.

@ronso0
Copy link
Member Author

ronso0 commented Dec 27, 2016

Please don't merge yet, I'll extend this PR with further clean-up and softStart() function.

ronso0 added 2 commits January 3, 2017 05:38
removed calculation of current deck rate, this is done in brake/softStart function when no/default rate values are passed.
@ronso0
Copy link
Member Author

ronso0 commented Jan 3, 2017

...and updated common-controller-scripts.js accordingly

@ronso0
Copy link
Member Author

ronso0 commented Jan 12, 2017

bump
I'm done, this can be merged if it's okay. Travis errors aren't related to this code, as far as I can see.
Is clear how softStart() works or should I add comments?

@Be-ing
Copy link
Member

Be-ing commented Jan 12, 2017

Noticed some small JavaScript nitpicks glancing at the code. I'll give this a try soon.

@ronso0
Copy link
Member Author

ronso0 commented Jan 12, 2017

Okay, thank you! updated
Those === and !== are actually copy/paste from existing brake() call.

softStart() is in general like brake() but has different rampTo[] value and turns on 'play'.
scratchProcess() now considers m_softStartActive[] and uses rampTo[] value as target rate when to turn off scratching.

Both brake() and softStart() can interrupt each other without glitches or crashes.

Copy link
Member

@Be-ing Be-ing 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 and compared it to current master. I confirm that it fixes the bug.

It would be nice if the deck was stopped when there is practically no audio coming through. I tried using script.brake() with really low factors (< .3). I held down the button for a long time after sound stopped coming through, but the deck did not actually stop; when I let go of the button, it started playing at its normal speed again. If you don't want to solve this (or don't want to do it right now), that's fine. I think this PR could be merged without that.

res/controllers/common-controller-scripts.js Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Jan 18, 2017

It would be nice if the deck was stopped when there is practically no audio coming through. I tried using script.brake() with really low factors (< .3)

Yeah, that would be nice! So, there had to be a minimum rate, something like setting rampTo to 0.001 for brake(). I'll experiment with what sounds reasonable.
Now that you mention it, I guess the same should be applied to softStart() when it's triggered with really low factors. I'm confident I can finish this until the weekend, then it can be merged.

when I let go of the button, it started playing at its normal speed again.

I hope you don't mind that, it was intended for original brake/spinback. I'm quite sure you noticed that you can map start and stop of brake() or softStart() to different buttons, and then sending just NoteOn (NoteOff respetively).
I can add that explicit info to the wiki to clarify the usage of those two functions. That would also solve this bug I guess https://bugs.launchpad.net/mixxx/+bug/1090780

Edit
Concerning https://bugs.launchpad.net/mixxx/+bug/1090780 I'll check if a parameter like 'continuePlaying' can be sent to engine.brake/spinback functions to automatically continue playback after effect has finished. But this will happen in another PR.

@Be-ing
Copy link
Member

Be-ing commented Jan 18, 2017

I hope you don't mind that, it was intended for original brake/spinback. I'm quite sure you noticed that you can map start and stop of brake() or softStart() to different buttons, and then sending just NoteOn (NoteOff respetively).

Yeah, they could be mapped that way. They could also be mapped to replace a normal play button.

@Be-ing
Copy link
Member

Be-ing commented Jan 29, 2017

@Pegasus-RPG any thoughts on this?

@ronso0
Copy link
Member Author

ronso0 commented Jan 29, 2017

It would be nice if the deck was stopped when there is practically no audio coming through. I tried using script.brake() with really low factors (< .3)

Yeah, that would be nice! So, there had to be a minimum rate, something like setting rampTo to 0.001 for brake(). I'll experiment with what sounds reasonable.

I tried to set the minimum rate via rampTo but that would make the unwanted effect even worse.
I'll check if setting a minimum rate in scratchProcess (for brake/spinback only)
Will be back in a few days, I guess.

@Be-ing
Copy link
Member

Be-ing commented Jan 29, 2017

I think this should be merged now. Improvements for handling very low factors can come in a future PR.

@ronso0
Copy link
Member Author

ronso0 commented Jan 30, 2017

Yup! As soon as this is merged, I'll complete the wiki, including a hint for the low factors.

@daschuer
Copy link
Member

daschuer commented Mar 2, 2017

Thank your for the PR and the review!

@daschuer daschuer merged commit f05b14a into mixxxdj:master Mar 2, 2017
@Pegasus-RPG
Copy link
Member

Especially since features like this are nuanced and carefully determined, PLEASE add tests to ensure that future changes don't adversely affect the behavior.

@ronso0
Copy link
Member Author

ronso0 commented Mar 3, 2017

Cool, thanks for merging! I'll add info to the wiki.
@Pegasus-RPG Unfortunately I have no idea how to write tests. Though, I'm eager to learn, so it would be great if someone else writes those test and I follow that.
There's no test for brake function so far. Because that it's not necessary/possible or just that nobody wrote one yet?

@Be-ing
Copy link
Member

Be-ing commented Mar 3, 2017

There's no test because no one wrote one yet. There is a quick intro to writing tests on the wiki.

@ronso0 ronso0 deleted the fix-brake-function branch March 4, 2017 00:19
@ronso0 ronso0 changed the title Fix brake function Fix brake function, add softStart (accelerate from zero) Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants