adafruit/Adafruit_CircuitPython_LED_Animation

Issues with Sparkle and SparklePulse animations

nnja opened this issue · 4 comments

nnja commented

I noticed the following issues with Sparkle animations:

  • Sparkle will never sparkle the first or last pixel
  • SparklePulse animates one less pixel than defined in pixel_num, the last pixel is always off

Simple SparklePulse example that will only light up 4 pixels, not 5:

import board
import neopixel
from adafruit_led_animation.animation.sparklepulse import SparklePulse
from adafruit_led_animation.color import RED

pixels = neopixel.NeoPixel(board.D12, 5, brightness=0.5, auto_write=False)
animation = SparklePulse(pixels, speed=0.1, period=3, color=RED)
while True:
    animation.animate()

The same issue does not affect the regular Pulse animation.

Simple Sparkle example that doesn't sparkle the first or last pixel:

import board
import neopixel
from adafruit_led_animation.animation.sparkle import Sparkle
from adafruit_led_animation.color import RED

pixels = neopixel.NeoPixel(board.D12, 5, brightness=0.5, auto_write=False)
animation = Sparkle(pixels, speed=0.2, color=RED, num_sparkles=10)
while True:
    animation.animate()

I tried to fix this one myself but wasn't able to narrow down the cause of the issue.

nnja commented

As a follow up to this, unless there's an explicit design reason for it what about renaming self._pixels to something else in Sparkle.py? It's non-obvious that it's a list of pixel indicies that will be sparkled of length num_sparkles versus referring to the strip itself like in other animations.

I'm not great at naming things, but maybe something more descriptive like pixel_sparkle_sequence, pixels_to_sparkle, pixel_sparkle_indexes or even sparkles to reflect the _num_sparkles variable?

I do think it could be benefit from being a bit more descriptive name. I think we should to keep the leading underscore on it to represent it's meant for internal use.

I like _pixels_to_sparkle the best I think.

Though I can see the case for _sparkles to match _num_sparkles.
In grid_rain.py there is a _raindrops which feels like similar usage to _sparkles to me as well.

nnja commented

I always lean towards more explicit so _pixels_to_sparkle seems like a fair compromise. If this rename is a welcomed change I'd be happy to open a PR.

Renaming _pixels to _pixels_to_sparkle would be welcome.