add WEffectMetaKnob, draws arc from default meta position #12638
Conversation
ronso0
force-pushed
the
weffectmetaknob
branch
from
0b79e6c
to
f3b7873
Compare
|
This one looks now: |
|
No prob, This revealed some misplaced 'neutral point on scale' values, e.g. high freq. of Graphic EQ. |
|
Draft so I can clean up and force-push without disrupting a review. |
src/widget/wknobcomposed.cpp
Outdated
| if (!parameter.has_value() || parameter.value() < 0 || parameter.value() > 1) { | ||
| m_defaultAngle = std::nullopt; | ||
| } else { | ||
| m_defaultAngle = std::lerp(m_dMinAngle, m_dMaxAngle, parameter.value()); | ||
| } |
There was a problem hiding this comment.
not a suggestion, just a preview of what would be possible in C++23. (imo easier to read and also faster because it only does a single has_value check.
| if (!parameter.has_value() || parameter.value() < 0 || parameter.value() > 1) { | |
| m_defaultAngle = std::nullopt; | |
| } else { | |
| m_defaultAngle = std::lerp(m_dMinAngle, m_dMaxAngle, parameter.value()); | |
| } | |
| m_defaultAngle = parameter.transform([this](double value) { | |
| return (parameter < 0 || parameter > 1) ? | |
| std::nullopt : | |
| std::lerp(m_dMinAngle, m_dMaxAngle, parameter); | |
| }); |
There was a problem hiding this comment.
In general I would prefer to give m_defaultAngle a valid value here. It is better to have a variable that can be used unconditional in all cases.
The idea to have an indicator that the knob does not control an effect or just "---" is still open. Than the question is how to check that easily. Probably a topic of a follow up PR if we ever want to do it.
There was a problem hiding this comment.
In general I would prefer to give m_defaultAngle a valid value here. It is better to have a variable that can be used unconditional in all cases.
That defeats the purpose of the optional. This snippet of code here can't accurately determine what value is appropriate.
|
By the way: Is this a last minute 2.4 PR? |
|
Yes, that can slip into 2.4.1 last minute! Please check the rect commit and the fixup with your suggestion. |
Looks good. Thank you. |
😞 Otherwise we may as well move this to 2.5 |
No idea. Just AppleClang being completely outdated as usual...
Yeah. maybe leave a TODO there so the next time someone stumbles over this, they can do the trivial refactoring. |
|
Alright, CI all green. Please take a look, then I'll squash the fixups (had to fixup since the commit introducing std::optional couldn't be built). |
| // TODO(xxx) Use m_defaultAngle->value() as soon as | ||
| // AppleClang 10.13+ is used. |
There was a problem hiding this comment.
ideally this just used the monadic operations from C++23 by then 😉
|
Am I done here, all reviews addressed? |
| virtual std::optional<double> neutralPointOnScale() const { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Couldn't we implement this right here already?
| virtual std::optional<double> neutralPointOnScale() const { | |
| return std::nullopt; | |
| } | |
| std::optional<double> neutralPointOnScale() const { | |
| if (m_pManifestParameter == nullptr) { | |
| return std::nullopt; | |
| } | |
| return m_pManifestParameter->neutralPointOnScale(); | |
| } |
There was a problem hiding this comment.
Then we'd have to override it for EffectButtonParameterSlot, just for the possible case that the neutral point is requested for a button parameter?
That would otherwise return the unchanged default 0. Though, in that case (controlling a button parameter with a knob widget) a lot else wouldn't work quite as desired.
I don't mind..
Wdyt?
There was a problem hiding this comment.
Then we'd have to override it for EffectButtonParameterSlot, just for the possible case that the neutral point is requested for a button parameter?
Well if that makes sense for a button parameter, we should make sure the value also makes sense. If it doesn't make sense (which is what you are suggesting, right?) then we can also let the return value be nonsensical.
The advantage of implementing it in the base class is that we can avoid virtual dispatch.
I think the primary issue here is that neutralPointOnScale only makes sense on knobs, but the existing code infrastructure requires it to be implemented on the base-class. Is that correct?
There was a problem hiding this comment.
Alright, implemented as you suggested. In the unlikely button case it'll return the default 0.
|
Thank you |
Never mind, there was an unaddressed review and I've pushed this back until now. |
When an effect is loaded, the knob gets the default meta parameter from the effect manifest and draws the knob arc from there to current angle.
Closes #12634
Don't let this distract you from the 2.4 release, this can very well wait for 2.4.1 or later.
I added this widget only where arcs are or may be used, i.e. all skins except Shade.