-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[WIP] Paint in different color normal beats, bar beats, and phrase beats. #1918
[WIP] Paint in different color normal beats, bar beats, and phrase beats. #1918
Conversation
Also writes a tag with the beat number with the phrase beats. The colors are taken from the skin. Added placeholder values in all skins. Please improve them!!!
This is still a work in progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much. I have added some comments. Sorry for being too picky.
Did you concider to alter the minor down beat in a 7/8 measure?
while I see the benefit for others, I think (for me) this adds yet another distraction to the Mixxx interface. @daschuer what was that downbeat marker PR again? @JaviVilarroig I bet you can pick some snippets from that one.. (didn't test yet, but I'll help with the colors) |
This is the other PR targeting the same problem: |
I will have to investigate how to do it. Any pointer will be welcome :)
|
yeah, an Preferences option would be nice.
Maybe look at /src/preferences/dialog/dlgprefwaveform.cpp for example how waveform type is changed and how the value is passed to the waveform widget. Or the elapsed/remaining switch of the PlayPos widget, that's also changeable in the prefs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the whitespace you've added.
Seems to be causing a merge conflict, or at least, its making life difficult for me, manually merging.
Sorry @WaylonR didn't wanted to me it difficult. |
@JaviVilarroig you need to
Then you should see the merge conflicts, solve them and enter |
@JaviVilarroig read https://git-scm.com/book/en/v2/Git-Branching-Basic-Branching-and-Merging#_basic_merge_conflicts for information about resolving merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting started on this. It is a good step in the right direction.
The preference option currently does not work. The bar and phrase markers are always drawn regardless of the option.
I think before we merge this we should implement some way for users to set the first downbeat. I suspect any automatic method of determining this will be wrong in many cases, even with 4/4 beat grids. Let's discuss that further on Zulip and keep the discussion here focused on the code.
|
||
// Selects the right pen, if we are in phrase also paints the phrase tag | ||
bool showBarAndPhrase = true; | ||
if(beatNum % (c_beatsPerBar * c_barsPerPhrase) == 0 && beatNum > 0 && showBarAndPhrase) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic for determining a beat's position in the bar and the phrase should be moved to the Beats class. For this initial implementation that can rely on the constants that are currently private members of WaveformRenderBeat (those constants should be moved to Beats too). Then it will be easier to implement support for configurable time signatures and the waveform rendering logic shouldn't need any updating. The code here should look something like if (trackBeats->barNumberInPhrase(beatPosition) == 0 && trackBeats->beatNumberInBar(beatPosition) == 0) {
QString label(QString::number(beatNum/16+1)); | ||
QRect wordRect = metrics.tightBoundingRect(label); | ||
const int marginX = 1; | ||
const int marginY = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be multiplied by the scale factor.
painter->drawText(labelRect, Qt::AlignCenter, label); | ||
|
||
painter->setPen(phrasePen); | ||
} else if(beatNum % c_beatsPerBar == 0 && beatNum > 0 && showBarAndPhrase) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, this check should be replaced by a call to Beats, something like:
if (trackBeats->beatNumberInBar(beatPosition) == 0) {
const int marginX = 1; | ||
const int marginY = 1; | ||
wordRect.moveTop(marginX + 1); | ||
wordRect.moveLeft(marginY + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use scaleFactor()
instead of 1
//even wordrect to have an even Image >> draw the line in the middle ! | ||
|
||
int labelRectWidth = wordRect.width() + 2 * marginX + 4; | ||
int labelRectHeight = wordRect.height() + 2 * marginY + 4 ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int labelRectHeight = wordRect.height() + 2 * marginY + 4 ; | |
int labelRectHeight = wordRect.height() + 2 * marginY + 4 * scaleFactor(); |
rectColor.setAlpha(200); | ||
painter->setPen(m_phraseColor); | ||
painter->setBrush(QBrush(rectColor)); | ||
painter->drawRoundedRect(labelRect, 2.0, 2.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 * scaleFactor()
The first downbeat is obviously the Cue, what is there to discuss? Why would you set the Cue differently? |
That is not a safe assumption. There are many different ways to use the cue point with the variety of modes that Mixxx supports. With some of these modes, the cue point moves often and it would be very easy to throw off the grid if that was tied to the cue point. |
Okay. But then the track should also jump to the first downbeat at load instead of the cue. Isn't there an unused "Beat" Cue? What about using that? |
Btw, the green markings pop out much more than the phrase markings. It should be the other way around. |
IMO it would be inappropriate to use a cue for marking the beatgrid. That information belongs with the Beats class. |
So I guess it could just be part of the Beatgrid, and you just move the first downbeat with the Beatgrid? |
Also, we need something for songs that have extra one or two measures in the middle, sometimes that is like that before the drop and it would offset all phrase markings. |
Yes, but this is going beyond the scope of this PR to draw the marks on the waveforms. If you want to discuss that further, let's continue on Zulip. |
I think I manage to solve the conflicts that were affecting you. Not 100% sure dure to my lack of expertise on git. Can you please confirm? |
It looks like you accidentally committed a lot of new files with the last commit. |
Humm. May be I added in my branch all the changes from master? I just wanted to remove the conflict :( Again my lack of undestanding of git. Sorry for the mess. Is there any way I can fix that? Something like undo the commit and then try again to fix the files? |
First, list the files that should not be committed in |
1a33720
to
92bdfdd
Compare
Co-Authored-By: JaviVilarroig <43292281+JaviVilarroig@users.noreply.github.com>
Co-Authored-By: JaviVilarroig <43292281+JaviVilarroig@users.noreply.github.com>
…arroig/mixxx into lp1128005_Bar_and_phrase_marking
Yep. I know. Despite the fact of being able to add a new configuration variable, add it to the dialog and having it properly serialized, I have been unable to read the value from the WaveformRenderBeat class. I have set up a constant to allow testing while I try to find a solution to that issue. I tried with a Control/ControlProxy pair but failed. If you can point me to a practical example of the use of that classes to read configuration parameters I will investigate from there. |
I'm going to put this PR in stand by wile I work in a separate one for improving the Beats class to manage additional information for bars and phrases, variable BPM and beat emphasis, as discussed in Zulip. |
Can we please integrate this as is soon and care about the details later? I do a lot of stuff that just revolves around phrases, and having this would already save me a huge amount of work. |
I have been playing some sessions using this PR and the result is not really satisfactory. Without more control on what is considered a downbeat, initial one and short phrases, it's not really helpful. |
1 similar comment
I have been playing some sessions using this PR and the result is not really satisfactory. Without more control on what is considered a downbeat, initial one and short phrases, it's not really helpful. |
@JaviVilarroig can you undelete your branch? It will still be a useful starting point for whoever works on adding bar and phrase information to the Beats class. |
I agree that this is not in a state to be merged, but since there was some interest in the feature so far, I recreated the original branch and fixed the option to enable / disable from menu: https://github.com/sharst/mixxx/tree/beat_and_phrase_marks |
Cool Thank you. Can you issue a PR for this and describe the current state of work, to make it visible for all. |
Yes, I will create a new PR once I have integrated the suggestions made
here and finished adapting the Track class.
…On 6/8/19 7:08 PM, Daniel Schürmann wrote:
Cool Thank you. Can you issue a PR for this and describe the current
state of work, to make it visible for all.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1918?email_source=notifications&email_token=AAIQQPWJZEOT5YZAJCIPMN3PZPRQLA5CNFSM4GGHJLR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHY35Q#issuecomment-500141558>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIQQPXCXJCP6BKWKI4YXZLPZPRQLANCNFSM4GGHJLRQ>.
|
Also writes a tag with the beat number with the phrase beats.
The colors are taken from the skin.
Added placeholder values in all skins. Please improve them!!!