Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[POC] Beats: Add editing controls and show downbeats on scrolling waveforms #4489

Closed
wants to merge 8 commits into from
Closed

[POC] Beats: Add editing controls and show downbeats on scrolling waveforms #4489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
40ddcda
Beats: Add initial read-only support for downbeats
Holzhaus Oct 26, 2021
3403263
Beats: Add method to get the iterator to a beat close to a position
Holzhaus Nov 9, 2021
ebde151
BpmControl: Add Beats editing controls
Holzhaus Nov 7, 2021
c685809
WaveformRenderBeat: Add support for rendering downbeats
Holzhaus Oct 26, 2021
7348268
LateNight: Set downbeat color in skin.xml
Holzhaus Oct 26, 2021
5ab0bde
WaveformRenderBeat: Pass QVector directly to `drawLines()`
Holzhaus Oct 26, 2021
0b15e33
Show markers on waveforms [WIP]
Holzhaus Nov 5, 2021
f0faad9
LateNight: Add buttons for setting/removing beats markers [WIP]
Holzhaus Nov 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions res/skins/LateNight/palemoon/buttons/btn__beats_remove_marker.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 2 additions & 0 deletions res/skins/LateNight/palemoon/buttons/btn__beats_remove_marker_active.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
0 ...lemoon/buttons/btn__beat_curpos_large.svg → ...alemoon/buttons/btn__beats_set_marker.svg
View file
Open in desktop
File renamed without changes
0 ...buttons/btn__beat_curpos_large_active.svg → .../buttons/btn__beats_set_marker_active.svg
View file
Open in desktop
File renamed without changes
2 changes: 2 additions & 0 deletions res/skins/LateNight/skin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@

<SetVariable name="AxesColor">#999</SetVariable>
<SetVariable name="BeatColor">#999</SetVariable>
<SetVariable name="DownbeatColor">#f00</SetVariable>
<SetVariable name="MarkerbeatColor">#f0f</SetVariable>
<SetVariable name="PlayPosColor">#00c6ff</SetVariable>
<SetVariable name="CueColor">#ff7a01</SetVariable>
<SetVariable name="LoopColor">#00b400</SetVariable>
Expand Down
14 changes: 10 additions & 4 deletions res/skins/LateNight/style_palemoon.qss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2309,11 +2309,17 @@ WPushButton#PlayDeck[value="0"] {
image: url(skin:/palemoon/buttons/btn__beatgrid_controls_collapse.svg) no-repeat center center;
}

#BeatCurposLarge[displayValue="0"] {
image: url(skin:/palemoon/buttons/btn__beat_curpos_large.svg) no-repeat center center;
#BeatsSetMarker[displayValue="0"] {
image: url(skin:/palemoon/buttons/btn__beats_set_marker.svg) no-repeat center center;
}
#BeatCurposLarge[pressed="true"] {
image: url(skin:/palemoon/buttons/btn__beat_curpos_large_active.svg) no-repeat center center;
#BeatsSetMarker[pressed="true"] {
image: url(skin:/palemoon/buttons/btn__beats_set_marker_active.svg) no-repeat center center;
}
#BeatsRemoveMarker[displayValue="0"] {
image: url(skin:/palemoon/buttons/btn__beats_remove_marker.svg) no-repeat center center;
}
#BeatsRemoveMarker[pressed="true"] {
image: url(skin:/palemoon/buttons/btn__beats_remove_marker_active.svg) no-repeat center center;
}

#BeatsEarlier {
Expand Down
29 changes: 20 additions & 9 deletions res/skins/LateNight/waveform.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
<SignalRGBMidColor><Variable name="SignalMidColor"/></SignalRGBMidColor>
<SignalRGBLowColor><Variable name="SignalLowColor"/></SignalRGBLowColor>
<BeatColor><Variable name="BeatColor"/></BeatColor>
<DownbeatColor><Variable name="DownbeatColor"/></DownbeatColor>
<MarkerbeatColor><Variable name="MarkerbeatColor"/></MarkerbeatColor>
<AxesColor><Variable name="AxesColor"/></AxesColor>
<BeatHighlightColor></BeatHighlightColor>
<PlayPosColor><Variable name="PlayPosColor"/></PlayPosColor>
Expand Down Expand Up @@ -147,15 +149,24 @@

<WidgetGroup><Size>1f,0min</Size></WidgetGroup>

<!-- CurPos -->
<Template src="skin:/controls/button_1state_right.xml">
<SetVariable name="TooltipId">beats_translate_curpos</SetVariable>
<SetVariable name="ObjectName">BeatCurposLarge</SetVariable>
<SetVariable name="Size">26f,52f</SetVariable>
<SetVariable name="BtnSize">library_tall</SetVariable>
<SetVariable name="ConfigKey"><Variable name="Group"/>,beats_translate_curpos</SetVariable>
<SetVariable name="ConfigKeyRight"><Variable name="Group"/>,beats_translate_match_alignment</SetVariable>
</Template>
<WidgetGroup><!-- beats set & remove marker -->
<Layout>vertical</Layout>
<Size>26f,52f</Size>
<Children>
<Template src="skin:/controls/button_1state.xml">
<SetVariable name="TooltipId">beats_set_marker</SetVariable>
<SetVariable name="ObjectName">BeatsSetMarker</SetVariable>
<SetVariable name="Size">26f,26f</SetVariable>
<SetVariable name="ConfigKey"><Variable name="Group"/>,beats_set_marker</SetVariable>
</Template>
<Template src="skin:/controls/button_1state.xml">
<SetVariable name="TooltipId">beats_remove_marker</SetVariable>
<SetVariable name="ObjectName">BeatsRemoveMarker</SetVariable>
<SetVariable name="Size">26f,26f</SetVariable>
<SetVariable name="ConfigKey"><Variable name="Group"/>,beats_remove_marker</SetVariable>
</Template>
</Children>
</WidgetGroup><!-- /beats set & remove marker -->

<WidgetGroup><!-- beats earlier & faster -->
<Layout>vertical</Layout>
Expand Down
93 changes: 74 additions & 19 deletions src/engine/controls/bpmcontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ constexpr double kBpmRangeMax = 200.0;
constexpr double kBpmRangeStep = 1.0;
constexpr double kBpmRangeSmallStep = 0.1;

constexpr double kBpmAdjustMin = kBpmRangeMin;
constexpr double kBpmAdjustStep = 0.01;
constexpr double kBpmTabRounding = 1 / 12.0;

// Maximum allowed interval between beats (calculated from kBpmTapMin).
Expand Down Expand Up @@ -88,6 +86,18 @@ BpmControl::BpmControl(const QString& group,
connect(m_pTranslateBeatsLater, &ControlObject::valueChanged,
this, &BpmControl::slotTranslateBeatsLater,
Qt::DirectConnection);
m_pBeatsSetMarker = new ControlPushButton(ConfigKey(group, "beats_set_marker"), false);
connect(m_pBeatsSetMarker,
&ControlObject::valueChanged,
this,
&BpmControl::slotBeatsSetMarker,
Qt::DirectConnection);
m_pBeatsRemoveMarker = new ControlPushButton(ConfigKey(group, "beats_remove_marker"), false);
connect(m_pBeatsRemoveMarker,
&ControlObject::valueChanged,
this,
&BpmControl::slotBeatsRemoveMarker,
Qt::DirectConnection);

// Pick a wide range (kBpmRangeMin to kBpmRangeMax) and allow out of bounds sets. This lets you
// map a soft-takeover MIDI knob to the BPM. This also creates bpm_up and
Expand Down Expand Up @@ -139,6 +149,8 @@ BpmControl::~BpmControl() {
delete m_pBeatsTranslateMatchAlignment;
delete m_pTranslateBeatsEarlier;
delete m_pTranslateBeatsLater;
delete m_pBeatsSetMarker;
delete m_pBeatsRemoveMarker;
delete m_pAdjustBeatsFaster;
delete m_pAdjustBeatsSlower;
}
Expand All @@ -147,7 +159,11 @@ mixxx::Bpm BpmControl::getBpm() const {
return mixxx::Bpm(m_pEngineBpm->get());
}

void BpmControl::adjustBeatsBpm(double deltaBpm) {
void BpmControl::slotAdjustBeatsFaster(double v) {
if (v <= 0) {
return;
}

const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
Expand All @@ -157,31 +173,32 @@ void BpmControl::adjustBeatsBpm(double deltaBpm) {
return;
}

const mixxx::Bpm bpm = pBeats->getBpmInRange(
mixxx::audio::kStartFramePos, frameInfo().trackEndPosition);
// FIXME: calling bpm.value() without checking bpm.isValid()
const auto centerBpm = mixxx::Bpm(math_max(kBpmAdjustMin, bpm.value() + deltaBpm));
mixxx::Bpm adjustedBpm = BeatUtils::roundBpmWithinRange(
centerBpm - kBpmAdjustStep / 2, centerBpm, centerBpm + kBpmAdjustStep / 2);
const auto newBeats = pBeats->trySetBpm(adjustedBpm);
if (!newBeats) {
return;
const auto adjustedBeats = pBeats->tryAdjustTempo(
frameInfo().currentPosition, mixxx::Beats::TempoAdjustment::Faster);
if (adjustedBeats) {
pTrack->trySetBeats(*adjustedBeats);
}
pTrack->trySetBeats(*newBeats);
}

void BpmControl::slotAdjustBeatsFaster(double v) {
void BpmControl::slotAdjustBeatsSlower(double v) {
if (v <= 0) {
return;
}
adjustBeatsBpm(kBpmAdjustStep);
}

void BpmControl::slotAdjustBeatsSlower(double v) {
if (v <= 0) {
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}
adjustBeatsBpm(-kBpmAdjustStep);

const auto adjustedBeats = pBeats->tryAdjustTempo(
frameInfo().currentPosition, mixxx::Beats::TempoAdjustment::Slower);
if (adjustedBeats) {
pTrack->trySetBeats(*adjustedBeats);
}
}

void BpmControl::slotTranslateBeatsEarlier(double v) {
Expand Down Expand Up @@ -223,6 +240,44 @@ void BpmControl::slotTranslateBeatsLater(double v) {
}
}

void BpmControl::slotBeatsSetMarker(double v) {
if (v <= 0) {
return;
}
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

const auto modifiedBeats = pBeats->trySetMarker(frameInfo().currentPosition);
if (modifiedBeats) {
pTrack->trySetBeats(*modifiedBeats);
}
}

void BpmControl::slotBeatsRemoveMarker(double v) {
if (v <= 0) {
return;
}
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

const auto modifiedBeats = pBeats->tryRemoveMarker(frameInfo().currentPosition);
if (modifiedBeats) {
pTrack->trySetBeats(*modifiedBeats);
}
}

void BpmControl::slotBpmTap(double v) {
if (v > 0) {
m_tapFilter.tap();
Expand Down
4 changes: 4 additions & 0 deletions src/engine/controls/bpmcontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ class BpmControl : public EngineControl {
void slotAdjustBeatsSlower(double);
void slotTranslateBeatsEarlier(double);
void slotTranslateBeatsLater(double);
void slotBeatsSetMarker(double);
void slotBeatsRemoveMarker(double);
void slotTapFilter(double,int);
void slotBpmTap(double);
void slotUpdateRateSlider(double v = 0.0);
Expand Down Expand Up @@ -144,6 +146,8 @@ class BpmControl : public EngineControl {
ControlPushButton* m_pAdjustBeatsSlower;
ControlPushButton* m_pTranslateBeatsEarlier;
ControlPushButton* m_pTranslateBeatsLater;
ControlPushButton* m_pBeatsSetMarker;
ControlPushButton* m_pBeatsRemoveMarker;

// The current effective BPM of the engine
ControlLinPotmeter* m_pEngineBpm;
Expand Down
14 changes: 12 additions & 2 deletions src/skin/legacy/tooltips.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,13 +391,23 @@ void Tooltips::addStandardTooltips() {
<< tr("BPM Tap")
<< tr("When tapped repeatedly, adjusts the BPM to match the tapped BPM.");

add("beats_set_marker")
<< tr("Set Beat Marker")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It his actually "Set Bar" Or "Set Down-Beat"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both, but that also doesn't quite fit either. I'm still contemplating if another name would be better, e.g. "Set Tempo Region Marker" or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the cases where this is not a downbeat?

I can imagine if one does not bother with downbeats. We may consider to express that as beatsPerBar = 0.

Do you have something else In mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like "Add Tempo Marker". "set" implies there is only one, and if I'm not mistaken we can have more than one of these, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there can be multiple tempo markers. The distance between 2 markers needs to be at least 1 bar (= 4 beats currently). If you try to place a tempo marker very close to another on (within a range of 2 beats), the editing UI is smart enough to move the nearby marker instead of inserting one (which would double the BPM between the 2 markers, which is very likely undesired). So "Add" would be misleading IMHO.

<< tr("Set a beat marker at the current play position.");

add("beats_remove_marker")
<< tr("Remove Beat Marker")
<< tr("Remove the beat marker at the current play position.");

add("beats_adjust_slower")
<< tr("Adjust BPM Down")
<< tr("When tapped, adjusts the average BPM down by a small amount.");
<< tr("When tapped, decrease the BPM of the region around the "
"current play position by a small amount.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation would be:

Suggested change
"current play position by a small amount.");
"current play position by a small amount and shift the following regions accordingly.");

is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it just increases/decreases the beatsTillNextMarker value by a whole bar, but the marker positions stay the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have written the comments before I have discovered that.

We need to make a usability check if this works out.

When would one use this feature?
I original had something like this in mind, but I am afraid it is not (yet) possible:

1.) Set the first bar
2.) Move in the region after the bar, adjust the tempo.
3.) Play the track and verify the beats
4.) Identify a bar with an offset
5.) Adjust the bar by implicit setting a marker.
6.) Continue at 2.

Later you may recognize that there is a beat or bar rollover in the a region.
In case we assume that the following marker is set correct. You current approach works.

In case the user has messed up the beat grids and likes to start over from a certain position, the gentle tempo adjusting inclusive shifting all later beats might be desired.


add("beats_adjust_faster")
<< tr("Adjust BPM Up")
<< tr("When tapped, adjusts the average BPM up by a small amount.");
<< tr("When tapped, increases the BPM of the region around the "
"current play position by a small amount.");

add("beats_translate_earlier")
<< tr("Adjust Beats Earlier")
Expand Down
Loading