-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Beatloops snap to the closest (fractional) beat in quantized mode LP1752133 #2286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for continuing this. I have left some comments.
loops_per_beat)); | ||
newloopSamples.start = prevBeat + beat_len / loops_per_beat * beat_frac; | ||
double beatLength = nextBeat - prevBeat; | ||
double beatPos = currentSample - prevBeat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be samplesSinceBeat or so
|
||
// find the two closest beat fractions and place the new loop start to the closer one | ||
double fractionBeatLength = beatLength * beats; | ||
double previousFractionBeat = prevBeat + (floor(beatPos / fractionBeatLength)) * fractionBeatLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The () around is redundant. You can use it to emphasize that * is done first or move the + prevBeat to the end.
// find the two closest beat fractions and place the new loop start to the closer one | ||
double fractionBeatLength = beatLength * beats; | ||
double previousFractionBeat = prevBeat + (floor(beatPos / fractionBeatLength)) * fractionBeatLength; | ||
double nextFractionBeat = prevBeat + (floor(beatPos / fractionBeatLength) + 1) * fractionBeatLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double nextFractionBeat = previousFractionBeat + fractionBeatLength;
:-)
double previousFractionBeat = prevBeat + (floor(beatPos / fractionBeatLength)) * fractionBeatLength; | ||
double nextFractionBeat = prevBeat + (floor(beatPos / fractionBeatLength) + 1) * fractionBeatLength; | ||
|
||
if (abs(previousFractionBeat - currentSample) <= abs(nextFractionBeat - currentSample)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (currentSample - previousFractionBeat <= nextFractionBeat - currentSample) {
// | ||
// TODO: There must be a better way of checking if we are running reverse. Getting the "old_reverse" from the EngineBuffer somehow seems wrong. | ||
// Furthermore, it does not work when we are not playing and the reverse button is held. | ||
if (getEngineBuffer()->isReverse()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I have now better idea. This is the real reverse state of the last engine call that includes scratching and seeks. You actually want the future. This: ControlPushButton(ConfigKey(group, "reverse")); would be the reverse button.
This excludes scratches and seeks, probably desired?
src/track/trackmetadatataglib.cpp
Outdated
@@ -1867,7 +1867,7 @@ bool exportTrackMetadataIntoID3v2Tag(TagLib::ID3v2::Tag* pTag, | |||
DEBUG_ASSERT(pHeader); | |||
if (!checkID3v2HeaderVersionSupported(*pHeader)) { | |||
kLogger.warning() << "Legacy ID3v2 version - exporting only basic tags"; | |||
exportTrackMetadataIntoTag(pTag, trackMetadata); | |||
exportTrackMetadataIntoTag(pTag, trackMetadata, WRITE_TAG_OMIT_NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, totally unlerated but building failed without this. Occurred since one master merge from @uklotzde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see this change is already part of current master.
Can you rebase your commit on top of the current master:
"git rebase upstream/master"
This way we avoid to track the same change in two git branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked perfectly.
Thank you for this PR and the changes. LGTM |
Quantized beat loops always started on the previous beat while loop in and loop out markers snapped to the closest beat. Now both snap to the closest beat. If the closest beat is ahead of the current play position a catching loop is generated.
Fixes https://bugs.launchpad.net/mixxx/+bug/1752133