-
-
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
Beats: Add editing controls and show downbeats on scrolling waveforms [v2] #12343
Conversation
int calculateBeatsTillNextMarker(const mixxx::Beats::ConstIterator& it, | ||
mixxx::audio::FramePos nextMarkerPosition) { | ||
const double numBeatsDouble = std::fabs(nextMarkerPosition - *it) / it.beatLengthFrames(); | ||
const int numBeatsInt = static_cast<int>(std::round(numBeatsDouble)); |
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.
We round here twice. That may lead to a wrong overall rounding
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.
Assuming every int
can safely be represented as double
, which is the case on the overwhelming majority of desktop platforms where int
is 32-bit and double
is a 64-bit IEEE-754 floating point and given that std::round
is guaranteed to round to the nearest integer, this is usually safe. See https://stackoverflow.com/a/47153373/19890279.
I agree, however, that it might be more elegant to go with std::lround
. We would still have to cast the long
to an int
though.
return numBeats + beatsPerBar - numExcessBeats; | ||
} | ||
|
||
int calculateBeatsTillNextMarker(const mixxx::Beats::ConstIterator& it, |
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 function name is misleading. How about:
int beatsTillBarNearPosition()
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.
It looks like there is a need for "calculateBeatsTillNextMarker()" this should however be independent form a Bar.
|
||
/// Returns the current index of the beat in the bar. | ||
int beatIndexInBar() const { | ||
return m_beatOffset % beatsPerBar(); |
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 assumes that the markers always align with a bar. This is probably musically correct.
However the model allows overlapping bars, and especially when we have dynamic beat-gits created in earlier version of Mixxx, we can have tempo changes in any place. We need to deal with it.
From the GUI perceptive, I would be in favour to require a bar for a temp 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.
for cases where the tempo is increasing gradually, every beat may be a little faster or slower than the last. We should allow tempo changes on any beat marker.
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.
Perhaps a second parameter would be useful to describe accelerando/ritardando in beats/s². On the other hand, the case might be too rare to justify this overhead.
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.
I agree with @ywwg, tempo changes on every beat are common enough that we should support them. The problem I see is that it's probably not easy to retrofit into our current beatgrid model (that has no information about bars/measures) if we cannot interpret markers as downbeats. So we'd probably have to defer this for now, just like changing time signatures, which I'd very much be interested in having too.
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.
I see a chance to integrate that.
For my understanding we need to add a an bar offset along with the beats per bar value.
This way we are still able to express the old free beat map with the new model
‖___|___|___|___‖___|__|__|__‖__|__|___|___‖___
M M M .
120bpm. 180 bpm 120 bpm
barAt 0. barAt 3 barAt 2
beatsPerBar 4 beatsPerBar 4 beatsPerBar 4
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.
it may well be that bpm-per-measure is "close enough" that it works with bpm speed-up/slowdowns or non-quantized tracks (disco etc). If we can test this situation early then we'll know if it's a problem or not. I have a few tracks that are my go-tos for difficult beatgrids, I can send them to you @fwcd if that helps
By the way, another idea I had for making the implementation of downbeats + changing time signatures backwards-compatible would be to store downbeat/time-signature change markers in a separate map/db column, e.g. in terms of beat indices (as opposed to frames to avoid redundancy). That way we could safely iterate on that format without impacting older Mixxx versions until the 2.5 release. Thoughts? (Would also be out-of-sope for this PR, but perhaps as a general direction) |
The downside is that this needs another lookup. |
That's a fair point, though that lookup should be fairly quick if we do a binary search. I don't have a good enough overview of the current implementation, but if this is something that only becomes relevant at seeks, my intuition would be that that should be performant enough to be worth trying? I think maintaining a backwards-compatible data format would be a huge plus, since I worry that once we start changing the existing beatmap format, we'll (understandably) want to take our time to get it right to avoid messing up the databases of users/testers/developers and end up with massive PRs again... |
Maybe the new system could be used alongside the old one and we could have two beatgrids for one track. At track load time, the new code would first do a lookup in the new table, and if the beat grid is not there, look for it in the old table and then do an upgrade to the new format. Old code would ignore the new table and leave it alone. |
b23c33e
to
d6c5070
Compare
Did a quick rebase onto
I think that's a neat idea, though the redundancy in keeping both grids around might manifest itself in quite a bit of space in large libraries. Also, I'm a bit worried that if we bake this into our schema now, the decision to keep two grids around per track is going to be set in stone once we ship it (if we want to keep backwards compatibility). Another issue is that I am not sure how well this would "scale" to future changes of the beat grid scheme, since we would effectively have to add a new column every time (a table is probably overkill in any case). Storing just the "new" information would solve this if we could figure out how to make the lookups performant enough. |
I think we should import the old library at the first start of the new version. We can ask the user in a dialog, if he wants to delete the old library or delete it. |
Yeah, the question would be if we could make it backwards-compatible to avoid introducing a "hard" migration. I could see this being convenient, especially for developers which like to hop between different versions. The fact that I can run 2.3 without switching databases has proven useful several times while diagnosing issues. |
Or we add the read functionality for the new format in Mixxx 2.5, but the downbeat feature that writes this format one release later. |
I am not quite following, how would we use downbeats if we can't write them anywhere? Use a fresh grid every time the user opens the track? |
Than 2.5 would only show the beats, but read from the new library format, ignoring all new elements like downbeats. |
Ah I see, we would still break backwards compatibility with all versions older than 2.5 in that case though. I agree that it would be an option, but I also think it would be nice if users already had something to play around with. The specifics of the format should ideally be able to evolve alongside the UI as we figure out more details. |
I like the idea of allowing the user to choose whether to keep the old grids or delete them. For me, disk space is cheap, and even the 4.4G I have in my analysis directory is not a big deal. We could also give the user the option to delete the old beats later if they kept them during the upgrade. And when 2.6 comes out we can say "hey if you never plan on using 2.4 again we can delete this data". |
A two-version migration for the hard format change is a great idea, +1! |
This PR is marked as stale because it has been open 90 days with no activity. |
@fwcd do you think you can work on this during the dev phase? |
Sure, feel free to take over this PR (which is mostly just a rebase of @Holzhaus, nothing really novel here). I'm a bit short on time right now, so if you're interested, go ahead |
Closing in favor of #13330 |
This is a rebase of @Holzhaus's #4489 onto
main
, a proof-of-concept implementation of downbeat markers and buttons for setting downbeats in the grid:To do
Deferred