heronarts / LX

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

tempoLocked Click will never .click() #61

Closed jvyduna closed 2 years ago

jvyduna commented 2 years ago

This line ensures a tempoLocked LXPeriodicModulator will always have a basis <1:

https://github.com/heronarts/LX/blob/e3d0d11a7d61c73cd8dde0c877f50ea4a58a14ff/src/main/java/heronarts/lx/modulator/LXPeriodicModulator.java#L257

After this, looped is set if the basis is >= 1.

https://github.com/heronarts/LX/blob/e3d0d11a7d61c73cd8dde0c877f50ea4a58a14ff/src/main/java/heronarts/lx/modulator/LXPeriodicModulator.java#L276

Click relies on boolean loop() to compute a value of 1 and thus, to .click()

https://github.com/heronarts/LX/blob/e3d0d11a7d61c73cd8dde0c877f50ea4a58a14ff/src/main/java/heronarts/lx/modulator/Click.java#L90

Therefore, while a tempSynced Click will .click(), if it's also phase locked to global tempo, it will not.

I'm not confident enough on the effects on other modulators to submit a PR, but I wondered what you'd think of changing:

https://github.com/heronarts/LX/blob/e3d0d11a7d61c73cd8dde0c877f50ea4a58a14ff/src/main/java/heronarts/lx/modulator/LXPeriodicModulator.java#L257

to:

this.basis = rawBasis;

and leaving the basis >= 1 condition to be handled in:

https://github.com/heronarts/LX/blob/e3d0d11a7d61c73cd8dde0c877f50ea4a58a14ff/src/main/java/heronarts/lx/modulator/LXPeriodicModulator.java#L274

mcslee commented 2 years ago

Great catch, thanks for this.

I fixed this up in the tempo syncing section, the numLoops logic is a bit different than the section below. https://github.com/heronarts/LX/commit/c10fba3a15766bd9441506a09e3e27e938f9b4e0

Think that should sort it out - let me know if you still see any weirdness here!