KnobComposed masked ring #645

ferranpujolcamins · 2015-07-12T19:30:29Z

Added support for a new "Ring" pixmap in WKnobComposed that is automatically masked.

A new "Ring" pixmap is added to WKnobComposed. It is automatically masked between MinAngle and MaxAngle as the knob is turned.

The BackPath pixmap is drawn on top of the new "Ring" pixmap. This allows the background to cast shadows to the ring.

The new "Centered" tag makes the ring totally masked at 12 o'clock position and makes it "sweep-in" as the knob is turned from there.

The new "RingMaskXOffset" and "RingMaskYOffset" should be used in case the Ring pixmap is not exactly centered in the middle of the canvas. They are relative to the center of the canvas.

The last commit is just to show the changes in action, it should be reverted before merging this PR. The new knobs are a bit ugly, I made them quickly. Don't judge this PR for their looks :)
LateNight probably needs a new set of knobs that integrate the ring well, if this PR is accepted, I'll make a new one with better knobs for LateNight.

…tically masked. A new "Ring" pixmap is added to WKnobComposed. It is automatically masked between MinAngle and MaxAngle as the knob is turned. The new "Centered" tag makes the ring totally masked at 12 o'clock position and makes it "sweep-in" as the knob is turned from there. The new "RingMaskXOffset" and "RingMaskYOffset" should be used in case the Ring pixmap is not exactly centered in the middle of the canvas. They are relative to the center of the canvas.

daschuer · 2015-07-14T10:24:08Z

I have tested this patch and it just works.

Please move the test skin changes to https://github.com/mixxxdj/developer_skins. This way we can merge this without destroying anything.

The BackPath pixmap is drawn on top of the new "Ring" pixmap. This allows the background to cast shadows to the ring.

I am afraid this will be hard to explain. "The background is not the bottom layer"
Can't we achieve the shadow by a static shadow on the ring?

daschuer · 2015-07-14T10:34:38Z

src/widget/wknobcomposed.cpp

+        QPainterPath path;
+        int w = width();
+        int h = height();
+        path.moveTo(w/2.0 + m_iMaskXOffset, h/2.0 + m_iMaskYOffset);


These calculations are hard to understand. Please use intermediate variables with explaining names and add more comments.

daschuer · 2015-07-14T10:39:18Z

"Centered" is hard to understand and somehow too common.
Can we calculate the value from the connected Control? It is the Zero point?
This is especially useful for Effect Knob where the scale of the connected control changes by the selected effect.
Otherwise "RingEmptyAngle" or something might be more self explaining.

daschuer · 2015-07-14T10:49:22Z

RingMaskXOffset is hard to understand.
I think we can ditch it, if we set the Mask start point always to the Knob center, independent from the Ring dimensions.

ferranpujolcamins · 2015-07-14T13:59:13Z

Thank you for reviewing this @daschuer later I'll try to address what you've commented.

Can we calculate the value from the connected Control? It is the Zero point?
This is especially useful for Effect Knob where the scale of the connected control changes by the selected effect.

You're right, this totally needs to be done. I'll see if I can achieve that.

RingMaskXOffset is hard to understand.

We can't get rid of the offset tags. As you can see in the picture, if the ring is not centered in the picture but the mask is draw centered, it just doesn't work.

However if you consider that the tags names are misleading I have no problem in changing them. I'll document the changes to the wiki, and probably add this picture, so it shouldn't be hard to understand anymore :)

I think we can ditch it, if we set the Mask start point always to the Knob center, independent from the Ring dimensions.

Thats a good idea and might work well with plain designs. But if designers want to give the skin some 3D sense, they'll probably have the knob offset from the center.

daschuer · 2015-07-14T18:26:17Z

Thank you for the drawings. They clarify much.
Is the mask actually rounded? I wonder if it is reasonable to just cover a triangle from the rotation point towards the pixamp borders.

IMHO the ring center point should be match the knob center by default.

Could "RingMaskXOffset" become "RingCenterXOffset"?
It may also be a good strategy to postpone this feature to the time when there is a real demand.

ferranpujolcamins · 2015-07-14T19:21:57Z

Is the mask actually rounded? I wonder if it is reasonable to just cover a triangle from the rotation point towards the pixamp borders.

Well, you almost guess what's really going on :) The mask is a pie like in the drawing, but the radius is actually the minimum that makes the rectangle containing the pixmaps to be inscribed inside the pie. What this means is that the whole knob area is masked, the picture I sent is not correct in this aspect.

Actually, the code you said is not clear (which I'm going to rewrite and comment) does the calculations needed for this..

IMHO the ring center point should be match the knob center by default.

Could "RingMaskXOffset" become "RingCenterXOffset"?

That's actually very reasonable, I'll fix this too.

It may also be a good strategy to postpone this feature to the time when there is a real demand.

Well, I'm the demand :)
I wanted this because I felt LateNight needs it. Imagine you are running Mixxx on a 15.6" laptop using LateNight with 4decks. You might actually be a bit far away from the screen. There's plenty of little knobs on the mixer section, and is difficult to see at a glance which EQ knobs are not at 12 o'clock. With a ring like the one I drafted in the PR, it is easier to identify active EQ knobs.

Additionally, this PR doesn't break neither modify any preexisting functionality, so it is safe in that aspect.

daschuer · 2015-07-14T20:54:30Z

Well, I'm the demand :)

Sure! And I like this feature as well. I was only worrying about the ring offset feature.

mixxx-buildbot · 2015-07-14T20:54:33Z

Automated Message from BuildBot: Mixxx Admins, is this safe to test? Respond with: "ok to test", "add to whitelist", "skip ci", or "test this please" (to retest).

daschuer · 2015-07-14T20:55:22Z

add to whitelist

ferranpujolcamins · 2015-07-14T21:13:35Z

It's a good idea to take the offset respect the center of the knob pixmap instead of the center of the background, I like it. Also to rename the tags to RingMaskXOffset and RingMaskYOffset.

As I said, they are necessary if some skin designer wants to add a 3d effect. I've made a quick example: the first is a plain design where both the ring and the knob have the same center, the second design has vertical and horizontal ring offset plus an additional background.

Don't worry, I promise I'll add proper documentation for this to the wiki :)

So, this will be ok once I find time to make all the discussed changes?

daschuer · 2015-07-15T12:32:34Z

Ok, go ahead and thank you again for the good explanations.

ferranpujolcamins · 2015-07-15T21:39:31Z

Oh, how shall I commit changes to developer skins? Shall I issue a PR with a copy of LateNight with the changes applied to it?

Now ring appears on top of background.

daschuer · 2015-07-15T21:50:58Z

Yes. You may also maintain it as you own testing skin there.

and "RingMaskYOffset" to "RingCenterYOffset".

ferranpujolcamins · 2015-07-16T00:07:04Z

Ok it turns out that this is a bit trickier than I thought. For CO we have defaultValue(). But defaultValue is not necessarily the reference value from which the ring mask should start expanding.
For example, for the echo effect, the feedback knob has the default value at 2 o'clock (because we want the default value to make the effect sound in all its glory). But we would like the ring to expand from the full counter clockwise position.

Furthermore, in the future we might offer the user the possibility to customize the controls default values, like Traktor does with the FX parameters. Clearly, we can't tie this with the CO defaultValue.

How can we handle this? Shall I add a new double member to CO that tells where the ring should start expanding, maybe something like "neutralValue" or "noEffectValue"?

daschuer · 2015-07-16T05:59:21Z

"neutralValue" as a general ControlPotmeter addition sounds reasonable. I think if this defaults to zero it will fit for almost all COs.

ferranpujolcamins · 2015-07-22T18:50:10Z

How do I get to the corresponding ControlPotmeter from WKnobComposed? From its m_connections inherited from WBaseWidget?

daschuer · 2015-07-23T06:30:24Z

Right. You may do it like this:
https://github.com/mixxxdj/mixxx/blob/master/src/widget/wslidercomposed.cpp#L71

ferranpujolcamins · 2015-07-23T14:14:12Z

Ok, so I've just realised I don't really understand how this should work, sorry.

Right now, in WKnobComposed, I have the ConfigKey of the corresponding CO. But, if I create a ControlPotmeter from it, the neutralValue is created arbitrarily on that ControlPotmeter, like m_stepCount for example. I don't know how each CO could have a specific neutralValue. Shouldn't neutralValue be in ControlDoublePrivate like defaultValue in order to achieve that?

On the other hand, I understand neutralValue doesn't make sense for a binary CO, so it seems to me that the correct place would indeed be ControlPotmeter. So I'm a bit confused :(

daschuer · 2015-07-23T15:01:23Z

ah, I see.

You cannot create a ControlPotmeter from an existing key. You can only create a ControObjectSlave to access the atomic value of the control Object.

You can also not access the ControlPotmeter via ControlParameterWidgetConnection -> ControlObjectSlave -> ControDoublePrivate

A possible solution would be to add getNeutralParameter() to the ControDoublePrivate and fill it by the ControlPotmeterBehavior class.

I wonder if this will fit to the target that we are able to change the NeutralParameter / NeutralValue on the effect knobs.

By the way: Parameter = normalized Value

ferranpujolcamins · 2015-07-23T18:26:08Z

Thank you Daniel. I'm definitely going to finish this, but It's just a cosmetic feature, so I'm going to see if I can fix other skin issues first.

…ode.

ferranpujolcamins · 2015-08-21T12:07:16Z

A possible solution would be to add getNeutralParameter() to the ControDoublePrivate and fill it by the ControlPotmeterBehavior class.

@daschuer can you give me some more details about this? I don't really understand what you mean.

daschuer · 2016-04-15T10:04:06Z

src/effects/effectmanifest.h

@@ -122,6 +131,8 @@ class EffectManifest {
    bool m_isMasterEQ;
    // This helps us at DlgPrefEQ's basic selection of Filter knob effects
    bool m_isForFilterKnob;
+    // This helps the skin draw knob rings correctly.
+    double m_dSuperKnobScaleStart;


Please remove m_dSuperKnobScaleStart from the manifest.
It is a property of each effect parameter, but that is can be postponed to an later PR.

daschuer · 2016-04-15T10:10:27Z

Your developer skins are working real nice, Thank you.
There are a view open comments before we can merge this.

Fixed comment in ControlAudioTaperPot. Tabs -> Spaces.

ferranpujolcamins · 2016-04-15T11:22:25Z

I addressed the issues.
Thank you for reviewing this.

daschuer · 2016-04-15T13:38:04Z

src/effects/effectmanifest.h

@@ -25,7 +25,6 @@ class EffectManifest {
        : m_isMixingEQ(false),
          m_isMasterEQ(false),
          m_isForFilterKnob(false),
-          m_dSuperKnobScaleStart(0.0),


this removes this initializer only ...

Wow sorry. Solved.

daschuer · 2016-04-17T13:46:35Z

Travis has detected an issue:

src/effects/effectslot.cpp: In member function ‘double EffectSlot::superParameterScaleStart()’:

src/effects/effectslot.cpp:256:25: error: ‘class EffectManifest’ has no member named ‘superKnobScaleStart’

         return manifest.superKnobScaleStart();

ferranpujolcamins · 2016-04-18T08:54:46Z

Sorry, I'll fix that. Should I completely remove knob rings from effect super knob or leave it with a default value like 0?

daschuer · 2016-04-18T10:31:32Z

Yes.

rryan · 2017-01-28T06:53:50Z

What's the status on this PR?

ferranpujolcamins · 2017-02-03T19:15:45Z

I plan to give it some love soon.

Be-ing · 2018-04-18T03:19:28Z

This pull request has not been updated in years so I am closing it to reduce the clutter of open PRs. If you solve the merge conflicts and would like to have this merged in the future, please reopen it.

daschuer · 2018-04-18T20:51:06Z

We have the commitment that this will be revived soon.

ferranpujolcamins · 2018-04-19T12:53:57Z

My commitment for this is for 2.3 release. I'll rework it in a new branch anyway, so I'm closing this branch. I've filed a bug in launchpad to track this: https://bugs.launchpad.net/mixxx/+bug/1765399

ferranpujolcamins added 3 commits July 12, 2015 20:02

Changed WKnobComposed "Centered" tag to read a boolean parameter.

2374e09

Quick proof of concept in LateNight eqs. To be reverted.

2f04eac

daschuer reviewed Jul 14, 2015
View reviewed changes

Merge branch 'master' into KnobComposed-Masked-Ring

cfd728a

WKnobComposed: Swapped background and ring render order.

fc4e035

Now ring appears on top of background.

WKnobComposed: Renamed tags "RingMaskXOffset" to "RingCenterXOffset"

f826d58

and "RingMaskYOffset" to "RingCenterYOffset".

ferranpujolcamins added 2 commits August 2, 2015 08:01

WKnobComposed: fixed ring not being drawn correctly on non centered m…

e7bcd9f

…ode.

Merge branch '1.12' into KnobComposed-Masked-Ring

108d7dd

daschuer reviewed Apr 15, 2016
View reviewed changes

Removed m_dSuperKnobScaleStart from EffectManifest.

52054d1

Fixed comment in ControlAudioTaperPot. Tabs -> Spaces.

daschuer reviewed Apr 15, 2016
View reviewed changes

Completely removed m_dSuperKnobScaleStart from EffectManifest.

f959d39

ferranpujolcamins mentioned this pull request Jan 7, 2018

Deere 2.1 knobs & faders redesign #1399

Merged

ronso0 mentioned this pull request Jan 17, 2018

Deere .: remove center gap from Meta/Super knob ring #1486

Merged

Be-ing closed this Apr 18, 2018

daschuer reopened this Apr 18, 2018

Be-ing added stalled and removed stalled labels Apr 18, 2018

Be-ing added this to the stalled milestone Apr 18, 2018

ferranpujolcamins closed this Apr 19, 2018

Be-ing modified the milestones: stalled, 2.3.0 Apr 19, 2018

ferranpujolcamins deleted the KnobComposed-Masked-Ring branch May 14, 2019 16:23

ferranpujolcamins mentioned this pull request Sep 14, 2019

Knobs with arcs #2275

Merged

This was referenced Aug 22, 2022

Add support for progress slider widget #7959

Open

effect specific Meta knob/QuickEffect knob default position #8882

Closed

mixxxbot mentioned this pull request Aug 24, 2022

Add ring around knobs to indicat knobs' position #9249

Closed

KnobComposed masked ring #645

KnobComposed masked ring #645

Conversation

ferranpujolcamins commented Jul 12, 2015

daschuer commented Jul 14, 2015

daschuer Jul 14, 2015

Choose a reason for hiding this comment

daschuer commented Jul 14, 2015

daschuer commented Jul 14, 2015

ferranpujolcamins commented Jul 14, 2015

daschuer commented Jul 14, 2015

ferranpujolcamins commented Jul 14, 2015

daschuer commented Jul 14, 2015

mixxx-buildbot commented Jul 14, 2015

daschuer commented Jul 14, 2015

ferranpujolcamins commented Jul 14, 2015

daschuer commented Jul 15, 2015

ferranpujolcamins commented Jul 15, 2015

daschuer commented Jul 15, 2015

ferranpujolcamins commented Jul 16, 2015

daschuer commented Jul 16, 2015

ferranpujolcamins commented Jul 22, 2015

daschuer commented Jul 23, 2015

ferranpujolcamins commented Jul 23, 2015

daschuer commented Jul 23, 2015

ferranpujolcamins commented Jul 23, 2015

ferranpujolcamins commented Aug 21, 2015

daschuer Apr 15, 2016

Choose a reason for hiding this comment

daschuer commented Apr 15, 2016

ferranpujolcamins commented Apr 15, 2016

daschuer Apr 15, 2016

Choose a reason for hiding this comment

ferranpujolcamins Apr 15, 2016

Choose a reason for hiding this comment

daschuer commented Apr 17, 2016

ferranpujolcamins commented Apr 18, 2016

daschuer commented Apr 18, 2016

rryan commented Jan 28, 2017

ferranpujolcamins commented Feb 3, 2017

Be-ing commented Apr 18, 2018

daschuer commented Apr 18, 2018

ferranpujolcamins commented Apr 19, 2018