Renamed symbols (Parameter -> NormalizedValue) and added aliases for controller script backward compatibility #13027
Conversation
… and added aliases for controller script backward compatibility
…re renamed, and a check for the aliases itself
|
Thank you for picking this up. |
|
Unfortunately It is not always the normalized value. In case of knobs and sliders, it reflects the potentiometer position normalized to the range of 0 ... 1. For all other COs it is equal to value. I am a bit torn if the term "NormilizedValue" just shifts the confusion. Maybe we can find a name the emphasize that this is only relevant for sliders and knobs. Something like PotentiometerPos, PotPosition, controlPosition, or just position.? What do you think? |
In these cases I would suggest to print a warning that set/getNormalizedValues is not supported for non-Pot-Meter-COs, and that set/getValue should be used for this CO instead. |
| function getNormalizedValue(group: string, name: string): number; | ||
|
|
||
| /** | ||
| * Gets the control value normalized to a range of 0..1 |
There was a problem hiding this comment.
| * Gets the control value normalized to a range of 0..1 | |
| * Returns the knob / slider position in the range of 0..1 or the value if there is no position associated. |
| function setNormalizedValue(group: string, name: string, newValue: number): void; | ||
|
|
||
| /** | ||
| * Sets the control value specified with normalized range of 0..1 |
There was a problem hiding this comment.
| * Sets the control value specified with normalized range of 0..1 | |
| * Sets the control value via the position on the knob / slider scale in the range of 0..1 or the value if there is no position associated. |
| * Normalizes a specified value using the range of the given control, | ||
| * to the range of 0..1 |
There was a problem hiding this comment.
| * Normalizes a specified value using the range of the given control, | |
| * to the range of 0..1 | |
| * Returns the knob / slider position in the range of 0..1 or the unchanged value if no | |
| * no position associated. |
| function getNormalizedDefaultValue(group: string, name: string): number; | ||
|
|
||
| /** | ||
| * Returns the default value of a control, normalized to a range of 0..1 |
There was a problem hiding this comment.
| * Returns the default value of a control, normalized to a range of 0..1 | |
| * Returns the default knob / slider position range of 0..1, or the default value if no no position associated. |
Good idea. In these case the "parameter" value should not be used. |
Sure, backward compatibilty is prio no. 1 for everything controller mapping related. I set the PR to draft, until I've addressed this. |
|
Did you consider to adjust the name? |
To be honest, neither Potentiometer nor Position is related to what the function does, and to have this selfdescriptive name is the sole reason I work on this. |
Normalization to 0..1 alone is IMHO not descriptive enough. For my understanding it is always scaled to the linear position of the slider / knob scale or otherwise defaults to value. This is at least true for rate, pregain and volume. Normalization involved always min and max value and a scale. In this case min and max are the end positions of the potentiometer and the scale linear 0...1. Please give examples where neither Potentiometer nor Position is related. |
|
An example is the normalized value returned by engine.getParameter is useful for VU-Meter, just multiply it by the number of LEDs in your controler and you get what you need to light them up in the right brightness. The typical use case for the normalization done by engine.getParameterForValue is range checking in an assert like code. The problem I'm addressing with this PR is that, as in any good API, the other function names of the controller scripting API describe what they do/implement, but not the "Parameter" ones. |
|
The 'vu_meter' CO is a plain ContolObect it has no potentiometer position assisted and using get/setPatameter may issue the discussed warning.
This does not work for plain ControlObejects and may issue a warning if dome anyway. I agree tin general, hat parameter was a name without any meaning so let's find a name that solves the issue. The relationship is defined here: |
|
Since it automatically selects the normalization mode depending on the CO type, what about "AutoNormalizedValue" ? |
|
There term "normalization" does not fit well IMHO. We need a name for this
|
|
Can you describe what the function does in other terms? I never heard another term than normalization for this operation. |
|
The parameter/value pair has been introduced to allow simple mappings of the controller hardware to the values in Mixxx. The advantage is that this logic is part of Mixxx and there is no need to implement it in mappings. This makes sure that controller and GUI sliders behave the same. The mapping author can override it by changing the "value" directly. |
| if (pControl && | ||
| !m_st.ignore( | ||
| pControl, coScript->getParameterForValue(newValue))) { | ||
| pControl, coScript->getNormalizedValueForValue(newValue))) { |
There was a problem hiding this comment.
Soft takeover deals with the position of the slider, not with the business logic value in Mixxx. That's we we need to calculate here the position on the slider form the value.
| void QmlControlProxy::slotControlProxyValueChanged(double newValue) { | ||
| emit valueChanged(newValue); | ||
| emit parameterChanged(m_pControlProxy->getParameter()); | ||
| emit normalizedValueChanged(m_pControlProxy->getNormalizedValue()); |
There was a problem hiding this comment.
Why we do we emit two signals here?
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
github-actions
bot
removed
the
stale
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
JoergAtGithub
removed
the
stale
2 participants
In the COs and the controller script API, the meaningless term "Parameter" was systematically used instead of the obviously correct term "Normalized Value".
This is particularly problematic as it is a symbol that is exposed to the user through our controller scripting API. A user has no chance to understand, that "Parameter" indicates a value with normalized range - unless he looks up in the documentation for it.
I renamed the affected methods therefore, and added aliases to the controller scripting API for backward compatibility.
Additionally I expanded the term Midi in some CO methods to Midi7Bit, as nowadays Midi allows larger ranges than 0...127. But this range is meant here.
Last change was extending the controllerscriptenginelegacy_test.cpp to cover the previously uncovered methods which were renamed, and a check for the aliases itself.