Render bar and phrase marks and a menu option to enable / disable #2186
Conversation
Fix beatgrid function to comply with tests in case of uneven bpm
src/track/beatgrid.h
Outdated
| @@ -89,6 +93,8 @@ class BeatGrid final : public Beats { | |||
| mixxx::track::io::BeatGrid m_grid; | |||
| // The length of a beat in samples | |||
| double m_dBeatLength; | |||
| const int c_beatsPerBar = 4; | |||
There was a problem hiding this comment.
Define constexpr constants for these default values instead of using const members. As long as the constants do not need to be publicly visible enclose them in an anonymous namespace in the .cpp file.
There was a problem hiding this comment.
IMHO this should be a control object. This would make it possible to modify them via controller buttons (maybe PARAMETER +/- ?).
But if this is too much work we can still merge this as is and add that later.
There was a problem hiding this comment.
I like the suggestion but I would like to include that in a later PR if you don't mind.
|
Nice, thanks for your work!
I'd rather have a control object which you can set to |
|
Welcome, Simon, and thank you for starting to contribute! Sorry, I didn't realize that this was your first PR for Mixxx. Would you please sign our Contributor Agreement? Otherwise we would not be allowed to distribute your code. |
|
@daschuer I like the idea to open a separate PR for the critical changes to findNthBeat, see linked PR |
Some formatting according to PR comments
|
I appreciate your review a lot, sorry for not coming back earlier. |
|
Hi @sharst, thank you very much for working on this. I looked through the code briefly and it looks like there are some good first steps here, but I am concerned the approach you are taking with this code will not lead to a very useful result. Simply setting a single point as the beginning of the first phrase and assuming the number of bars per phrase for the entire track will likely generate so much incorrect information that showing the phrase markers will be more distracting than helpful. The user needs to be able to both add and delete phrase markers at any beat anywhere in the track. Also, I am afraid that continuing to add features to the BeatGrid class which assumes a constant tempo over the entire track is digging deeper into a hole. I assumed you were already aware, but if not we have had many prior discussions about this topic on Zulip: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Rendering.20Bar.20.26.20Phrase.20Marks I highly recommend you read through all of those before continuing with this code. The consensus we reached toward the end of that first thread I linked is that we should unify BeatGrid and BeatMap into a single class (this can be the Beats class which is currently only an abstract interface) that is flexible enough to handle any music. This could work like a BeatMap of bars. Each bar could be like a BeatGrid where we assume a constant tempo, meter, and key, with the user able to set exactly where each bar begins (similar to how it works in Serato as demonstrated in this video). @JaviVilarroig mentioned that he intended to start working on this new class but I do not know how far he got. @JaviVilarroig, if you have started working on any code, could you open a work-in-progress pull request, even if the code is very rough and incomplete? Maybe @sharst can continue working on that. I hope you're with us for a long haul @sharst because this is going to be a big project. :) |
|
Hi. I have a solid work done in specking the changes needed. I have done some work on the code but not too much and has been on hold for some weeks due to holidays :). Not having enough to post a PR. |
Hmm, I think it may need to be a bit more complex than that. Assuming a constant tempo over a whole bar may not work very well when the tempo is accelerating or decelerating. |
|
But maybe it is close enough to the truth ( in favor of a simple model) It is anyway the question what will be the musical value of a constant increasing speed compared to other tracks. For example if we make a four beat loop, and reduce it to two our one beat, we will introduce a temp change, which I can imagine is not really desired. Do we have a reference track? Zorabs the Greek ... |
I think what would be most important in that case is that the boundaries of the loop are at the correct times. In a 4 beat bar that is accelerating, if the loop size is halved, the end of the new loop must be before the start of beat 3. |
|
Finally the datamodel should not limit us. We can do the same and allow accel and rtar measures. Just two additional flags. What do you think? |
|
Perhaps we could have a beginning and end tempo for each bar. We could either calculate the beat positions from that (I'm not sure how that math would work, but I think it would be possible) or store individual beat positions. On further thought, we don't need two tempos saved per bar; we can use the following bar's tempo as the end tempo. |
|
Any news on this? Instead of creeping in more and more considerations, I would like to finally have these markers in the software. Mixxx currently only works well with 4/4 anyways, so support for other measures is another beast imo. |
I'm working on it. Right now I'm working on removing BeatGrid and make BeatMap the only Beats class. Is not trivial because I have found parts of the code that assume you're using a BeatGrid, so I have to modify this other classes and make sure the unit tests mach them and work properly. But don't worry, this PR is not dead. Have you tried the current PR? it already works with a fixed 4/4 time signature. The only drawback is that it only works if your track has a BeatMap. BeatGrids sown nothing. |
|
Whats the difference between BeatMap and BeatGrid? |
BeatGrid stores only the position of the first beat in the track and calculates the remaining ones assuming constant tempo. In order to have BeatMaps attached to your track you must analyze them after deactivating "Assume constant tempo" in the preferences. |
|
I think this PR has been superseededby #2961. Shall we close this? |
This is a follow up to #1918 .
Basically this gives another menu option to enable highlighting of beat and phrase marks. Beat marks are painted in color, phrase marks get an extra marker with the phrase number.
Currently the number of bars per beat and the number of beats per bar are constants in the beatgrid class, but they could be pretty easily made configurable.
The beginning of the first beat can be set by aligning the beatgrid. Aligning the grid will assume that you are currently on a phrase start and calculate back the amount of preceding phrases accordingly (e.g. when you are on the 32rd beat and click on "align beatgrid", it will assume you are on the 3rd phrase and mark all phrases accordingly.
The location of the first phrase beginning within the track is saved away with the beatgrid.
One additional issue came up during this work: When using the findNextBeat-function (and associated functions), I noticed that the returned sample is not always a multitude of the beat length in samples, which is what I'd have expected and what the respective tests also seem to assume. This is particularly true for beatgrids with non-integer bpm.
I adapted the function to respect this again and adapted the test to be checking for an uneven bpm. This could break some other functions though.