Make rainbow effects more colorful #3681
Conversation
|
Bravo! That boggled me as well. |
|
One more thing: As to avoid angering certain users please make the new color wheel calculation optional. |
|
Update: Please check |
|
Tested this today but have to say that I fail to see any real difference on my LEDs. |
|
Hm. You are right. There is a difference for me, but I can't really say that one better than the other. My version certainly is a bit brighter (obviously, now that I think about it). I have the cheapest LEDs I could find though. Possibly there might be a greater difference with higher quality or future LEDs or setups. I mean there clearly is a difference on an LCD screen. |
I did actually look at that when I worked on
If you wish, (and if you want to keep my version in the first place,) I can have a look at what's possible, but it won't be as optimal. It might result in an ever so slightly smaller binary though. |
|
I've already updated code to utilize both as an option. Please review and comment. |
|
I saw that. Some more explicit feedback:
|
|
Another option would be to keep the original function for ESP8266, and use the hsv function for ESP32. I made some comparisons between the color computation of these functions: D1 Mini
WT32-ETH01
|
Are you talking about |
|
|
|
Perhaps change |
|
According to godbolt's compiler explorer, that produces the same binary |
Nice to learn something new. Quite a different ASM to Z80. ;) So simpler C code produces 27% overhead. Who would've thought. |
|
Is this still PR needed? It looks like there is now an alternative implementation with the same intention in place. However there are some key differences:
|
|
what is wrong with using fastled HSVtoRGB? I wrote an optimized version of that, stuck in PR limbo. |
|
Apart from the direction of H? |
|
Then I still dont understand why adding another function that does the same is needed? Is it used in any effects where the gained speed matters? If so why not use rainbow palette (IIRC that is fewer operations)? |
|
It's not adding a full function, just some specialized code. |
|
is the initial assesment still true? i.e. are the colorwheel colors bad? |
|
In the default configuration, yes. With the rainbow color wheel option enabled, it now uses an hsv conversion with the caveats described above. |
I forgot to address that: The rainbow color palette simply looks different. If you look at the actual values, it also isn't really an hsv sweep (or at least it wasn't when I last looked at it). |
|
I checked this and IMHO yes this is still relevant. However I think adding another version of a rainbow is not a good solution, it should just use fastled HSV to RGB and not add another option for users to choose: 0.16 changes color handling significantly and reading the comments above, the difference is minute in reality (please correct me if I am wrong). |
|
In all honesty I was using code from this PR for several months and though I tried I never really noticed huge difference on displays I used. So I forgot about it. However at some point @DedeHai came up with this topic and it turned out FastLED had "rainbow" color wheel for a long time. So reusing it seemed natural. I do not expect there are thousands of users that would want three variants of color wheel as the difference might be imperceptible on many LEDs. But I am not against giving users the option since the code is relatively compact and small. If you can integrate current implementation and add your take (perhaps via dropdown "Select color wheel: Standard/Rainbow/Colorful" or radio button) I think it can be integrated. |
|
I believe a quick recap is in order.
At the time, this PR was not merged because of performance considerations. Instead the much slower So much for what happened so far. So no, I don't think an option to choose between 3 versions is a good idea. Instead we should decide on one. I have no problem closing this PR. But I would also like to find the best rainbow. :)
Now for some new insights. Because there are now 3 versions, I wanted to create an update of the image in the original PR description, and noticed a few issues: 
The first (single) line is what gimp thinks a hue sweep should look like. The other double lines are Conclusions from this image: of the raw lines, only So I made a version that looks perfect in the UI const uint32_t h = (pos * 3) / 128;
const uint32_t f = ((pos * 6) % 256) * 7 / 12;
switch (h) {
case 0: return RGBW32(255 , f , 0 , w);
case 1: return RGBW32(148 - f, 255 , 0 , w);
case 2: return RGBW32(0 , 255 , f , w);
case 3: return RGBW32(0 , 148 - f, 255 , w);
case 4: return RGBW32(f , 0 , 255 , w);
case 5: return RGBW32(255 , 0 , 148 - f, w);
default: return 0;
} |
|
nice comparison. I assume you read the fastled article about why they do it that way? |
|
No, otherwise I wouldn't have wondered about it so much I assume ^^ Where is that article? |
|
I see, thanks. I would say they fail their stated goals. The yellow is even more dull than the one in Anyway, that fastled hsv rainbow is actually exactly the rainbow palette. (or at least what it used to be? I think I read something about gamma and modified palettes recently) So I would argue that there is no need for a |
|
Oh btw. if this is what fastled uses for "spectrum" hsv<->rgb conversions:
that also disagrees with what wikipedia says and what |
|
you are raising some good points.
the fastled hsv rainbow version was created for LED use without gamma correction, hence why you see it all dull when applying gamma to it, so we should now ditch that entirely. edit: |
|
Well, I want a technical looking palette, that's the whole point of this PR. ^^ For a more natural one there is always the rainbow palette. I therefore wouldn't get rid of it. Adding options for gamma correction, and even more gamma correction, and then for where to apply it is nice, but if you need a certain set of options enabled for one palette to look good, and then another set of options enabled for a different palette to look good, it doesn't help, does it. That would only work if those options were parts of the preset instead of the configuration. |
TripleWhy
force-pushed
the
colorfulRainbows
branch
from
86bb4ad to
23020ea
Compare
WalkthroughThe changes remove the "rainbow color wheel" feature from the codebase. This includes deleting the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@TripleWhy please start afresh with new PR. Keep both existing "rainbows" however pleasing they may look to you. In the end it is only this that matters: const uint32_t h = (pos * 3) / 128;
const uint32_t f = (pos * 6) % 256;
switch (h) {
case 0: return RGBW32(255 , f , 0 , w);
case 1: return RGBW32(255 - f, 255 , 0 , w);
case 2: return RGBW32(0 , 255 , f , w);
case 3: return RGBW32(0 , 255 - f, 255 , w);
case 4: return RGBW32(f , 0 , 255 , w);
case 5: return RGBW32(255 , 0 , 255 - f, w);
default: return 0;
}Everything else is cosmetics. |
|
Well force-pushing was the cleanest way to get this PR up to date, because it contained commits from yourself that were not supposed to be merged anymore. As I said, both rainbows still exist, one in the form of an hue sweep (color_wheel), the other in the form of the rainbow palette, which is (or used to be) identical to hsv2rgb_rainbow. I don't see a reason for a new PR, but I can of course create one if that makes you happy. I would however first like to know if we can agree on a solution, otherwise I don't need to open yet another PR. |
"I was playing around with some rainbow effects and was wondering why there was no real yellow color" was your initial statement, that is what the rainbow palette provides and the technical HSV2RGB does not, at least from your images shown above.
that is not what I was proposing. I will try to implement it. |
|
I ran some tests on this matter and here are my insights:
my code from #4798 has a too aggressive hue preservation, if I fix that, the gamma correction works very well in all cases, I currently see no need for the "apply gamma after effects" option I proposed earlier. Bottom line is: I do agree with the overall proposed changes in this PR:
I would propose one change: instead of "copying" the already existing hsv2rgb code just use the function. Its slightly slower but in practice I would not expect it to matter much as its at least as fast as Here is my proposed solution: uint32_t Segment::color_wheel(uint8_t pos) const {
if (palette) return color_from_palette(pos, false, false, 0); // only wrap if "always wrap" is set
pos = 255 - pos; // legacy color order
uint8_t w = W(getCurrentColor(0));
uint32_t rgb;
hsv2rgb(CHSV32(pos, 255, 255), rgb);
return rgb | (w << 24); // add white channel
}Also corrected the comment about wrapping. |
|
Sounds good to me, assuming that version of However, the direction of the hue gradient should match the original color_wheel, not the rainbow color wheel IMO. I.e. remove As a side note I just noticed |
|
you can update this PR if you like, since its already 95% there. I believe its correct the way it is now. But please test it. since when is a constructor overload a questionable construct? |
This is the current result of the discussion in wled#3681
|
Done, tested, looks fine to me. Anything that makes a reader stop and wonder or look something up is questionable. An overload isn't questionable on its own, but providing more than one overload for an integer type is problematic, especially if both expect different value ranges. If both expected the range 0-255 it would be fine, but there would also be no point in providing both. If we had strong type guarantees that would also be fine, but c++ casts freely and implicitly between many primitive types. You could of course provide some 8 and 16 bit range types that don't allow implicit casting, to make this safe, but that would just complicate things. Here are some examples where it's not immediately obvious or even misleading which function is called: CHSV32(255, s, v);
int h;
CHSV32(h, s, v);
uint8_t h;
CHSV32(2*h, s, v);
float h;
CHSV32(h * 255, s, v); |
did you also test the color order against the original? re: overload: ah, I see. what's the best alternative? a wrapper? or leave it to the caller and not provide it at all? IIRC my idea was to be 1:1 compatible to CHSV but that may have been a bad idea to start with. |
yes, it's the same order
I just noticed that none of the above examples compile, precisely because they are ambiguous, but I guess you get the gist ^^ Edit: you could also provide just the 16 bit constructor and allow implicit casting from CHSV, but I don't think that would improve anything |
compared against main? I remember it being flipped. That would mean hsv2rgb is also flipped against the _rainbow version of fastled. Or I remember incorrectly. So I think I will remove the ambiguous overload at one point. Can you therefore please provide a 16bit value so its one less thing to clean up later? |
the rainbow color wheel was flipped, because the
Ok ^^ |
|
@DedeHai you are being inconsistent. In #4798 you are arguing for speed and reimplementing If you insist on preferring speed over compact code, please be consistent. @TripleWhy 's implementation of HSV to RGB was faster as it didn't deal with S & V. |
|
I know I am being inconsistend and I gave the reason why I think its acceptable. Please read it again. |
4 participants
I was playing around with some rainbow effects and was wondering why there was no real yellow color. Cyan and magenta are also mysteriously underrepresented. I think rainbow effects should use the full rainbow spectrum.
If you agree, that is what this PR does.
Here are two images displaying the Rainbow effect on 256 LEDs, captured using the web UI:
Top: 0_15 (874b24f)
Bottom: mine (760a1d4)
This works by replacing the linear transitions R -> G -> B in
color_wheelby the HSV transition 0° -> 360°.Summary by CodeRabbit
New Features
Bug Fixes
Refactor