Skip to content

Issues with Sparkle and SparklePulse animations #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nnja opened this issue Jun 14, 2020 · 4 comments · Fixed by #49
Closed

Issues with Sparkle and SparklePulse animations #48

nnja opened this issue Jun 14, 2020 · 4 comments · Fixed by #49

Comments

@nnja
Copy link

nnja commented Jun 14, 2020

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
Copy link
Author

nnja commented Jun 14, 2020

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?

@FoamyGuy
Copy link
Contributor

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
Copy link
Author

nnja commented Jun 22, 2020

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.

@rhooper
Copy link

rhooper commented Jun 26, 2020

Renaming _pixels to _pixels_to_sparkle would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants