Issues with Sparkle and SparklePulse animations
nnja opened this issue · 4 comments
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.
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.
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.