Skip to content
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

ColorFromPaletteWLED: improved speed and fixed issue #4524

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Jan 27, 2025

  • fixed issue: blending was also done when color was on a key-index-color which is now skipped
  • speed improvement: conversion is skipped if color is key-color

- fixed issue: blending was also done when color was on a key-index-color which is now skipped
- speed improvement: conversion is skipped if color is key-color
@DedeHai DedeHai added the optimization re-working an existing feature to be faster, or use less memory label Jan 27, 2025
@DedeHai DedeHai added this to the 0.15.1 candidate milestone Jan 27, 2025
@blazoncek
Copy link
Collaborator

After our (me and @DedeHai) discussion on Discord, I experimented a bit about performance and the original version (without color_fade()) performs best (twice as fast as when using color_blend() and color_fade()) at the expense of readability.
But as @DedeHai put it plainly, that is a good trade-off for function that is called several hundred times per frame.

IMO only adding check for lo4 is enough to fix the issue that caused the change.

- ran a few more tests, it is 30% faster like it was originally so reverting. The conversion to 32bit color appears to be wasteful in resources.
@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 28, 2025

@blazoncek is correct, I tested again and the original was actually faster. I thought the final scaling could be done in 32bit at similar speed but apparently that is not the case. Reverted some of the changes.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization re-working an existing feature to be faster, or use less memory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants