use HCL color space (Rec.2020 for luma) instead of HSV #1235

caiofreitaso · 2022-10-04T23:50:29Z

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:

netlify · 2022-10-04T23:50:57Z

✅ Deploy Preview for lambent-marshmallow-4057de canceled.

Name	Link
🔨 Latest commit	`fae9aa5`
🔍 Latest deploy log	https://app.netlify.com/sites/lambent-marshmallow-4057de/deploys/633cc6c7495f7e0008970825

netlify · 2022-10-04T23:51:18Z

✅ Deploy Preview for conkyweb canceled.

Name	Link
🔨 Latest commit	`99709da`
🔍 Latest deploy log	https://app.netlify.com/sites/conkyweb/deploys/634743ee99df7e000846a3ef

brndnmtthws · 2022-10-05T01:58:59Z

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?

caiofreitaso · 2022-10-05T03:28:52Z

It indeed is quite easy to have them both. My issue with the HSV code is that it has some wrong calculations.

H' is assigned as (diff + chroma/2)/chroma + offset, when this chroma/2 shouldn't be there.
Saturation has a similar problem: (chroma + value/2)/value. value/2 also shouldn't be there.
Chroma at scaled_hsv_to_scaled_rgb is calculated as value * saturation + 0.5, when it should have been value * saturation.
X is calculated with a Hue tilted 30°, and increased by 0.5: (chroma * (1 - abs((H-30)/60 % 2 - 1)) + 0.5, when it should have been chroma * abs((H/60) % 2 - 1)
The way increments are calculated also use (diff + divisor/2)/divisor instead of diff/divisor for some reason, and hues are rotated unnecessarily.

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 max_value - 1 and 0.5, for no apparent reason.

After correcting them, this is the result for the gradient:

brndnmtthws

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.

caiofreitaso · 2022-10-05T14:14:39Z

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!

brndnmtthws · 2022-10-05T15:15:51Z

Can I fix those errors I mentioned on HSV or should I leave it as it is?

Yeah, that would be great!

caiofreitaso · 2022-10-06T06:26:38Z

Done! Now the config file can declare which mode with graph_gradient_mode = 'hcl', for example.

RGB:

HSL:

HCL:

brndnmtthws

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.

brndnmtthws · 2022-10-06T13:01:13Z

doc/config_settings.yaml

+  - name: graph_gradient_mode
+    desc: |-
+      Changes the color space used for interpolation. Arguments are hcl, hsv,
+      and rgb (default).


Can you add args here, like:

args: - (rbg|hcl|hsv)

brndnmtthws · 2022-10-06T13:04:47Z

src/CMakeLists.txt

@@ -68,6 +68,8 @@ set(conky_sources
    exec.h
    fs.cc
    fs.h
+    gradient.cc


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.

brndnmtthws

Sweet, looks good! Thanks for your patience, and I think we're good to go once all the checks pass.

brndnmtthws · 2022-10-06T16:09:55Z

doc/config_settings.yaml

+      and rgb (default).
+    args:
+      - (rbg|hcl|hsv)
+    default: rgb


Ah yes, good catch. Thanks!

brndnmtthws · 2022-10-06T16:10:38Z

cmake/ConkyBuildOptions.cmake

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


caiofreitaso · 2022-10-06T18:29:56Z

Hey, could you explain what is the ON and OFF that appear on those builds? I want to reproduce the errors here, but I can't understand what exactly those are. Is it a no GUI or no X11 flag? D:

brndnmtthws · 2022-10-06T18:36:56Z

Good question, in this case OFF is referring to the BUILD_X11 flag. I will update the build so it prints the build command.

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

brndnmtthws · 2022-10-06T18:44:51Z

Oh I guess you can also expand the substituted commands if you click the Run ... log line at the top of the step.

brndnmtthws · 2022-10-11T16:13:06Z

Are you still working on this PR? I think we're very close to completing it.

caiofreitaso · 2022-10-12T00:23:59Z

Are you still working on this PR? I think we're very close to completing it.

I was. Sorry for the delay! D:

brndnmtthws · 2022-10-13T01:35:26Z

src/gradient.cc

+
+void gradient_factory::setup_colour_depth() {
+#ifdef BUILD_X11
+  if (state == nullptr) { // testing purposes


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.

use LCh color space (Rec.2020 for luma)

fae9aa5

github-actions bot added sources PR modifies project sources tests Issue or PR related to project tests labels Oct 4, 2022

Merge branch 'main' into use-chroma-luma

0ce692c

caiofreitaso added 3 commits October 4, 2022 21:08

explicit L

829e2d2

merge origin

da5d750

use clang-format

6f42a8e

fix decimal conversions

d609c57

caiofreitaso added 3 commits October 5, 2022 00:51

restore hsv_gradient

e192f47

restore tests

b3ca0f4

Merge branch 'main' into use-chroma-luma

251eb58

brndnmtthws requested changes Oct 5, 2022

View reviewed changes

caiofreitaso added 3 commits October 6, 2022 03:08

refactor gradients to use config value instead of hard-coded option

4148e83

merge origin

4d71569

remove hsv_gradient

34329c7

Merge branch 'main' into use-chroma-luma

ed68b78

brndnmtthws requested changes Oct 6, 2022

View reviewed changes

caiofreitaso added 3 commits October 6, 2022 13:06

PR changes; fix saturation calculation (scale before dividing)

4586fd3

fix docs

5bcdf8d

merge origin

c940ea8

github-actions bot added the documentation Issue or PR that suggests documentation improvements label Oct 6, 2022

brndnmtthws approved these changes Oct 6, 2022

View reviewed changes

fix delete abstract with non-virtual dtor

20957cb

caiofreitaso added 2 commits October 6, 2022 13:35

fix unused parameter

e106de5

fix undeclared identifier

d1d5f01

caiofreitaso added 2 commits October 11, 2022 20:30

use shift for masks; fix tests for 16-bit color depth

65f264b

Merge branch 'main' into use-chroma-luma

60ae018

caiofreitaso added 3 commits October 12, 2022 19:27

add special case for X11 tests

9a544ba

merge origin

b43d3bc

remove include

99709da

brndnmtthws reviewed Oct 13, 2022

View reviewed changes

brndnmtthws merged commit 43a1043 into brndnmtthws:main Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use HCL color space (Rec.2020 for luma) instead of HSV #1235

use HCL color space (Rec.2020 for luma) instead of HSV #1235

caiofreitaso commented Oct 4, 2022

netlify bot commented Oct 4, 2022 •

edited

Loading

netlify bot commented Oct 4, 2022 •

edited

Loading

brndnmtthws commented Oct 5, 2022

caiofreitaso commented Oct 5, 2022

brndnmtthws left a comment

caiofreitaso commented Oct 5, 2022

brndnmtthws commented Oct 5, 2022

caiofreitaso commented Oct 6, 2022

brndnmtthws left a comment

brndnmtthws Oct 6, 2022

brndnmtthws Oct 6, 2022

brndnmtthws left a comment

brndnmtthws Oct 6, 2022

brndnmtthws Oct 6, 2022

caiofreitaso commented Oct 6, 2022

brndnmtthws commented Oct 6, 2022

brndnmtthws commented Oct 6, 2022

brndnmtthws commented Oct 11, 2022

caiofreitaso commented Oct 12, 2022

brndnmtthws Oct 13, 2022

use HCL color space (Rec.2020 for luma) instead of HSV #1235

use HCL color space (Rec.2020 for luma) instead of HSV #1235

Conversation

caiofreitaso commented Oct 4, 2022

Description

netlify bot commented Oct 4, 2022 • edited Loading

✅ Deploy Preview for lambent-marshmallow-4057de canceled.

netlify bot commented Oct 4, 2022 • edited Loading

✅ Deploy Preview for conkyweb canceled.

brndnmtthws commented Oct 5, 2022

caiofreitaso commented Oct 5, 2022

brndnmtthws left a comment

Choose a reason for hiding this comment

caiofreitaso commented Oct 5, 2022

brndnmtthws commented Oct 5, 2022

caiofreitaso commented Oct 6, 2022

brndnmtthws left a comment

Choose a reason for hiding this comment

brndnmtthws Oct 6, 2022

Choose a reason for hiding this comment

brndnmtthws Oct 6, 2022

Choose a reason for hiding this comment

brndnmtthws left a comment

Choose a reason for hiding this comment

brndnmtthws Oct 6, 2022

Choose a reason for hiding this comment

brndnmtthws Oct 6, 2022

Choose a reason for hiding this comment

caiofreitaso commented Oct 6, 2022

brndnmtthws commented Oct 6, 2022

brndnmtthws commented Oct 6, 2022

brndnmtthws commented Oct 11, 2022

caiofreitaso commented Oct 12, 2022

brndnmtthws Oct 13, 2022

Choose a reason for hiding this comment

netlify bot commented Oct 4, 2022 •

edited

Loading

netlify bot commented Oct 4, 2022 •

edited

Loading