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

Rate ratio #1765

Merged
merged 23 commits into from
Nov 15, 2019
Merged

Rate ratio #1765

merged 23 commits into from
Nov 15, 2019

Conversation

daschuer
Copy link
Member

This PR introduces the new control rate_ratio, which is 1.0 at normal speed.
This can is used by scripts and in various places in Mixxx and helped to remove code duplication, calculating it on the fly. Reduced by 146 Lines of code.
It decouples also the rate from the rate sider. This allows to introduce alternative rate control ideas.

For example:
https://bugs.launchpad.net/mixxx/+bug/1762172

Which makes the rateRange finally usable during a Gig.
This new concept also includes the feature that the rate-slider soed not go out of sync

this, SLOT(slotRateSliderChanged(double)),
Qt::DirectConnection);
m_pRateRange = new ControlPotmeter(
ConfigKey(group, "rateRange"), 0.01, 0.90);
Copy link
Member

Choose a reason for hiding this comment

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

Change this to 0.01, 4.00 please, in keeping with #1662.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this will happen automatically when merging 2.1 to master.

@Pegasus-RPG
Copy link
Member

I haven't looked this over completely yet, but can you post here what will be in the MixxxControls wiki for rate_ratio? Its name doesn't clearly intimate what it does or how it's supposed to be used.

@daschuer
Copy link
Member Author

rate_ratio is the unscaled ratio between the original rate (Speed) and the current rate. It is 1.0 if the track plays at original rate.

@daschuer
Copy link
Member Author

It matches also the mpris definition of rate
https://specifications.freedesktop.org/mpris-spec/2.0/Player_Interface.html#Property:Rate

@rryan
Copy link
Member

rryan commented Oct 31, 2018

Is there a use case for this CO for skin or controller authors, or is it only internal usage?

I would prefer we do not make it a public CO and keep it as an implementation detail -- one risk is that we could produce feedback loops between the 4 COs.

@daschuer
Copy link
Member Author

daschuer commented Nov 1, 2018

The case is to provide a easy to use CO for script calculations, that does not depend on user settings.
This "reliable" value is for my feeling a prerequisite when we start to allow changing the rate scale on the fly.

Conflicts:
	src/engine/sync/synccontrol.cpp
Conflicts:
	src/engine/bpmcontrol.cpp
	src/engine/keycontrol.cpp
	src/engine/keycontrol.h
	src/engine/ratecontrol.cpp
@daschuer
Copy link
Member Author

I have now added the follow up commits, to show how useful the new rate_ration CO is.

Conflicts:
	src/engine/bpmcontrol.cpp
	src/engine/ratecontrol.cpp
	src/engine/ratecontrol.h
@Be-ing
Copy link
Contributor

Be-ing commented Jan 10, 2019

There are some merge conflicts now. Could you fix those?

Conflicts:
	src/engine/controls/bpmcontrol.cpp
	src/engine/controls/keycontrol.cpp
	src/engine/sync/synccontrol.cpp
	src/widget/wnumberrate.cpp
Conflicts:
	src/engine/controls/ratecontrol.cpp
	src/engine/controls/ratecontrol.h
	src/engine/sync/synccontrol.cpp
	src/test/enginesynctest.cpp
@daschuer
Copy link
Member Author

This is again merge-able.

@@ -873,14 +849,9 @@ void BpmControl::collectFeatures(GroupFeatureState* pGroupFeatures) const {

// Note: dThisBeatLength is fractional frames count * 2 (stereo samples)
pGroupFeatures->beat_length_sec = dThisBeatLength / kSamplesPerFrame
/ sot.rate / calcRateRatio();
/ sot.rate / m_pRateRatio->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Without the lower bound of 1e-6 in calcRateRatio() this might now trigger a division-by-zero. Did you consider this edge case?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, good point.

@@ -226,7 +222,7 @@ void BpmControl::slotTapFilter(double averageLength, int numSamples) {

// (60 seconds per minute) * (1000 milliseconds per second) / (X millis per
// beat) = Y beats/minute
double averageBpm = 60.0 * 1000.0 / averageLength / calcRateRatio();
double averageBpm = 60.0 * 1000.0 / averageLength / m_pRateRatio->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential division-by-zero here, too

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

2 new potential division-by-zero

@daschuer
Copy link
Member Author

done

@daschuer
Copy link
Member Author

This is again in a meregable state.

@daschuer
Copy link
Member Author

@uklotzde: Can we merge this now?

@uklotzde
Copy link
Contributor

Sure, sorry for the delay.

@uklotzde uklotzde merged commit 60079a8 into mixxxdj:master Nov 15, 2019
daschuer added a commit to daschuer/mixxx that referenced this pull request Dec 3, 2019
uklotzde added a commit that referenced this pull request Dec 17, 2019
@Be-ing
Copy link
Contributor

Be-ing commented Sep 10, 2021

This has not been documented in the manual.

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.

5 participants