Visualize distance to a key when pitch shifting #215

badescunicu · 2014-03-30T15:45:19Z

Launchpad link: https://bugs.launchpad.net/mixxx/+bug/1258624

…in a QPair object

…displaying the same value for both loaded tracks. Haven't figured out how to solve this

daschuer · 2014-03-30T19:10:24Z

Thank you for the pull request.
I think I get the point where you get sucked.

The visual_key is connected via skins

[Channel1],visual_key

Currently there is only one CO display connection allowed. So you may introduce a separate Key distance widget or just introduce a new "visual_key_float" or just make "visual_key" a float.

If we are sure we don't need the discrete key value, I would go the way to add th fraction of a key to the visual_key. This would clean up the code a lot. But be care full if overflows occurs.
the key must not change from minor to major.

There was a related discussion in #47

@rryan, do we the integer nature of visual_key?

daschuer · 2014-03-30T19:17:19Z

From the programmatic point of view having a spitted CO for value and fraction break the atomic nature of the CO, because you cant be sure if the two values read from the CO belong together.
This is no issue in this PR, but we should not do it her to avoid later issues.

daschuer · 2014-03-30T19:19:09Z

I am not our skin artist, but IMHO we should remove the parentheses and separate key and cent by a space.

daschuer · 2014-03-30T19:23:51Z

This PR uncovers an other issue: It is very hard to set the pitch slider to a full key.
Any idea how to fix that? We may remove the cent value if it is near enough to a full key. Mmm but that just tricks the user.
A slider with a snap mode .. Or a key snap button ...

rryan · 2014-03-30T23:32:24Z

visual_key should remain an integer -- the overflow issues aren't worth complicating things.

WKey is a dedicated key display widget so it's fine to (e.g. just like WNumberPos and WNumberRate) use multiple controls. You could introduce a float tonic control and a bool major/minor control for use in this widget.

daschuer · 2014-03-31T05:52:41Z

IMHO the overflow issue is not that hard, so my vote is for add a "visual_key_float" this keeps the atomic nature of the data. For a possible interpretation see #47 (comment)

badescunicu · 2014-03-31T12:12:08Z

What should I further do to this PR?

As @daschuer suggested, maybe we should remove the parentheses. Here are some possible formats.

This is the current one, with parentheses:

Without parentheses:

Without parentheses and space before 'c':

rryan · 2014-03-31T14:34:49Z

The atomic update issue isn't worth worrying about. These controls are typically used for display. If that was really an issue then the key control shouldn't exist at all and client code should just calculate the effective key directly from file_key and pitch.

rryan · 2014-03-31T14:35:12Z

As for formatting, the widget should support a format string so the skinner controls it.

rryan · 2014-03-31T14:39:16Z

Rendering the cents as text isn't worth the pixels for such a low-value piece of information -- @badescunicu what about coloring the background or painting a bar across the widget that indicates how close you are?

rryan · 2014-03-31T14:45:54Z

Actually, WKey shouldn't do this by default or at least it should be disable-able via the skin.

Any skin with high production value isn't going to put this in there as text and will want a visual way to represent it. In order to do that, we need a potmeter CO that represents the distance to the key (not a key + fraction CO, just the -1 to 1 fraction).

That way you can create a WDisplay widget to represent the distance and put a WKey widget positioned over it to render the key text.

rryan · 2014-03-31T14:46:57Z

src/track/keyutils.cpp

                          (key_changes_scaled > 0 ? 0.5 : -0.5));

-    return scaleKeySteps(key, key_changes);
+    // Distance to the nearest key in cents
+    int cents = static_cast<int>((key_changes_scaled - key_changes) * 100);


Instead of providing cents and rounding off the precision I think you should just keep this as a fraction in floating point.

(because units and round-off should be decided by the caller of this utility function)

daschuer · 2014-03-31T15:38:53Z

Hi Nicu,

sorry for the discussion here. We should have clarify this in the bug report. But we must get a conclusion now.

First the answer to your question: I like the last version most.

RJs: Potentiometer Idea might be the best solution but is also the most work. For this we really need the a pure distance CO like your "visual_key_distance".
For me it is fine to have it normalizes to 1 cent. This it the most common unit and there is no need to normalize it to one, because all COs have already normalized "Parameter" value.

Proposal:

keep key and distance separate
keep cent resolution for distance
add style sheet capability for key and cent value in a way that also allow to remove the cent value
Format the third way
Merge

All other Ideas should be issued in a separate bug and pull request.

@RJ and @badescunicu Is this Ok?

rryan · 2014-03-31T16:18:38Z

RJs: Potentiometer Idea might be the best solution but is also the most work.

Not sure what you mean -- it's like 3 lines of code.

// in constructor 
m_pKeyDistance = new ControlPotmeter("key_distance", -1, 1);
// in update
QPair<key, double> result = scaleKeyOctaves(...)
m_pKeyDistance->set(result.second);

That's it. Now skinners can make <Display> widgets that represent the key distance.

rryan · 2014-03-31T16:19:17Z

Or really, it's -0.5 to 0.5 because whenever the value is greater than 0.5 it wraps around.

rryan · 2014-03-31T16:22:39Z

keep cent resolution for distance

Eh, no please don't. Units and formatting is a display concern. If WKey will have the option to show cents, it should calculate them there.

rryan · 2014-03-31T16:25:55Z

Format the third way Merge

I don't believe it's worth the pixels and given the resolution of our detector it's over-promising to the user to tell them that we know the cents here.

@esbrandt will likely want the display option to indicate with a subtle overlay or some kind. Kind of like the phase meter in Traktor.

rryan · 2014-03-31T16:35:32Z

Ooh, I'm actually imagining a circular key distance meter that wraps around WKey. That would look cool :).

kain88-de · 2014-03-31T16:39:31Z

Ooh, I'm actually imagining a circular key distance meter that wraps around WKey. That would look cool :).

+1

esbrandt · 2014-03-31T16:43:49Z

Hmm, i really don't have my mind made up here.
My first impression was "Thats something i don't want to have permanently visible, because of the low information value".

But maybe i`mistaken what this PR actually does.
Can some of you guys explain the feature, like you would to a user?
Currently, i understand that the widget displays a information on track load. The distance from the detected key, to the rounded key we actually display. Does this cent value even change on manual key change, do we support key change in the cent range?

On Mar 31, 2014, at 6:25 PM, RJ Ryan notifications@github.com wrote:

Format the third way Merge

I don't believe it's worth the pixels and given the resolution of our detector it's over-promising to the user to tell them that we know the cents here.

@esbrandt will likely want the display option to indicate with a subtle overlay or some kind. Kind of like the phase meter in Traktor.

—
Reply to this email directly or view it on GitHub.

esbrandt · 2014-03-31T16:45:13Z

So display the circle of fifth?

On Mar 31, 2014, at 6:39 PM, kain88-de notifications@github.com wrote:

Ooh, I'm actually imagining a circular key distance meter that wraps around WKey. That would look cool :).

+1

—
Reply to this email directly or view it on GitHub.

rryan · 2014-03-31T16:58:13Z

@esbrandt -- for some context:

In master today:

We detect the key as X
When you load the track into Mixxx, if the rate is neutral (or keylock is on) and pitch shift is 0 we show you the key X if you use a WKey widget in the skin.
When you pitch shift or tweak rate without keylock, the key widget changes to show you the current "closest key" which is the key after the pitch shift.
the bug is about representing how "close" you are to the key. So if you've turned the pitch knob just over half way between C Major and D Flat Major, Mixxx says you are already in "D Major" but that is simply the key that is closest to your current pitch shift. You are still out of tune by 50 cents.

This PR should add a control that tells you the current distance (either -0.5 to 0.5 out of tune from the current closest key, where 0 is right in tune).

That way, you could use WDisplay to make a series of pixmaps that represent the range -0.5 to 0.5. I'm thinking of a transparent image with a grey circle slice -- like the knobs in Shade. 0 is repesented as no displaying color. If you are out of tune the amount out of tune is represented by the circle (as in Shade, the amount the knob is turned is represented by the circle). A WKey widget could be centered in the same position as the WDisplay widget but on top of it so that the key text is drawn in the center of the circle. This would not waste any additional pixels but still inform you of the distance away from the current closest key.

daschuer · 2014-03-31T21:21:14Z

RJs: Potentiometer Idea might be the best solution but is also the most work.

Not sure what you mean -- it's like 3 lines of code.

Yes, of cause. Once we have a "visual_key_distance" CO of potentiometer nature it is done for
this PR, that was part of my Idea. The "most work" is for the skinners.

Or really, it's -0.5 to 0.5

This is also fine. Actually the unit is "Key" or "semitone" (100c) in this case.
COs can distinguish between "Value", the value with unit and "Parameter" the normalized value 0 .. 1 -> min .. max.
In this case this is important, because distance to a semitone can also expressed in
Millioctave or Savart.

By the way: What is the reason for -0.5 to 0.5 (100c) Scale)? For me the 1c scale is more natural.

daschuer · 2014-03-31T21:25:31Z

OK guys , we are running out of time, and Nicu needs a conclusion. The is still nothing against my first proposal as intermediate step, isn't it?

If this is wrong, we need a new proposal!

rryan · 2014-04-01T16:02:43Z

A cent isn't really meaningful in equal-temperament. It's simply a scaling of a ratio by 100 -- a per-cent. If we're not going to show cents in our UI, why should a CO be measured in them?

But I really don't care if it's -0.5 to 0.5 or -50 to 50. It just feels arbitrary when you effectively have a ratio to multiply it by 100.

I don't think anything beyond the CO is necessary for the official Mixxx skins but an XML option to WKey to show the distance in cents is fine.

badescunicu · 2014-04-04T12:11:19Z

From what I've understood, I have to turn EngineKeyDistance into a ControlPotmeter and keep the cents value as a double, as @rryan suggested here (#215 (comment)). However, I'm not sure what I should change in wkey.cpp.

daschuer · 2014-04-04T16:20:32Z

Yes, right.
It seams like a fast solution that allows to show or hide the cent value by a skin option is the best for wkey.cpp.
If it will be shown in a default skin, depends on @esbrandt's plans.
IMHO the distance info is required for harmonic mixing and a "analog" display suits best.

…nce and store the cent difference as a double

badescunicu · 2014-04-05T14:28:07Z

I did the back end modifications. Let me know what I should do on the WKey side.

rryan · 2014-04-05T16:25:48Z

@badescunicu looks good! The last thing is that WKey should take an option in it's setup() method. (Over-ride it from WLabel but make sure to call WLabel::setup(node, context); first to get all the setup from WLabel done too). That should just call context.selectBool("DisplayCents", ...) or similar. The key node is specified in skins like this:

<Key>
  <Group>[Channel1]</Group>
  <MinimumSize>40,20</MinimumSize>
</Key>

And then all the skinner will have to do is put a: <DisplayCents>true</DisplayCents> in there if they want to show cents or not.

badescunicu · 2014-04-06T23:10:35Z

Now it is configurable from the skin. I have noticed an issue: if the name of the track is too long, it overlaps with WKey widget. But I suppose it is all a matter of skin configuration.

daschuer · 2014-04-09T19:24:06Z

Hi Nicu,

sorry for the delay. I have just tested your branch and it works fine.

The only thing is that the display is jumpy, in case it changes from one decimal place to two and in case it changes from C to Cb. Do you have an idea to solve? (IMHO this is not an issue to delay the merge)

Unfortunately your branch conflicts with master. After you have solved this we can merge.

Would you mind to edit this for the new schema?
http://www.mixxx.org/wiki/doku.php/creating_skins?s[]=skins#effective_musical_key_display
And add the new control to
http://www.mixxx.org/wiki/doku.php/mixxxcontrols

Thank you very much.

badescunicu · 2014-04-11T00:05:55Z

I have fixed the conflicts. However I don't have the rights to edit the wiki. Should I register for an account and edit the schema and control table?

daschuer · 2014-04-11T06:53:08Z

If you register an account, at the Wiki, you should be able to edit it.
Thank you for your work!

Visualize distance to a key when pitch shifting

badescunicu added 6 commits March 30, 2014 18:27

Remove whitespace

79c6e56

Add and set EngineKeyDistance ControlObject in keycontrol.cpp

07e642b

Compute distance to the nearest key and return it along with the key …

22887e4

…in a QPair object

Display the distance to the nearest key WKey widget. Currently it is …

cfeac44

…displaying the same value for both loaded tracks. Haven't figured out how to solve this

Modify WKey constructor to take a const char* as a first parameter

190dc16

Implement parseEngineKey method to create a WKey widget

980185d

rryan reviewed Mar 31, 2014
View reviewed changes

Use a ControlPotmeter instead of a ControlObject for visual_key_dista…

8f26e75

…nce and store the cent difference as a double

Make the display of cents configurable from the skin

d3bd683

Fix conflicts with master

2c9acf1

daschuer added a commit that referenced this pull request Apr 11, 2014

Merge pull request #215 from badescunicu/feat_1258624

bfe902a

Visualize distance to a key when pitch shifting

daschuer merged commit bfe902a into mixxxdj:master Apr 11, 2014

mixxxbot mentioned this pull request Aug 22, 2022

Visualize distance / closeness to a key when pitch shifting. #7215

Closed

m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024

Welcome Niko to the core team (mixxxdj#215)

28d7797

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visualize distance to a key when pitch shifting #215

Visualize distance to a key when pitch shifting #215

badescunicu commented Mar 30, 2014

daschuer commented Mar 30, 2014

daschuer commented Mar 30, 2014

daschuer commented Mar 30, 2014

daschuer commented Mar 30, 2014

rryan commented Mar 30, 2014

daschuer commented Mar 31, 2014

badescunicu commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan Mar 31, 2014

rryan Mar 31, 2014

daschuer commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

kain88-de commented Mar 31, 2014

esbrandt commented Mar 31, 2014

esbrandt commented Mar 31, 2014

rryan commented Mar 31, 2014

daschuer commented Mar 31, 2014

daschuer commented Mar 31, 2014

rryan commented Apr 1, 2014

badescunicu commented Apr 4, 2014

daschuer commented Apr 4, 2014

badescunicu commented Apr 5, 2014

rryan commented Apr 5, 2014

badescunicu commented Apr 6, 2014

daschuer commented Apr 9, 2014

badescunicu commented Apr 11, 2014

daschuer commented Apr 11, 2014

Visualize distance to a key when pitch shifting #215

Visualize distance to a key when pitch shifting #215

Conversation

badescunicu commented Mar 30, 2014

daschuer commented Mar 30, 2014

daschuer commented Mar 30, 2014

daschuer commented Mar 30, 2014

daschuer commented Mar 30, 2014

rryan commented Mar 30, 2014

daschuer commented Mar 31, 2014

badescunicu commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan Mar 31, 2014

Choose a reason for hiding this comment

rryan Mar 31, 2014

Choose a reason for hiding this comment

daschuer commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

rryan commented Mar 31, 2014

kain88-de commented Mar 31, 2014

esbrandt commented Mar 31, 2014

esbrandt commented Mar 31, 2014

rryan commented Mar 31, 2014

daschuer commented Mar 31, 2014

daschuer commented Mar 31, 2014

rryan commented Apr 1, 2014

badescunicu commented Apr 4, 2014

daschuer commented Apr 4, 2014

badescunicu commented Apr 5, 2014

rryan commented Apr 5, 2014

badescunicu commented Apr 6, 2014

daschuer commented Apr 9, 2014

badescunicu commented Apr 11, 2014

daschuer commented Apr 11, 2014