Make the overview-waveform darker for the played portion of the track #1203

MK-42 · 2017-03-04T00:39:50Z

… as a stronger visual indicator of the current position

aiming to fix lp:1328748
Actually that change set is only one single line of code. I don't have any experience with qt painting and perfomance measures. So please feel free to comment.

I am thinking about making the color configurable for skin artists. What do you think about that?

… as a stronger visual indicator of the current position

Be-ing · 2017-03-04T00:41:03Z

Cool idea. Please post screenshots when proposing changes to the GUI.

MK-42 · 2017-03-04T00:46:51Z

Cool idea. Please post screenshots when proposing changes to the GUI.

sorry, forgot about that. There you are:

I chose black and opacity of 200 (in range 0-255). Looks good for me. But as said, maybe let the skin artists decide that?

Edit: Added view of hsv waveform at least for latenight

Be-ing · 2017-03-04T00:49:33Z

Yes, I think skin designers should be able to customize it. Does this make the playhead indicator redundant?

MK-42 · 2017-03-04T00:52:44Z

I would like to stick with the additional playhead indicator (so having both). For some skins it might be enough to use this overlay, but I think not every usecase will fit in that one.

My usecase for the overlay was to have a quick view how far the track is if I'm further away from the screen.
Having that sharp 'here we are' line may help to see the current position wrt to some change in the track (visible in the waveform) or sth like that.

MK-42 · 2017-03-04T01:17:57Z

Never looked into the skin stuff before. Any way to get a color with alpha from the skin xml to a QColor? Only saw usages of setNamedColor, that can handle alpha channels only since QT 5.2

We could use a default alpha value, or two Nodes in the skin xml, one for color, one for alpha?

…form configurable by skin artists

MK-42 · 2017-03-04T14:50:07Z

I chose to parse the format that QT5.2 uses for RGBA colors in hex format, which is #AARRGGBB. Parsing that color is not really something that WaveformSignalColors should do, as its more a lowlevel helper function, but I don't know what would be an appropriate place for that function. Also I forgot to make it static... Any hints where to put that piece of code in the skin section is appreciated, I don't know the skin code well yet.

The Skin-Node is called PlayedOverlayColor as a first shot.

What do you think about that?

daschuer · 2017-03-05T11:52:10Z

src/waveform/renderers/waveformsignalcolors.cpp

@@ -134,6 +140,30 @@ void WaveformSignalColors::fallBackDefaultColor() {
    fallBackFromSignalColor();
 }

+QColor WaveformSignalColors::rgbaColorFromString(QString sColorString) {
+    // Takes a string with format #AARRGGBBAA and returns the corresponding QColor with alpha-channel set


Extra AA at the end

daschuer · 2017-03-05T11:58:24Z

src/waveform/renderers/waveformsignalcolors.cpp

@@ -134,6 +140,30 @@ void WaveformSignalColors::fallBackDefaultColor() {
    fallBackFromSignalColor();
 }

+QColor WaveformSignalColors::rgbaColorFromString(QString sColorString) {


I think it would be straight forward to move this function to skincontext.h similar to
hasNodeSelectString()

something like

hasNodeSelectColor()

This way all color strings in the skin can benefit from the #AARRGGBB syntax.

I would go with selectColor() alone, I think the presence-info is not required in that place, falling back to transparent makes most sense for backwards-compat. And an empty string will be converted to fully transparent by QT.

daschuer · 2017-03-05T12:01:36Z

src/waveform/renderers/waveformsignalcolors.cpp

@@ -55,6 +55,12 @@ bool WaveformSignalColors::setup(const QDomNode &node, const SkinContext& contex
        m_playPosColor = m_axesColor;
    }

+    m_playedOverlayColor = rgbaColorFromString(context.selectString(node, "PlayedOverlayColor"));
+    if (!m_playedOverlayColor.isValid()) {
+        m_playedOverlayColor = QColor(0,0,0,200);


Please add a comment what this color is.
This should also become a const in th anonymous namesace on to of this file.

will be using Qt::transparent for backwards-compat.

daschuer · 2017-03-05T12:12:06Z

src/widget/woverview.cpp

@@ -338,6 +338,8 @@ void WOverview::paintEvent(QPaintEvent * /*unused*/) {
            }

            painter.drawImage(rect(), m_waveformImageScaled);
+            // desaturate the scaled waveform-image up to the current play-position by overdrawing semi-transparent black


Since the color is now customizable the comment is outdated.
Please skip this line if the color is fully transparent.

... and we "try" to break lines before column 80.

Be-ing · 2017-03-05T14:30:48Z

Never looked into the skin stuff before. Any way to get a color with alpha from the skin xml to a QColor? Only saw usages of setNamedColor, that can handle alpha channels only since QT 5.2

Then we cannot rely on setNamedColor alone because Mixxx 2.1 will still be using Qt 4. Qt4 does support transparency but it will take a little more code than just calling setNamedColor.

MK-42 · 2017-03-05T15:51:50Z

Then we cannot rely on setNamedColor alone because Mixxx 2.1 will still be using Qt 4. Qt4 does support transparency but it will take a little more code than just calling setNamedColor.

For that reason I already implemented that parsing logic by hand, using the QT5.2 #AARRGGBB Syntax.

Thanks @daschuer for reviewing, will address those comments asap.

ninomp · 2017-03-05T16:07:37Z

src/widget/woverview.cpp

@@ -338,6 +338,8 @@ void WOverview::paintEvent(QPaintEvent * /*unused*/) {
            }

            painter.drawImage(rect(), m_waveformImageScaled);
+            // desaturate the scaled waveform-image up to the current play-position by overdrawing semi-transparent black
+            painter.fillRect(0, 0, m_iPos, m_waveformImageScaled.height(), m_signalColors.getPlayedOverlayColor());


This will only work with horizontal waveform. Please handle the vertical waveform case also. You can use https://github.com/ninomp/mixxx/tree/verticalwaveform_skin for testing.

Thanks for that hint!

Be-ing · 2017-03-06T05:48:58Z

src/waveform/renderers/waveformsignalcolors.cpp

+        m_playedOverlayColor = QColor(0,0,0,200);
+    }
+    m_playedOverlayColor = WSkinColor::getCorrectColor(m_playedOverlayColor).toRgb();
+
    m_bgColor.setNamedColor(context.selectString(node, "BgColor"));


While you're at it, could you use the new rgbaColorFromString function here?

…OverlayColor to Qt::transparent

MK-42 · 2017-03-08T14:03:17Z

Now working for vertical and horizontal waveforms. Moved color-parsing logic into skincontext.h, made some more colors 'alpha-capable'.
I didn't chose rgba colors for the waveform signal colors, because I don't know what that would mean performance wise. Is there a usecase for that?

Be-ing · 2017-03-08T16:00:04Z

I didn't chose rgba colors for the waveform signal colors, because I don't know what that would mean performance wise. Is there a usecase for that?

I can't think of a use case for that.

Be-ing

Almost there, just a few small changes.

Be-ing · 2017-03-08T17:18:15Z

src/skin/skincontext.h

+
+        QColor color;
+        QString sColorString = nodeToString(selectElement(node, nodeName));
+        if (sColorString.startsWith('#') && sColorString.length() == 9){


nitpick: space between ) and {

Be-ing · 2017-03-08T17:18:43Z

src/widget/woverview.cpp

+
+            // Overlay the played part of the overview-waveform with a skin defined color
+            QColor playedOverlayColor = m_signalColors.getPlayedOverlayColor();
+            if (playedOverlayColor.alpha() > 0){


nitpick: space between ) and {

Be-ing · 2017-03-08T17:18:50Z

src/widget/woverview.cpp

+            // Overlay the played part of the overview-waveform with a skin defined color
+            QColor playedOverlayColor = m_signalColors.getPlayedOverlayColor();
+            if (playedOverlayColor.alpha() > 0){
+                if (m_orientation == Qt::Vertical){


nitpick: space between ) and {

Be-ing · 2017-03-08T17:58:50Z

src/waveform/renderers/waveformsignalcolors.cpp

+        m_playedOverlayColor = Qt::transparent;
+    }
+
+    m_bgColor = context.selectColor(node, "BgColor");
    if (!m_bgColor.isValid()) {
        m_bgColor = QColor(0, 0, 0);


I should have changed this in #1208, but could you change this from QColor(0, 0, 0) to Qt::transparent to restore backwards compatibility with old skins?

MK-42 · 2017-03-09T11:24:55Z

should be fixed now

Be-ing · 2017-03-09T15:05:22Z

LGTM! 👍 @daschuer, ready for merge?

MK-42 · 2017-03-10T07:43:31Z

failing travis is a timeout I think. ready to merge from my perspective

ronso0 · 2017-03-10T20:03:51Z

I can see the Cue mark in the screens, but just to be sure: HotCue marks are layed on top of this, right?

MK-42 · 2017-03-10T20:11:27Z

I can see the Cue mark in the screens, but just to be sure: HotCue marks are layed on top of this, right?

The overlay is drawed directly on top of the waveform, before (under) everything else:

ronso0 · 2017-03-10T20:13:05Z

Cooooool! nice skin you have there ;)

MK-42 · 2017-03-10T20:27:51Z

It was your amazing work in Tango that made me implement that ;) Its nice to have that small Deck section without (for me) unneeded stuff. But the play pos wasnt that clear I thought. Would really like to see that in Tango therefore.

ronso0 · 2017-03-10T20:32:03Z

It was your amazing work in Tango that made me implement that ;) Its nice to have that small Deck section without (for me) unneeded stuff.

My pleasure! Very nice to feel that it works out as intended.

But the play pos wasnt that clear I thought.

Wasn't 100% happy with that either. Once this is merged I am -like billions of mixxxers I suppose

ronso0 · 2017-03-10T20:33:31Z

So <PlayedOverlayColor>#AARRGGBB<.. will be the new tag to work with. Anything else?

MK-42 · 2017-03-10T20:36:50Z

Yes, thats it.
New color is called PlayedOverlayColor.
And Alpha also works for AxesColor PlayPosColor and BgColor ;-)

ronso0 · 2017-03-10T20:38:15Z

Bamm!!

daschuer · 2017-03-11T23:18:40Z

Thank you for this PR. It LGTM.

Before merge wee need your permission
Please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done. Thank you.

MK-42 · 2017-03-12T06:29:56Z

Already did that Back in #587 :)

daschuer · 2017-03-12T19:31:03Z

Ah sorry, of cause you did.

…ixxxdj/mixxx#1203

Make the overview-waveform darker for the played portion of the track…

b13545b

… as a stronger visual indicator of the current position

Make the color of the overlay for played portion of the overview wave…

ddc6bd9

…form configurable by skin artists

daschuer reviewed Mar 5, 2017

View reviewed changes

ninomp reviewed Mar 5, 2017

View reviewed changes

Be-ing reviewed Mar 6, 2017

View reviewed changes

Be-ing mentioned this pull request Mar 7, 2017

Tango skin #1151

Merged

MK-42 added 3 commits March 8, 2017 14:44

Moved parsing of #AARRGGBB colors to skincontext and defaulted played…

97e0218

…OverlayColor to Qt::transparent

use selectColor for all non-signal colors in Waveforms

1f69334

Merge remote-tracking branch 'upstream/master' into desaturate_waveforms

a3ea433

Be-ing suggested changes Mar 8, 2017

View reviewed changes

MK-42 added 2 commits March 9, 2017 12:16

style changes

ab0710b

make background color of waveform overview transparent by default

8ecce73

daschuer merged commit 0b34263 into mixxxdj:master Mar 12, 2017

MK-42 deleted the desaturate_waveforms branch March 13, 2017 14:18

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

esbrandt added a commit to esbrandt/manual that referenced this pull request Jun 24, 2017

Make the waveform overview darker for the played portion of the track m…

a812668

…ixxxdj/mixxx#1203

mixxxbot mentioned this pull request Aug 22, 2022

Desatuate played part of track in decks and samplers #7504

Closed

Make the overview-waveform darker for the played portion of the track #1203

Make the overview-waveform darker for the played portion of the track #1203

Conversation

MK-42 commented Mar 4, 2017

Be-ing commented Mar 4, 2017

MK-42 commented Mar 4, 2017 • edited Loading

Be-ing commented Mar 4, 2017

MK-42 commented Mar 4, 2017 • edited Loading

MK-42 commented Mar 4, 2017

MK-42 commented Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Mar 5, 2017

MK-42 commented Mar 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MK-42 commented Mar 8, 2017

Be-ing commented Mar 8, 2017

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MK-42 commented Mar 9, 2017

Be-ing commented Mar 9, 2017

MK-42 commented Mar 10, 2017

ronso0 commented Mar 10, 2017

MK-42 commented Mar 10, 2017

ronso0 commented Mar 10, 2017 • edited Loading

MK-42 commented Mar 10, 2017

ronso0 commented Mar 10, 2017

ronso0 commented Mar 10, 2017 • edited Loading

MK-42 commented Mar 10, 2017

ronso0 commented Mar 10, 2017

daschuer commented Mar 11, 2017

MK-42 commented Mar 12, 2017

daschuer commented Mar 12, 2017

MK-42 commented Mar 4, 2017 •

edited

Loading

MK-42 commented Mar 4, 2017 •

edited

Loading

MK-42 commented Mar 4, 2017 •

edited

Loading

ronso0 commented Mar 10, 2017 •

edited

Loading

ronso0 commented Mar 10, 2017 •

edited

Loading