Skip to content

Fix off by 1 error affecting Sparkle and SparkePulse animations #49

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

Merged

Conversation

nnja
Copy link

@nnja 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

See #48 for details and example code that tests this fix.

Closes #48

@nnja nnja force-pushed the fix_sparkle_and_sparklepulse_animations branch from bfec5a2 to a4bb2dd Compare June 14, 2020 05:40
@@ -94,5 +94,6 @@ def draw(self):
def after_draw(self):
self.show()
for pixel in self._pixels:
self.pixel_object[pixel] = self._half_color
self.pixel_object[pixel + 1] = self._dim_color
num_pixels = len(self.pixel_object)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this outside the loop to avoid having to check the length on each iteration? (This should be O(1) but avoiding as many calls as possible in loops is good)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Since this value doesn't change during the course of execution I moved it up to the constructor. Let me know if you'd prefer it back in the after_draw method.

@nnja nnja force-pushed the fix_sparkle_and_sparklepulse_animations branch from a4bb2dd to 51621ef Compare June 14, 2020 21:41
@nnja
Copy link
Author

nnja commented Jun 15, 2020

The CI failure seems unrelated to my changes.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I successfully recreated the issue and tested this fix. Looks good to me, thank you for the fix!

The CI issue is unrelated to this. The fix for it was just recently merged into library. If you can pull in the latest changes and push a commit to this branch I think it should resolve that issue now.

@nnja nnja force-pushed the fix_sparkle_and_sparklepulse_animations branch from 51621ef to da0d851 Compare June 20, 2020 21:47
@nnja
Copy link
Author

nnja commented Jun 20, 2020

@FoamyGuy I rebased on top of master, looks like that fixed the CI issue 👍

@nnja
Copy link
Author

nnja commented Jun 20, 2020

@FoamyGuy also, do you happen to have any thoughts on the comment I left in the issue? #48 (comment)

@rhooper rhooper merged commit 48ff1e8 into adafruit:master Jun 26, 2020
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jun 30, 2020
Updating https://github.com/adafruit/Adafruit_CircuitPython_LIS3MDL to 1.1.4 from 1.1.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_LIS3MDL#11 from kattni/update-combo-example

Updating https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO to 3.3.2 from 3.3.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#38 from makermelissa/master
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#37 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Button to 1.3.3 from 1.3.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Button#21 from makermelissa/master
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Button#20 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.3.3 from 2.3.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#58 from cjsieh/cycle_complete
  > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#49 from nnja/fix_sparkle_and_sparklepulse_animations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with Sparkle and SparklePulse animations
4 participants