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.

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

Bug #1128005 - Emphasize the downbeats of a track by a separate marker on the waveform #1529

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 28, 2018

This PR attempts a simple solution to bug #1128005. It introduces measure (downbeat, every 4 beats) and phrase (16 beats) markers as part of the Waveform view. The location of these markers will eventually be determined by the Cue or a new Cue type, especially designated for beat grid anchoring. The main purpose of this feature is to facilitate better beat-matching.

The implementation plan for this feature is:

  1. Add measure and phrase markers (every 4 and 16 beats, number of beats and color hard coded), the first marker is the same as the first beat of the track
  2. Make the number of beats per measure/phrase and colors part of Preferences/mixxx.cfg
  3. Change the logic so CUE (Cue::LOAD) ) determines the first beat
  4. Add a new cue type (beat grid anchor or first beat) to serve the same role, restore CUE functionality

mixxx_beatgrid_downbeat2
mixxx_beatgrid_downbeat3

@ritschwumm
Copy link

just a few works about my experiences with using something similar:

4 measures per phrase works quite well for many tracks, but there are a surprising number of them where the scheme shifts by 2 measures somewhere in the middle. quite irritating, when you're mixing.
whenever this happens, i wish i could either tell my app such a shift happenend, or at least use 2 measures per phrase for this specific track.

@ghost
Copy link
Author

ghost commented Feb 28, 2018

You are 100% right, this mirrors my experience as well. However, with this one, I'd like to take more gradual approach first (with the simples steps outlined in the overview). Hopefully at some point we can introduce multiple "beat cues" that can also be used for:
1.tempo changes (instant or gradual)
2.meter changes (4/4, 3/4, etc)
3.Phrase length changes

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, it is a good start.
But let's hold it up until we can handle other than 4/4 measure.

@@ -14,6 +14,11 @@
WaveformRenderBeat::WaveformRenderBeat(WaveformWidgetRenderer* waveformWidgetRenderer)
: WaveformRendererAbstract(waveformWidgetRenderer) {
m_beats.resize(128);

// TODO(ntmusic): Read from settings
m_measureSize = 4;
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be a settings option, since this varies by tracks and also inside tracks.


// TODO(ntmusic): Read from settings
m_measureSize = 4;
m_phraseSize = 16;
Copy link
Member

Choose a reason for hiding this comment

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

The same here, this should be read from the beatgrid itself. It cannot be a fixed value.

The input of these values into the beatgrid can follow these fixed width by default.

// TODO(ntmusic): Read from settings
m_measureSize = 4;
m_phraseSize = 16;
m_phraseColor.setRgb(255,0,0);
Copy link
Member

Choose a reason for hiding this comment

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

This need to be one a skin option.
Skins can already colour the waveforms.

@@ -67,8 +72,7 @@ void WaveformRenderBeat::draw(QPainter* painter, QPaintEvent* /*event*/) {
painter->setRenderHint(QPainter::Antialiasing);

QPen beatPen(m_beatColor);
beatPen.setWidthF(std::max(1.0, scaleFactor()));
painter->setPen(beatPen);
beatPen.setWidthF(std::max(0.5, scaleFactor()));
Copy link
Member

Choose a reason for hiding this comment

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

Can we see a all beat markers at 50 % with a halve pixel width?

// If we don't have enough space, double the size.
if (beatCount >= m_beats.size()) {
m_beats.resize(m_beats.size() * 2);
}

if(beatIndex >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic needs to be moved at least to the
Iterator.
I suggest a isBar() and isPhraseStart()
A phrase does not necessarily strat with a bar.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, it is a good start.
But let's hold it up until we can handle other than 4/4 measure.

@cgrinham
Copy link

I use Mixxx mainly for funk & disco, you'd be surprised how many weird things go on in these tracks. This is a really nice idea but definitely needs more work. Good job so far. Even if it was something like "section markers", or just various types of markers other than Cues, that might be helpful!

@Be-ing Be-ing added this to the 2.2.0 milestone Mar 8, 2018
@ghost
Copy link
Author

ghost commented Mar 10, 2018

The requested changes are quite easy to add, however I am starting to think we should have Time Signature (2/4, 3/4, 4/4, etc.) added as part of the Track class first.

@daschuer
Copy link
Member

Yes, right, this should be available in the library column as well. Be aware that we will have the same issue like BPM and key: We can only show one significant (the first?) value in the library, while the actual value may change during the track.

@daschuer
Copy link
Member

Matroska has a MEASURE Tag: https://matroska.org/technical/specs/tagging/index.html

@daschuer
Copy link
Member

A follow up PR is this #1918

@Be-ing
Copy link
Contributor

Be-ing commented Dec 31, 2018

I am closing this because the original PR author has deleted their GitHub account and someone else is actively working on this now in #1918.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants