-
-
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 [vn+1] #13330
base: main
Are you sure you want to change the base?
Conversation
Just for completeness, #4489 was just a small part splitted of from this work: https://github.com/mixxxdj/mixxx/wiki/Measures-Downbeats-Bars-And-Phrases |
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.
haven't been able to look at the meat (beats.cpp
) yet. In the meantime, can you take care of CI failures. The inline annotations produced make it hard to review (and they need to resolved anyways, so better sooner than later).
Thanks for looking into this @Swiftb0y this quick! With my latest commit, I should fix most of the CI issues + failing test, and also started cleaning up some of the mess. Before looking too much into the code, I'd been keen to hear whether or not we are okay with approach I've taken from a UX perspective, and perhaps also gets some user testing first. As I said in introduction, I know there was a lot of discussion on the matter and I'd like to make sure this doesn't end up abandoned as well. |
636a042
to
b1967d9
Compare
With the last commit, I have introduce customizable beats per bar for each markers. To do so, I'm using a "double beat" in the beat map, meaning a beat with the same position that the previous one. This is used to infer the bar length. Pro: no need for a proto change @JoergAtGithub I tried many things but couldn't reproduce it. Could you please provide me with some steps and maybe a short recording? I'm probably missing a settings or something. |
And now I got the original again:
|
Right, my fault, The first one is related to the |
The link "here" doesn't point to a particular line of code. But I wonder if it's possible to add some code to the 2.4.2 release that just prevents the grid invalidation. And than merge this PR with the new beatgrid feature into 2.6. |
@JoergAtGithub I believe fixed most of the asserts and added some tests to cover the new introduced use-cases. I would appreciate if you could give it another go 🙏 |
94c256f
to
e06279c
Compare
e06279c
to
bc15b8d
Compare
It's worth to highlight the invalidation would only happen if one:
So while it feels quite an edge case, it may also feels fairly natural to need to regenerate the beatgrid to the user, so perhaps worth keeping as is. |
I tested again, and no asserts occured anymore!
The missing backward compatibility, was one of the main reasons, why the predecessor PRs got never merged. We need an commonly agreed strategy here. |
The markers in the waverforms are difficult to recognize. The visual representation in the original PR #2961 was more clear in my opinion: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/beatgrid.20editing.20UI/near/206216183 |
Great to hear!
Make sense to me. What do you think of my proposition of backward compatibility, except if the multiple bar length feature is used?
Yeah it could be better I agree. The colour of the bar marker can be customised by the them. By default I wanted to have something not striking to much ( like the original red colour used before), but I guess we could find a middle ground. |
Overall, are we happy with getting this PR in as is? Just wondering if I should start looking at making it production ready, and clean up the implementation/fix conflict or of there is still outstanding controversial aspects. |
Well, the primary thing I'm confused about currently are the complications around |
One of the primary changes are that you changed the beatmarkers to store the beatlength instead of the total beatcount, right? Can you share some of the rationale behind that or link me to a previous discussion? |
bc15b8d
to
693b0e4
Compare
I broke down the feature in two PR now (follow up here) and managed to reduce slighly the size. |
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'm not familiar with the code in question so I'm the wrong person to review this for anything but surface-level comments.
enum class TempoAdjustment { | ||
Faster = 0b01, | ||
Slower = 0b10, | ||
// The following variant are used to indicate wider adjustment requests | ||
MuchFaster = 0b100 | Faster, | ||
MuchSlower = 0b100 | Slower, | ||
}; |
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 don't see the value in this flag-style approach, after all, what would 0b11
mean? But I don't see any downside right now either.
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.
Yeah, that's a good point. The reason for doing so, as oppositie to use maybe on bit to define the direction was to allow a null type but I guess that would make sense.
Do you think the following would make more sense? The reason for that flag-style approach was to have meaningful paramater, opposition to a bool, bool
signature.
enum class TempoAdjustment { | |
Faster = 0b01, | |
Slower = 0b10, | |
// The following variant are used to indicate wider adjustment requests | |
MuchFaster = 0b100 | Faster, | |
MuchSlower = 0b100 | Slower, | |
}; | |
enum class TempoAdjustment { | |
Faster = 0b1, | |
Slower = 0b0, | |
// The following variant are used to indicate wider adjustment requests | |
MuchFaster = 0b10 | Faster, | |
MuchSlower = 0b10 | Slower, | |
}; |
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 saw that there is some more complicated bit manipulation in the code after. I think it would make sense to encapsulate that by wrapping the enum in a class and the implementing the possible operations as simple member functions. This shouldn't introduce a huge amount of boilerplate and would the code more robust and easier to understand.
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.
we should look into how often this is called, just because the modulo might get expensive.
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 will have a look.
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.
don't waste your time on it for now, it was just a thought. I'm actually more concerned that the iterator operations may be inefficient (because operations such as --
actually need
// Track: |--------------------------------------------------------| | ||
// | | | | | || | | || | | || | | | || |
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'm not familiar with the double beat concept. Where do they come from, the analyzer?
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.
No, this is how the beat marker is translated in the BeatMap: two marker at the exact same positions. By default, the analyser will only issue single beat marker, and so the deserialisation process will infer that the first beat is a beat marker and that the bar length is 4 as default. However, if you set a beat marker (Using the CO, UI available in next PR), it will be serialised using this double beat in the map, the 4-beat bar will be relative to that beat, If you have an other double beat marker withing the acceptable range ([2-32]) it will be used to inferred the bar length, starting from that beat till the end of the track (or the next marker)
I appreciate it may feel a bit complicated, but that was the only way I could reuse the existing proto definition and make that change non-breaking (and I could read it was a point of contention in the past attempt of addressing that feature...)
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.
Not sure about this.
The main reason for not changing the serialized beatmap format is to stay compatible with other mixxx versions (since we were not sure if this approach for downbeat is really the way to go). Although this PR doesn't change the beatmap format, it actually results in beatmap data that never occurred before. Will older versions of Mixxx be able to handle the double beats gracefully?
If not, we might as well add a new "is_marker" field or revamp the serialization format entirely. Not against this, just wanted to point out that sticking to the old format becomes pointless if it becomes incompatible anyway.
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.
Will older versions of Mixxx be able to handle the double beats gracefully?
From my testing with 2.4 and 2.5, yes it does. The only issue is a DEBUG_ASSERT
, but nothing else which seems to impact the correct functionning.
we might as well add a new "is_marker" field
That would definitely be cleaner. Do we have consnsus on adding a new proto definition? AFAIK, we should be able to add this without issue a new versionning either and only make the most of protobuf field versionning here, so that sounds minimal enought to be acceptable here. Wdyt?
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 appreciate it may feel a bit complicated, but that was the only way I could reuse the existing proto definition and make that change non-breaking
I like the idea to have a compatible format. I am just afraid that some sync and quantize code will be confused by the double beat. What are the alternatives?
message BeatMap {
repeated Beat beat = 1;
}
Will an extra array for the bar lines also work?
message BeatMap {
repeated Beat beat = 1;
optional repeated Beat bar_line = 2;
}
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 doesn't not, since the double beat is only used to detect marker and bar length (beats per bar). The problem is, When storing the beeatmap, we store the interpolated beats as well. Markers are transformed in double-beat, so we could add two new properties to Beat
? Something like ismarker
and beatPerBar
?
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'm wondering if we could just go with this in a first place, and then introduce the new proto definition if it does turn out to break something. After all, that is what alphas are for ;-)
🙏🏻 thank you! |
I'm not familar with this part of the code. AFAIK @Holzhaus is the expert here. |
@Holzhaus are you interested in reviewing this? |
Yes, I promise I will review this weekend. |
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.
Thanks for the PR. I have to admin that I cannot comment on the dowbeat support tbh, because that seems to have been removed from this PR (as well as the skin changes). Right-click-and-holding the beats adjustment buttons also didn't work.
Code looks mostly good to me (apart from some minor nits).
I'm not entirely sure regarding usability. My most common usecase for which I see the need to edit the beatgrid are following:
- Beatgrid shifted by a half beat
- Beats are detected correctly at the start of the track, but slowly go out of sync later on in the track.
To fix 1) I can just use the beats translation. I usually use beats_translate_curpos
or beats_translate_match_alignment
.
For 2), my idea for #4489 was that I can fix the situration by moving the playpos to a late beat in the track, click beats_set_marker
(mapped to a button on my controller) and the beatgrid autostretches to accomodate this. I might have to adjust the number of beats between the two markers, but you get the idea.
This PR does not do that, as described in the PR description:
BPM doesn't get change when adding or removing a new marker
This makes 2) kind of cumbersome, because now the new marker is kind of pointless: It doesn't really help because you still need to adjust manually using the beats adjust faster/slower buttons, and you cannot be sure that the number of beats between the two markers is a multiple of four without checking youself (the latter might be resolved by adding back downbeat display).
Here's a smal video to show what I mean:
Peek.2024-10-13.13-26.mp4
I have to admit that I didn't really follow the discussion, so maybe there is a good reason for this behavior. But this way of editing the beatgrid seems to be not a good fit for my use cases (or I don't know how to use it properly). Anyway, I'm not against this PR at all, so you have green light from my side if the other team members like the editing workflow.
// Track: |--------------------------------------------------------| | ||
// | | | | | || | | || | | || | | | || |
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.
Not sure about this.
The main reason for not changing the serialized beatmap format is to stay compatible with other mixxx versions (since we were not sure if this approach for downbeat is really the way to go). Although this PR doesn't change the beatmap format, it actually results in beatmap data that never occurred before. Will older versions of Mixxx be able to handle the double beats gracefully?
If not, we might as well add a new "is_marker" field or revamp the serialization format entirely. Not against this, just wanted to point out that sticking to the old format becomes pointless if it becomes incompatible anyway.
Thanks for the in depth review @Holzhaus !
This was requested above - split the PR to reduce the amount of changes. UI changes can be seem in this PR. The branch also include the commit from this PR so it might make it easier to test! Apologies, I should have updated the description accordingly.
I'll look into that. It could be that I broke it when I moved all the UI changes to a follow up PR.
This approach was preventing using this feature on a few of my tracks, and didn't allow a workaround. This was including tracks with
With the current approach, as your rightfully demoed, this covers all usecase, although I appreciate for yours, it makes it a little bit less smooth. What do you think about me adding another CO (e.g |
Yup, smaller PRs are easier to review. Thanks.
Sounds reasonable, but for a controller-based workflow, the number of buttons is limited and I'd like to avoid that the mapping creator make the choice which CO to map because the library/use cases might be different for other users with the same controller. If there already is a marker at the current play position, What do you think about calling |
I guess one could always bind the
My fear is that there could be risk of accidentally breaking the grid. It's worth to say that this code was written before |
Yup, sounds good. I think there are many ways to support such use cases. We could also add a config option to modify the default behavior. Anyway, nothing in scope of this PR. There are failing checks now. If you fixup the commit, I suppose this is ready to merge. |
Great to hear! I have added some |
@Holzhaus are you happy to approve this before I squash the fixups? |
src/engine/controls/bpmcontrol.cpp
Outdated
@@ -117,6 +143,34 @@ BpmControl::BpmControl(const QString& group, | |||
this, | |||
&BpmControl::slotTranslateBeatsMove, | |||
Qt::DirectConnection); | |||
m_pBeatsSetMarker = std::make_unique<ControlPushButton>( | |||
ConfigKey(group, "beats_set_marker"), false); |
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 "marker" in imprecise.
We may use "bar line", in German "Taktstrich". That would be easier to undertsand for me.
https://en.wikipedia.org/wiki/Bar_(music)
Is that ment here?
So we may call it beats_set_bar_line
We may also consider to introduce a new bar
prefix for
bar_line_set
bar_increase_lenght
An alternative would be downbeat
https://en.wikipedia.org/wiki/Beat_(music)#Downbeat_and_upbeat
`downbeat_set'
This works in the CO interface but is a translator trap because there does not exist a precise translation in
German and other languages. But for "counting downbeats" I would say in German "Takte Zählen" = "Counting bars". The Downbeat itself is "Die Eins" = "The One".
How is the situation in france?
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 is not the same. You don't need to set every bar line manually, they are interpolated based on the position of the marker and the BPM. The "Marker" is the frame position where the first downbeat with new settings is. I think the term is fine. Using established terminology that doesn't fit exactly can lead to more confusion.
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.
You don't need to set every bar line manually
That's clear, but you also set at least the first. And it is about the alignment of the bars.
So I think my proposal is not completely wrong.
Is the an other way to relate "beats_set_marker" to bars?
Maybe "beats_start_new_bars" or something.
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 think it makes sense to keep the clear distinction between markers and bars/downbeat as @Holzhaus rightfully pointed out.
There is is also the fact that marker define a new grid, so in the case where the marker is set in between two beats, the new grid will start of tempo compare to the previous grid. As a reminder, this is a requirement to deal with tracks that have variable tempo or may shift by a bit fraction in the track.
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 OK, in is just a new grid start bar line. I thought this sets a bar line and then other bar lines are interpolated form that.
The essence of my first comment is that "beats_set_marker" reads as if you set a beat marker which is just the vertical line we had always marking beats. I think this is a real issue and needs to be solved.
proposals:
"beats_set_bar_change"
"beat_signature_change"
"beats_set_bars_start"
"beats_mark_bar_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.
The visuals are implications here.
Right, here we are looking to the CO interface.
I'm interested in the future plans to align our requirements.
As I have understood with the new "beats_set_change_marker" CO we can adjust either adjust tempo slightly, or mark a signature change which is a double bar line on a musical sheet.
I consider this to be sufficient for all use cases, in a way that all change markers shall be visible on the waveform. But I like to confirm it with you to avoid future discussions. (Compared to an additional interface which allows to move single beats)
We have two use cases regarding tempo changes:
- A const beat grid which aligns at the start and end but is off in the middle.
- User can find the tempo change positions, preferably pick a downbeat not on the grid and mark it using the new control. The beast before and after, schall be adjusted to hit this new beat marked with e.g a double bar line.
- A non const beat grid where the beats are more likely on time but with issues making "tshank" instead of "boom". The latest detector is able to group beats with the same tempo. I consider it very much desired to not change tempo in every beat to avoid youling in followers.
- User may want to explicitly mark tempo changes and combine regions if similar tempo to one region. After finishing, it is probably desired to be in the same state as 1. User should also be able to align change markers to downbeats. Technical he is free to mark every beat, but that should be an exception.
Can you confirm that?
I think Mixxx should make it hard to create a padding bar, that does not match the previous and not the follower bars. In case users really wants it, they can mark the start of this padding bar as well.
What do you think?
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 guess we could make the marker align on the previously set grid if quantize is on. IMO that makes sense indeed to guide users to keep, their beatgrid as aligned as possible.
Could we please address that as a follow up PR tho? I really would like to get this merge ASAP to set the base for future work related to beatgrid.
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 don't have demands for additional changes here, so that's fine. I just like to confirm that we are on the right track. @Holzhaus, can you confirm that as well?
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 have pushed the CO update. Are we happy to move forward now?
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.
Yes, thank you.
d31af27
to
34ee683
Compare
Rebased and ready to merge! |
Co-authored-by: Jan Holthuis <[email protected]>
34ee683
to
9de4d5c
Compare
@daschuer you still have changes-requested marked, are all your notes addressed? |
A manual test is pending. I hope I will find time this weekend. Did other reviewers testing regarding the new beat format? |
This PR is based on @fwcd 's PR #12343, itself based on @Holzhaus 's #4489 PR.
Kooha-2024-06-07-18-43-20.mp4
This attempt TL;DR; is: do not disrupt the way the beat grid currently works
This is how this attempt differs from the previous one:
TODO:
Fixes #13308 and #10164
Disclaimer:
I know there was a lot of discussion on these types of change in each of the above PRs as well as in the Zulip thread that goes with it. I'm sorry, but I only read the latest messages of each as there was a few hundreds of them, going over a few years of activity. Apologies if this proposal contains approaches that have been discussed and ruled out already.