-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
use HCL color space (Rec.2020 for luma) instead of HSV #1235
Conversation
✅ Deploy Preview for lambent-marshmallow-4057de canceled.
|
✅ Deploy Preview for conkyweb canceled.
|
First of all, thanks for doing this. Is there a reason why you removed the HSV code? I think it would make sense to add HCL, which does indeed look better to me, and keep HSV so it doesn't break existing configs. It seems like it should be fairly straight forward to have both HSV and HCL. What do you think? |
It indeed is quite easy to have them both. My issue with the HSV code is that it has some wrong calculations.
So my line of reasoning was that I could either correct the HSV code or scrap it to use a better color space (specially for HDR monitors that have really bright magentas). PS: Found out conversions are also wrong. Decimal scale conversions add and subtract |
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 looks good, thanks for fixing HSV gradient.
Now we have 3 different ways to colour the graphs, so we should probably add a config setting and document the available methods. Something like graph_griadent_method = (rgb|hsv|hcl)
. Do you want to take a stab at that?
Also, please unstage tests/catch2/catch.hpp
, that's a 3rd party library and I'd like to keep the original.
That seems nice. I’ll try 😊 Can I fix those errors I mentioned on HSV or should I leave it as it is? And I will unstage the header! Sorry! |
Yeah, that would be great! |
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.
Overall this looks much better. Thanks for combining all the code into one file, I think that's cleaner.
It looks like the tests were removed, I'm not sure if that was intentional or not, but we keep & update the tests so they work with this code. I don't generally require people include tests with submissions, but we shouldn't be removing tests.
- name: graph_gradient_mode | ||
desc: |- | ||
Changes the color space used for interpolation. Arguments are hcl, hsv, | ||
and rgb (default). |
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.
Can you add args
here, like:
args:
- (rbg|hcl|hsv)
@@ -68,6 +68,8 @@ set(conky_sources | |||
exec.h | |||
fs.cc | |||
fs.h | |||
gradient.cc |
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.
Since we're including the sources like this, we can remove all references to BUILD_HSV_GRADIENT
from the CMake files and source. The build option is defined in ConkyBuildOptions.cmake.
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.
Sweet, looks good! Thanks for your patience, and I think we're good to go once all the checks pass.
and rgb (default). | ||
args: | ||
- (rbg|hcl|hsv) | ||
default: rgb |
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.
Ah yes, good catch. Thanks!
@@ -267,8 +267,6 @@ option(BUILD_PULSEAUDIO | |||
option(BUILD_INTEL_BACKLIGHT | |||
"Enable support for Intel backlight" false) | |||
|
|||
option(BUILD_HSV_GRADIENT "Enable gradient in HSV colour space" true) |
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.
Thanks!
Hey, could you explain what is the |
Good question, in this case The full command should be as follows: cmake .. \
-DBUILD_AUDACIOUS=ON \
-DBUILD_HTTP=ON \
-DBUILD_ICAL=ON \
-DBUILD_ICONV=ON \
-DBUILD_IRC=ON \
-DBUILD_IRC=ON \
-DBUILD_JOURNAL=ON \
-DBUILD_LUA_CAIRO=ON \
-DBUILD_LUA_IMLIB2=ON \
-DBUILD_LUA_RSVG=ON \
-DBUILD_MYSQL=ON \
-DBUILD_NVIDIA=ON \
-DBUILD_PULSEAUDIO=ON \
-DBUILD_RSS=ON \
-DBUILD_TESTS=ON \
-DBUILD_WLAN=ON \
-DBUILD_X11=OFF \
-DBUILD_XMMS2=ON \
-DCMAKE_C_COMPILER=$CC \
-DCMAKE_CXX_COMPILER=$CXX \
-DMAINTAINER_MODE=ON |
Oh I guess you can also expand the substituted commands if you click the |
Are you still working on this PR? I think we're very close to completing it. |
I was. Sorry for the delay! D: |
|
||
void gradient_factory::setup_colour_depth() { | ||
#ifdef BUILD_X11 | ||
if (state == nullptr) { // testing purposes |
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 is okay for now, but it would be better to initialize the state with appropriate values as part of the setup for the tests.
Description
HSV Gradient brought some unpleasant colors (either too bright or too saturated) with my conky config because of how the HSV color space works. I changed it to HCL (Hue-Chroma-Luma) so that colors blend with a better distribution of luminosity.
Using HCL color space:
Using HSV color space: