Sync: Use mixxx::Bpm class #4071
Conversation
Holzhaus
force-pushed
the
sync-bpm-refactor
branch
2 times, most recently
from
5088610
to
8faf04d
Compare
|
Except for the leader renaming there were barely any merge conflicts, and all of them were trivial. I think this is ready to merge. |
Pull Request Test Coverage Report for Build 1105872166
💛 - Coveralls |
Use `mixxx::audio::SampleRate` instead of `int` in the `EngineMaster`, `EngineSync` and `InternalClock` code. The type was not applied to the channel mixer and effects code to reduce potential merge conflicts with mixxxdj#1966 and mixxxdj#2618. Based on PR mixxxdj#4071.
Holzhaus
force-pushed
the
sync-bpm-refactor
branch
from
8faf04d
to
e99bf9d
Compare
Use `mixxx::audio::SampleRate` instead of `int` in the `EngineMaster`, `EngineSync` and `InternalClock` code. The type was not applied to the channel mixer and effects code to reduce potential merge conflicts with mixxxdj#1966 and mixxxdj#2618. Based on PR mixxxdj#4071.
| @@ -12,16 +12,17 @@ | |||
|
|
|||
| namespace { | |||
| const mixxx::Logger kLogger("InternalClock"); | |||
| constexpr mixxx::Bpm kDefaultBpm(124.0); | |||
There was a problem hiding this comment.
we've talked about having a mixxx-wide default bpm, have we established one yet? (not needed for this PR)
There was a problem hiding this comment.
I'm not aware we have. We can probably add it later on.
| mixxx::Bpm SyncControl::getBaseBpm() const { | ||
| const mixxx::Bpm bpm = getLocalBpm(); | ||
| if (!bpm.isValid()) { | ||
| return {}; |
There was a problem hiding this comment.
does {} result in an invalid bpm getting returned? It would be clearer to explicitly return kInvalidBpm.
There was a problem hiding this comment.
Yes, this returns a default constructed Bpm which is invalid (it's the same as mixxx::Bpm(), but repeating the return type is not that nice).
There is no kInvalidBpm. We could add it if you think itvs necessary.
| << bpm << "/" << m_leaderBpmAdjustFactor; | ||
| } | ||
| if (!bpm.isValid()) { | ||
| return {}; |
There was a problem hiding this comment.
as elsewhere, rather than returning "empty" I would prefer to explicitly return the Invalid value
There was a problem hiding this comment.
That would require to introduce a named constant for default constructed instances of Bpm.
src/engine/sync/synccontrol.cpp
Outdated
| @@ -484,11 +491,14 @@ void SyncControl::updateAudible() { | |||
| } | |||
|
|
|||
| void SyncControl::slotRateChanged() { | |||
| double bpm = m_pLocalBpm ? m_pLocalBpm->get() * m_pRateRatio->get() : 0.0; | |||
| mixxx::Bpm bpm = getLocalBpm(); | |||
| if (bpm.isValid()) { | |||
There was a problem hiding this comment.
it looks like it would be simpler to early-return if bpm is invalid.
There was a problem hiding this comment.
Unsure if the rate ratio could be NaN/infinity. Is this a read-only CO? Otherwise Bpm may become invalid due to the multiplication. I restructured the code a bit.
|
Already approved by two of use, will merge now. LGTM |
Use `mixxx::audio::SampleRate` instead of `int` in the `EngineMaster`, `EngineSync` and `InternalClock` code. The type was not applied to the channel mixer and effects code to reduce potential merge conflicts with mixxxdj#1966 and mixxxdj#2618. Based on PR mixxxdj#4071.
Use `mixxx::audio::SampleRate` instead of `int` in the `EngineMaster`, `EngineSync` and `InternalClock` code. The type was not applied to the channel mixer and effects code to reduce potential merge conflicts with mixxxdj#1966 and mixxxdj#2618. Based on PR mixxxdj#4071.
Marked as draft until @ywwg sync PRs are merged.