heronarts / LX

Core library for 3D LED lighting engines
https://chromatik.co/
Other
41 stars 25 forks source link

Add parameter presets #79

Closed jkbelcher closed 1 year ago

jkbelcher commented 1 year ago

I've been using this for a while and find it to be helpful with the MFT. It allows a dedicated Preset knob that, while held, causes the knob turn of associated parameters to jump between a list of values in a "snap-to" manner.

Useful for example with an Angle parameter, which is continuously variable but with presets can be snapped to exact right angles.

Could imagine a UI mouse drag + keyboard function key that accesses this behavior as well by calling incrementNormalized(amount, wrap, TRUE).

mcslee commented 1 year ago

Good UX idea, I'm into the functionality. Will dig into a closer look at the implementation. My off the cuff reactions are:

(1) I think this probably should not be a full subclass, vs. setting functionality on CompoundParameter itself, e.g. I could imagine something like setNotches() or setDetents() - and in fact this should probably live on BoundedParameter since it could be useful for knobs that do not support modulation.

(2) The name Preset could be confusing here, since we have device presets which store a set of parameter values. This is also a set of parameter values, but in a different way - could be a bit confusing. So prefer something like Detents/Notches here for a clearer parallel to editing behavior.

(3) I'm not sure about the mechanism of requiring the existence of a BooleanParameter to toggle this editing mode. That makes sense with the MFT implementation where the push-button behavior is toggling a boolean, but as you've hinted, there could be other UX interactions for this on the mouse/keyboard/whatever that might manage their state in some other way (e.g. modifier keys). We don't, for instance, toggle a BooleanParameter to keep track of when the shift modifier key is being held down during a knob edit.

Will do a deep dive on the implementation review when I get back to coding time.

jkbelcher commented 1 year ago

Agreed on all these improvements.

This could be used on DiscreteParameter as well, so the location might be LXListenableNormalizedParameter or an interface applied to both BoundedParameter and DiscreteParameter.

jkbelcher commented 1 year ago

Incremental progress:

mcslee commented 1 year ago

This is structurally reasonable, but could have fewer state variables and more code consolidation for the update pathways. I went ahead and simplified here: https://github.com/heronarts/LX/commit/a946aed600c89c92a37f3c39b78963b50c2619d3

FYI note that I don't think your version was working properly for normalized detents, since you're still calling setValue with the detentsNormalized array values (which would be 0-1, not the full value range).

FYI I don't love subclasses overriding isDetentEnabled - I think you are better off with a listener that toggles the state via calls to setDetentEnabled for that situation.

Let me know if you find any issues in this alternate implementation.