Lock BPM updates #3012
Lock BPM updates #3012
Conversation
|
What exactly does the BPM lock do? Is is a way to lock the beatgrid? Or does it prevent the BPM sliders from being used? |
|
It's the normal lock bpm function from the library and prevents the analyzer from changing the beatgrid. When I have fixed the beatgrid alignment I usually try to lock it. Having this mapped on the controller makes my life easier, seeing if the lock is enabled on the deck helps me not to forget to check it before mixing it in. |
|
Ok, then we should probably pick a more fitting name, like "beatgrid_locked" and also rename the existing function. What do you think? |
|
Ping. Still unresolved review comments. |
|
@Holzhaus I renamed the new everything to beatgrid lock or appropriate variations. I polished the whole dialog. There is still one old bug left I can't figure out. In the BPM tab, everything has Alt+Key shortcuts. |
Works here. Do you have other shortcuts set in a custom keyboard mapping? |
|
I incorporated the suggestions. The remaining comments where deliberate design decisions |
|
please merge |
|
Please enable the pre-commit hooks. I will not review any new PRs as long as the pre-commit build fails. |
|
@poelzi Explicitly requesting to merge while the CI builds are obviously failing doesn't speed up the process. |
|
@daschuer All CI builds are failing! |
|
@uklotzde sorry, I somehow missed the introduction of the pre-commit hook. I fixed my setup and squished all commits together so the hooks are run correctly and formatting of all files is nice. |
|
There are some merge conflicts. |
|
Can you remember which track you modify? Where they analyzed with the fixed analyzer? |
There was a problem hiding this comment.
Thank you. This is now in a merge-able state.
To issue that can be improved: We have no two entries for the lock in the context menu.
It would be nice to have one with a checkbox. This requires to gray out the other entries on the fly with is some extra work.
In case the bpm is locked we may hide the beat grid edit tools.
@uklotzde: Are your issues fixed?
|
I have skimmed through my tracks, but I have not yet found a one where the analyzer rounding not works. I am really interested in your samples to have a chance to improve the analyzer. For my understanding these remaining Tracks must be one with two different beat instruments, where Mixxx is able to find a non integer beat that is not more than 20 ms of any of those beats. Can you confirm that? How about add the rounding feature into the beat_adjust_slower/beat_adjust_faster controls? This can also snap to all fractionals, that are useful for multiplying BPM later. For my understanding this covers both of our use cases:
|
|
I have implemented that in #3790 |
|
@daschuer https://sanatonrecords.bandcamp.com/track/already-maged-circle-dance-of-cold-constellations detected as 151.03 for example. |
|
https://badgersrecords.bandcamp.com/track/auraka-creeping-inspiration-148-bpm
|
|
Can we make a wiki page for those analyzer test tracks, so that they don't get lost? |
|
We have: https://github.com/mixxxdj/mixxx/wiki/Iconic-Tracks but I must admit I have not updated frequently. |
|
I have just checked How do you get: |
|
https://sanatonrecords.bandcamp.com/track/procs-frigolitpuffens-magiska-trampdyna is exactly 140.000 BPM here. https://sanatonrecords.bandcamp.com/track/already-maged-circle-dance-of-cold-constellations is here 151,01485970 BPM |
|
@daschuer maybe a compiler difference: [----------] Global test environment tear-down (TrackExporter failures are from merged export branch) |
|
I have testes the "creeping-inspiration" with various Machines:
So there must be an imprecise issue on Fedora and NixOs. |
|
I have just tested the mp3 version with Fedora 34 and It works correctly. So only the FLAC version is affected. This is the mp3 track output: And this the flac track output both versions are starting with the same beats when using non const and shift apart at 39 s An Ubuntu Focal I have exactly the same log: mp3 Flac And judged by eye the same non const beat grid with the same shifting point between mp3 and flac at 39 s. I am curious where the compiler sensitive part is hidden. |
|
@uklotzde I have now build Mixxx with the instructions on our wiki, and the build is working. It is using gcc and based on these: https://github.com/rpmfusion/mixxx/blob/f34/mixxx.spec but I am not able to reproduce the issue that happens with the rpmfusion version. Do I miss something? |
Last f34 builds: https://koji.rpmfusion.org/koji/buildinfo?buildID=18678 Logs x86_64 build.log: https://koji.rpmfusion.org/kojifiles/packages/mixxx/2.3.0/0.20.beta.20210322git25f342e.fc34/data/logs/x86_64/build.log |
|
Strange, I have just build the exact same commit with clang and gcc, but I cannot reproduce the issue with these. Only the rpmfusion version is affected. It is 2.3.0-beta (build beta r20210322git25f342e) |
|
Floating-point rounding may depend on register allocations and optimization settings, i.e. internal 80-bit (long double) vs. 64-bit (double). https://en.wikipedia.org/wiki/Extended_precision#IEEE_754_extended_precision_formats |
|
The behavior might differ depending on compiler versions. It's floating-point, just deal with it. I experienced this behavior >20 years ago when writing rasterization algorithms in C. |
|
This can only hardly explained with a double optimization issue. I have the impression there is something more serious going on. Now I cannot reproduce the issue even with the RpmFusion version. To sort out updates, I have cross checked with a fresh installation and and there the issue is gone as well. |
|
It looks like we are hunting a phantom. The not updated Fedora version has mixxx build beta r20210126gitf009e06 which has not yet the ironing pr merged. I think that was the original state in my other Fedora reported in #3012 (comment). Than it was auto updated and now it has the ironing PR so I cannot reproduce the issue. @poelzi: Can you still reproduce the issue if you build the current 2.3 branch with clang? |
|
@daschuer I merged main, and this is mergeable now. |
|
Thank you, @uklotzde merge? |
| // called from an engine worker thread | ||
| void BpmControl::trackUnloaded(TrackPointer pOldTrack) { | ||
| if (pOldTrack) { | ||
| disconnect( |
There was a problem hiding this comment.
pOldTrack->disconnect(this)
| m_value = std::round(m_value); | ||
| } | ||
|
|
||
| QString displayString() { |
| static double valueFromString(const QString& str, bool* pValid = nullptr); | ||
| static QString valueToString(double value); | ||
| static int valueToInteger(double value) { | ||
| return static_cast<int>(std::round(value)); | ||
| } | ||
|
|
||
| /// Returns a string depending on non zero decimal placess. |
| /// Returns a string depending on non zero decimal placess. | ||
| /// If the value is round enough, use 1 decimal places, otherwise 2 | ||
| /// @param {value} bpm value | ||
| static QString displayString(double value) { |
There was a problem hiding this comment.
Using the same name for both a static and a non-static function is confusing.
| /// If the value is round enough, use 1 decimal places, otherwise 2 | ||
| /// @param {value} bpm value | ||
| static QString displayString(double value) { | ||
| if (fabs(round(value * 10) / 10 - value) < 0.001) { |
There was a problem hiding this comment.
Calculate the precision beforehand and then use a single, parameterized QString formatting expression. To reduce redundancy.
| // unlock the bpmLock in case force is requested | ||
| if (forceBpm) { | ||
| wasLocked = isBpmLocked(); | ||
| setBpmLocked(false); |
There was a problem hiding this comment.
Invoking mutating, public functions of Track while the object is locked is forbidden! This could result in deadlocks when emitting signals.
| @@ -290,6 +299,10 @@ bool Track::replaceRecord( | |||
| bpmUpdatedFlag = trySetBpmWhileLocked( | |||
| newRecord.getMetadata().getTrackInfo().getBpm().value()); | |||
| } | |||
|
|
|||
| if (forceBpm) { | |||
| setBpmLocked(wasLocked); | |||
There was a problem hiding this comment.
Again, this is not permissible. The object is in an inconsistent state and locked.
| @@ -515,48 +555,92 @@ void DlgTrackInfo::clear() { | |||
| m_pBeatsClone.clear(); | |||
|
|
|||
| txtLocation->setText(""); | |||
| txtBitrate->setText(""); | |||
There was a problem hiding this comment.
Why is this needed? Doesn't resetTrackRecord() reset all those fields implicitly? We should check that.
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
This patchset fixes some long time annoyances of mine.
[ChannelN],beatgrid_lock