-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix for the bad palette performance #3978 #3979
Conversation
…te creating loading/creating palettes on each call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I have to disapprove this PR as it negates effect and palette blending.
@@ -91,6 +91,7 @@ | |||
//#define SEGLEN strip._segments[strip.getCurrSegmentId()].virtualLength() | |||
#define SEGCOLOR(x) strip.segColor(x) /* saves us a few kbytes of code */ | |||
#define SEGPALETTE strip._currentPalette | |||
#define SEGPALETTEIDX strip._currentPaletteIndex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong.
Current palette is not a strip
property, it is an effect property and needs to be stored in Segment object.
See explanation below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to use SEGPALETTE, which resolves to strip._currentPalette, as the palette cache.
Alternative would be to declare a new global variable for this cache, as I did before in the example.
(i thought that SEGPALETTE was still referred to, but not in active use, so wanted to 'reuse' it efficiently)
@@ -571,7 +572,7 @@ typedef struct Segment { | |||
uint8_t currentMode(void); // currently active effect/mode (while in transition) | |||
uint32_t currentColor(uint8_t slot); // currently active segment color (blended while in transition) | |||
CRGBPalette16 &loadPalette(CRGBPalette16 &tgt, uint8_t pal); | |||
CRGBPalette16 ¤tPalette(CRGBPalette16 &tgt, uint8_t paletteID); | |||
inline CRGBPalette16 ¤tPalette(uint8_t paletteID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inlining will increase code size which is unacceptable as we are already at 98.5% flash size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segment::currentPalette is only ever called in WS2812FX::service and Segment::color_from_palette, so it might even reduce flash size (no function call overhead).
loadPalette(targetPalette, pal); | ||
unsigned prog = progress(); | ||
inline CRGBPalette16 &Segment::currentPalette(uint8_t pal) { | ||
if ( SEGPALETTEIDX != pal ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work during mode (aka effect) blending. Each mode (effect) can have its own palette that needs to be blended during execution of mode (effect).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think that if will not work for blending? It does not interfere with the effect and segment palettes, but just gets used as a cache?!?
unsigned noOfBlends = ((255U * prog) / 0xFFFFU) - _t->_prevPaletteBlends; | ||
for (unsigned i=0; i<noOfBlends; i++, _t->_prevPaletteBlends++) nblendPaletteTowardPalette(_t->_palT, targetPalette, 48); | ||
targetPalette = _t->_palT; // copy transitioning/temporary palette | ||
uint16_t noOfBlends = ((255U * prog) / 0xFFFFU) - _t->_prevPaletteBlends; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint16_t
will unnecessarily increase code size and slow execution down as it adds & 0xFFFF
to every operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we can keep both vars as 'unsigned'
@blazoncek , the change does not negate effect and palette blending, but just acts as a cache for loadPalette. |
Hi @blazoncek and @jw2013, maybe before jumping directly into a possible solution, we could do some good old "design engineering"? From what I understood,
Assuming that some caching is possible there are design decisions to be made
✏️ Opinions and comments, please! |
Obsolete by 77e6ea8 |
Fix for the bad performance #3978 caused by Segment::color_from_palette creating loading/creating palettes on each call.