heronarts / LX

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

APCMk2 uses one physical knob for each LinkedColor #58

Closed raldi closed 2 years ago

raldi commented 2 years ago

Hmm, this isn't working quite right. Since changing to cc.getNormalized(), changing saturation through the CUE_LEVEL knob doesn't seem to work anymore. Will investigate...

mcslee commented 2 years ago

FYI getting on a plane so I’ll be a bit slow to reply to this next revision.

For the controls issue, make a test pattern with a few parameters and then call in it’s constructor:

setRemoteControls( p1, p2, p3, p4, p5, lcp1.hue, lcp1.saturation, lcp2.hue, lcp1.brightness, p6, lcp2.mode );

p1-6 can be whatever. But the point is to overflow the number of knobs and for the sub-parameters of the linked color parameters to not all be present and to be out of order, particularly in slots 8-9 where we’ll hit knobs.length.

Le mar. 22 mars 2022 à 15:59, raldi @.***> a écrit :

Hmm, this isn't working quite right. Since changing to cc.getNormalized(), changing saturation through the CUE_LEVEL knob doesn't seem to work anymore. Will investigate...

— Reply to this email directly, view it on GitHub https://github.com/heronarts/LX/pull/58#issuecomment-1075731953, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAER6WWYTKIG4VFIMLHZC5TVBJGD7ANCNFSM5RLVEDHA . You are receiving this because you commented.Message ID: @.***>

raldi commented 2 years ago

Ah, I hadn't realized that was something a pattern could customize. I'll take a crack at this tomorrow.

On Tue, Mar 22, 2022 at 5:01 PM Mark Slee @.***> wrote:

FYI getting on a plane so I’ll be a bit slow to reply to this next revision.

For the controls issue, make a test pattern with a few parameters and then call in it’s constructor:

setRemoteControls( p1, p2, p3, p4, p5, lcp1.hue, lcp1.saturation, lcp2.hue, lcp1.brightness, p6, lcp2.mode );

p1-6 can be whatever. But the point is to overflow the number of knobs and for the sub-parameters of the linked color parameters to not all be present and to be out of order, particularly in slots 8-9 where we’ll hit knobs.length.

Le mar. 22 mars 2022 à 15:59, raldi @.***> a écrit :

Hmm, this isn't working quite right. Since changing to cc.getNormalized(), changing saturation through the CUE_LEVEL knob doesn't seem to work anymore. Will investigate...

— Reply to this email directly, view it on GitHub https://github.com/heronarts/LX/pull/58#issuecomment-1075731953, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAER6WWYTKIG4VFIMLHZC5TVBJGD7ANCNFSM5RLVEDHA

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/heronarts/LX/pull/58#issuecomment-1075766618, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACEZG3MW2ZW3BQPBMFJG43VBJNNLANCNFSM5RLVEDHA . You are receiving this because you authored the thread.Message ID: @.***>

raldi commented 2 years ago

I think everything is ready now.

mcslee commented 2 years ago

Here's a bit of cleanup: https://github.com/heronarts/LX/commit/8e92758affdb2ba3839cddff11a9b4aab712fe78

Some of the checks there are a bit more robust. The main functionality difference was that your implementation would skip adding knobs the second time the same parent parameter was seen. But I don't think we should do this - the idea of getRemoteControls() is that the programmer can specify a specific layout for the ordering of knobs on a control surface (in the future potentially across multiple dynamic pages). This is why null is expressly allowed in that list, if someone wants to say "I really want this parameter in spot 7, even though there's nothing in knob spots 5 and 6" - e.g. for ergonomic layout reasons.

I think that's the right general behavior, but if this is super awkward for your project resulting in loads of redundant color knobs... I would recommend you make a little helper for your pattern classes that calls setRemoteControls() and filters out adding all the ColorParameter sub-params by default.

Also removed that one updateColorKnobs() call in registerChannel - as I think that was better handled by having register() properly set the value on each color knob at init-time, which it now does.

I was flying slightly blind on this as don't have an APC40mk2 to test here - so you'll have to final test for now until I'm back home where there's one on my desk. Apologies if something was not quite right in here...

raldi commented 2 years ago

I agree with you about honoring setRemoteControls() for devices that call it, but right now the default for patterns that don't is that each of their LinkedColorParameters, if any, take up five physical knobs on the APC. I can write a helper function for our patterns, but wouldn't it make more sense to set things up so that the default was that each LCP gets one physical knob, and if someone wants to customize, they can use setRemoteControls()? I think I can write a PR that sets things up that way, if you agree.

mcslee commented 2 years ago

Yeah, agreed. I thought about doing this while I was doing the cleanup change-set, but then shrugged it off out of laziness/is-it-really-necessary/it-was-late. Now you've said the same thing, so I'm sold.

https://github.com/heronarts/LX/commit/d19eb4902eb1696aefa3de2879b8342693e2d5ac

I made this something that AggregateParameter classes can specify. Let me know if this works as expected for you?