GNOME Bugzilla – Bug 749902
Add Hue-Chroma operation/tool and LCH color selector
Last modified: 2017-07-16 13:22:08 UTC
Created attachment 304005 [details] [review] patch to replace HSV with LCH in the color picker dialog To make full use of the LCH blend modes and the ability to manipulate LCH instead of RGB, the GIMP color picker should allow to pick LCH colors. In addition, the Hue-Saturation, Color Balance, and Colorize tools should allow to use LCH, but this patch is only for the color picker. This patch very wrongly puts the RGB/LCH code back in libgimpcolor/colorspace.c. I haven't figured out how to use the code in CIE.c. The patch probably only works for linear gamma sRGB images, at linear precision. But I don't know for sure because I can't make sense of the color picker values for GIMP with the babl flips enabled. What Massimo did for the LCH blend modes would also need to be done for the LCH color picker code. If the babl flips are disabled, the patch works for linear gamma sRGB images regardless of the precision. Using the LCH and RGB sliders does work. Picking a color from the color box and the "formerly HSV triangle" doesn't work. I wasn't able to decipher the code for color picking from the box or "formerly HSV triangle" which should now be an LCH triangle. So I just did a very blunt and obviously wrong "find and replace". At floating point precision, unclipped RGB values become a problem and the LCH and RGB slider values can easily be out of step if changes to the LCH sliders causes any of the RGB sliders to reach 0 or 255. This is part of a larger problem with the color picker dialog being out of step with picked colors that are out of gamut with respect to the RGB working space. This patch replaces the color picker HSV sliders with LCH sliders. If the decision were made to eliminate HSV from the GIMP code base, then this patch would be a start in that direction. If the decision were made to carry LCH and HSV side by side, then the color picker layout would need to be modified, and I don't know how to do that. Screenshots of the LCH/RGB color picker are here: http://ninedegreesbelow.com/bug-reports/gimp-lch-color-picker.html
Thanks, that's a great start. We cannot change the libgimp API during 2.x, but HSV is certainly on its way out, setting milestone to 3.0.
Created attachment 305938 [details] [review] updated patch 1 of 4: add lch to colorspace.c
Created attachment 305939 [details] [review] updated patch 2 of 4: remove color picker panels
Created attachment 305940 [details] [review] updated patch 3 of 4: replace hsv with lch in color picker
Created attachment 305941 [details] [review] updated patch 4 of 4: replace hue-saturation tool with hue-chroma tool
The attached four patches replace the original patch and work with the latest GIMP from git. Patch 3 of 4 makes the LCH panels works in the color picker (simple fix to the original patch). Patch 4 of 4 replaces the HSV hue-saturation tool with an LCH hue-chroma tool. This last patch only works with the master hue. I couldn't figure out the code for changing hue ranges.
Some comments on an LCH color picker: * In HSV, all the color combinations of H, S, and V represent valid colors in your chosen RGB working space. * In LCH, many combinations of L, C, and H represent colors that are out of gamut with respect to your chosen RGB working space. * In LCH, many combinations L, C, and H represent colors that are not real colors that you could actually see out there in the real world. So it seems to me that an artist would need two very different "guides" overlaid on top of the LCH square of colors: * An outline enclosing all possible LCH real world colors. * A second outline enclosing all possible LCH colors that are in gamut with respect to the user's chosen RGB working space. Most of the time users would probably want to only pick colors that are inside the intersection of "all possible real world colors" and "all colors that are in gamut with respect to the user's chosen RGB working space". I don't think there should be any "stops" to keep people from picking unreal or out of gamut colors, but at least there should be a way for the user to see whether the picked color is real or unreal, and in or out of gamut. Also, on the LCH color picker and hue-chroma tool, I set the "chroma" scale to run from 0 to 200. The "200" was an arbitrary number. Some real world colors have chroma values that are greater than 200. Checking several large-gamut color spaces that have real primaries, the highest chroma I could find was WideGamutRGB greenest green, with a chroma of 236.
Created attachment 315417 [details] [review] color-picker.patch I rebased these patches on top of yesterday's master and added three that use babl instead of the conversions in libgimpcolor (that are removed), preserve the H C L order of the sliders in the color-picker dialog so it was easier to keep the color box in sync with the sliders and reintroduced the color-wheel-selector. Few incompatible hunks have been deleted from gimp-lch3 patch, so now it compiles only after the whole patchset is applied git am color-picker.patch Anyway this is somewhat premature now. If you select an lch color whose corresponding rgb triple is outside of [0 255]^3 GIMP warns and either refuses to set the context fg/bg color or clips it. When displaying the colors in the color box, instead of the babl conversion to sRGB, probably it would be better to use a lcms2 transform to the screen color space on a non OS color managed window/surface otherwise colors outside sRGB gamut are clamped to non corresponding in gamut colors.
Created attachment 315776 [details] [review] Puts all the patches including Massimo's into one patch
(In reply to Massimo from comment #8) > Created attachment 315417 [details] [review] [review] > color-picker.patch > > I rebased these patches on top of yesterday's master and > added three that use babl instead of the conversions in > libgimpcolor (that are removed), preserve the > H C L order of the sliders in the color-picker dialog so > it was easier to keep the color box in sync with the > sliders and reintroduced the color-wheel-selector. Massimo, thanks! for making the color-picker.patch. I had tried but not succeeded in fixing the order of the sliders. > If you select an lch color whose corresponding rgb triple > is outside of [0 255]^3 GIMP warns and either refuses to > set the context fg/bg color or clips it. I'm not sure what this means. It's still possible to dial in out of gamut RGB values using the LCH sliders. > When displaying the colors in the color box, instead of the babl > conversion to sRGB, probably it would be better to use a lcms2 > transform to the screen color space on a non OS color managed > window/surface otherwise colors outside sRGB gamut are clamped > to non corresponding in gamut colors. I'm not sure what this means. A matrix multiplication would probably be faster. There are three? two and a half? sets of "RGB to XYZ/LAB/LCH" equations in the babl CIE.c file. I think you used the double precision set? Anyway, the sliders don't move as smoothly as before using the babl code to convert to LCH.
(In reply to Elle Stone from comment #10) > (In reply to Massimo from comment #8) > > Created attachment 315417 [details] [review] [review] [review] > > color-picker.patch > > > > I rebased these patches on top of yesterday's master and > > added three that use babl instead of the conversions in > > libgimpcolor (that are removed), preserve the > > H C L order of the sliders in the color-picker dialog so > > it was easier to keep the color box in sync with the > > sliders and reintroduced the color-wheel-selector. > > Massimo, thanks! for making the color-picker.patch. I had tried but not > succeeded in fixing the order of the sliders. > > > If you select an lch color whose corresponding rgb triple > > is outside of [0 255]^3 GIMP warns and either refuses to > > set the context fg/bg color or clips it. > > I'm not sure what this means. It's still possible to dial in out of gamut > RGB values using the LCH sliders. > Well if I select as foreground color a lch triple that corresponds to an out of sRGB gamut GIMP prints on the console: (gimp-2.9:1): GLib-GObject-WARNING **: value "((GimpRGB*) 0x603000c98e90)" of type 'GimpRGB' is invalid or out of range for property 'color' of type 'GimpRGB' and if I then drag the fg color from the toolbox to a layer with floating point precision, reading the value used to fill the layer it is the clipped one > > When displaying the colors in the color box, instead of the babl > > conversion to sRGB, probably it would be better to use a lcms2 > > transform to the screen color space on a non OS color managed > > window/surface otherwise colors outside sRGB gamut are clamped > > to non corresponding in gamut colors. > > I'm not sure what this means. A matrix multiplication would probably be > faster. > A matrix multiplication if the display color profile is a matrix profile, I don't know if that's always the case for user calibrated screens. > There are three? two and a half? sets of "RGB to XYZ/LAB/LCH" equations in > the babl CIE.c file. I think you used the double precision set? Anyway, the > sliders don't move as smoothly as before using the babl code to convert to > LCH. yes I used the double version because the source/destination is a double. GimpRGB and GimpLch are declared with gdouble members. Probably there is no fast path for double.
(In reply to Massimo from comment #11) > > I'm not sure what this means. It's still possible to dial in out of gamut > > RGB values using the LCH sliders. > Well if I select as foreground color a lch triple that corresponds > to an out of sRGB gamut GIMP prints on the console: > > (gimp-2.9:1): GLib-GObject-WARNING **: value "((GimpRGB*) 0x603000c98e90)" > of type 'GimpRGB' is invalid or out of range for property 'color' of type > 'GimpRGB' You get those warnings even in default GIMP any time you color pick an out of gamut color. They don't have anything to do with LCH, per se. It would be nice if the glib warnings could be suppressed as they seem pretty useless and clutter up the terminal output. > and if I then drag the fg color from the toolbox to a layer > with floating point precision, reading the value used to fill the > layer it is the clipped one Interesting. I've never dragged the fg color from the toolbox to a layer. But you are right, the values are clipped. If you fill the layer using the Bucket Fill tool, the values are not clipped. > > > When displaying the colors in the color box, instead of the babl > > > conversion to sRGB, probably it would be better to use a lcms2 > > > transform to the screen color space on a non OS color managed > > > window/surface otherwise colors outside sRGB gamut are clamped > > > to non corresponding in gamut colors. > > > > I'm not sure what this means. A matrix multiplication would probably be > > faster. > > A matrix multiplication if the display color profile is a matrix > profile, I don't know if that's always the case for user calibrated > screens. Hmm, I thought you meant something else. Isn't LCMS always used to send RGB values to the monitor? Has a conversion to sRGB been put somewhere in the path?
(In reply to Elle Stone from comment #12) ... > > A matrix multiplication if the display color profile is a matrix > > profile, I don't know if that's always the case for user calibrated > > screens. > > Hmm, I thought you meant something else. Isn't LCMS always used to send RGB > values to the monitor? Has a conversion to sRGB been put somewhere in the > path? Well I thought only the image on canvas was color managed, but recently there was work also for thumbnails and something else so I'm not sure if the color picker is already color managed. Anyway the color box is drawn filling a 8bit buffer and I converted the lch values to sRGB as babl doesn't know anything of display color profiles.
Created attachment 316082 [details] [review] Fixes hue-chroma slider ranges Patch 315776 made the slider value ranges for the hue-saturation tool off by multiples of 100 or 360, depending on the slider. This patch includes a fix for the slider value ranges and also renames the hue-saturation tool as "hue-chroma".
Created attachment 316084 [details] [review] patch to show out of gamut colors and limit picking a color to in-gamut colors This patch should be applied after applying 316082. This patch makes the horizontal LCH sliders show solid black for all out of gamut colors and also for any color for which R, G, or B equals either 0 or 255. The relevant variable was guchar. To not exclude these "almost out of gamut" colors, somewhat more invasive code changes are required. The patch also constrains colors that are picked to be in gamut, even if the user dials in out of gamut colors. Sample points and the pointer still allow to read the values for out of gamut colors (as produced by Levels, LCH blend modes, etc). It would be nice if the square LCH color picker panel also showed black to indicate all out of gamut colors, but I haven't figured out where in the code the colors are sent to the square color picker panel. The other panels (CMYK and etc) either don't work or are difficult to use, depending on the panel.
(In reply to Elle Stone from comment #12) > (In reply to Massimo from comment #11) > > > I'm not sure what this means. It's still possible to dial in out of gamut > > > RGB values using the LCH sliders. > > > Well if I select as foreground color a lch triple that corresponds > > to an out of sRGB gamut GIMP prints on the console: > > > > (gimp-2.9:1): GLib-GObject-WARNING **: value "((GimpRGB*) 0x603000c98e90)" > > of type 'GimpRGB' is invalid or out of range for property 'color' of type > > 'GimpRGB' > > You get those warnings even in default GIMP any time you color pick an out > of gamut color. They don't have anything to do with LCH, per se. > > It would be nice if the glib warnings could be suppressed as they seem > pretty useless and clutter up the terminal output. > ... these warnings are generated here: https://git.gnome.org/browse/gimp/tree/libgimpcolor/gimprgb.c#n709 properties of type GimpRGB are validated by 'gimp_rgb_clamp'ing them to [0 1]. If you want to suppress the warnings change that function to return FALSE unconditionally. I don't know if there are bad side effects beside the context back/foreground being out of gamut
Would be great to see this committed.
Some of these patches change API and can't go in, we can only extend API for 2.10, not replace it.
(In reply to Michael Natterer from comment #18) > Some of these patches change API and can't go in, we can only extend > API for 2.10, not replace it. Would it be possible to add LCH as a separate panel in the current Color Picker, instead of make LCH a straight-up replacement for HSV (which is what the current patches do)? Would this qualify as extending rather than replacing the 2.10 API? I'm not sure what an API is.
Yes of course, LCH and/or LAB would be very cool to have in some places in 2.10. Also, I'll look into applying your changes to libgimpcolor, all of which only seem to add new API, and are no problem at all.
That would be very awesome :)
Comment on attachment 305938 [details] [review] updated patch 1 of 4: add lch to colorspace.c This patch only adds libgimpcolor API and thus should be ok to apply, according to mitch in comment 20.
(In reply to Michael Schumacher from comment #22) > Comment on attachment 305938 [details] [review] [review] > updated patch 1 of 4: add lch to colorspace.c > > This patch only adds libgimpcolor API and thus should be ok to apply, > according to mitch in comment 20. The RGB/XYZ/LAB/LCH code in attachment 305938 [details] [review] has been moved to babl's extensions/CIE.c. So I think all the patches need to be rewritten, and likely need to be rewritten anyway given various code changes over the last year.
Thanks. I'll mark all of them as needs-work for now.
(In reply to Michael Schumacher from comment #24) > Thanks. I'll mark all of them as needs-work for now. I'm working on a patch to add the Hue-Chroma color tool. The code compiles and adds the tools, and the LCH sliders all work, in the limited sense that the color changes are "in the right direction". But the resulting colors aren't the right colors, for example a -10 adjustment in Lightness doesn't actually make a color with LCH Hue and Chroma the same and LCH Lightness reduces by 10. When I fix the Hue-Chroma LCH sliders to actually work correctly, I'll upload a patch for the Hue-Chroma tool and then start working on a patch for adding the LCH color sliders.
(In reply to Elle Stone from comment #25) > (In reply to Michael Schumacher from comment #24) > > Thanks. I'll mark all of them as needs-work for now. > > I'm working on a patch to add the Hue-Chroma color tool. The code compiles > and adds the tools, and the LCH sliders all work, in the limited sense that > the color changes are "in the right direction". But the resulting colors > aren't the right colors, for example a -10 adjustment in Lightness doesn't > actually make a color with LCH Hue and Chroma the same and LCH Lightness > reduces by 10. Hmm, well, I overestimated my coding abilities. I don't know why the Hue-Chroma tool doesn't work, but I'm going to attach the patch anyway. At least it does successfully add a Hue-Chroma tool, even though the tool doesn't produce correct colors. > When I fix the Hue-Chroma LCH sliders to actually work correctly, I'll > upload a patch for the Hue-Chroma tool and then start working on a patch for > adding the LCH color sliders. It turns out I don't have any clue how to *add* lch to the color picker while keeping hsv. I could *replace* lch with hsv, though that doesn't mean the color sliders would work correctly.
Created attachment 346378 [details] [review] Add an LCH Hue-Chroma tool This patch *adds* an LCH Hue-Chroma tool. The tool "works" in that the sliders do modify the colors, and even in the right direction. But the resulting colors are not the correct colors.
Created attachment 346474 [details] [review] Patch for an LCH hue-chroma tool that does produce correct colors This patch adds an LCH Hue-Chroma tool that produces correct colors and has fewer autogenerated files. But the code still processes all the pixels twice, and still has non-functioning placeholder code for modifying just a specified range of hues.
Created attachment 346479 [details] [review] Patch to replace HSV color pickers with LCH color pickers This patch must be applied after applying attachment 346474 [details] [review] Patch for an LCH hue-chroma tool that does produce correct colors. This patch *replaces* HSV with LCH in the color picking tools What is really wanted is to *add* LCH to the color picking tools, instead of wholesale replacing HSV, but I don't know how to do this. Also this code likely needs a prepare () function to avoid repeatedly processing the entire buffer. Possibly the code might need additional code to switch between RGB(A) and R'G'B'(A), but it seems to run just fine only calling for R'G'B'(A). Also because this patch removes HSV from the color pickers, several color picker panels (CMYK, WaterWheel, etc) had to be disabled in the modules Makefile.am to allow the code to compile.
Created attachment 346512 [details] [review] Hue-Chroma tool using prepare () This patch uses prepare () in gimp_operation_hue_chroma_process. Should it also use prepare () in gimp_operation_hue_chroma_map?
Created attachment 347268 [details] [review] Removes Range and Overlap from UI, but not from the actual code I don't know how to add code to the Hue-Chroma tool that modifies only a range of L, C, and/or H. This updated patch removes all references to "range" and "overlap" from the Hue-Chroma tool User Interface. But I haven't figured out how to remove all references to "range" and "overlap" from the actual code, which was patterned after the HSV Hue-Saturation tool. I sort of doubt whether I'll ever figure out how to remove all references to "range" and "overlap" from the actual code. The Hue-Chroma tool is incredibly useful even without the option to modify by range (which personally I've never missed having, and I do use the Hue-Chroma tool a lot). To be truly 100% useful, the LCH blend modes do need an LCH color picker and an LCH Hue-Chroma tool (just as the HSV blend modes have an HSV color picker and an HSV Hue-Saturation tool).
Created attachment 351218 [details] [review] Updated patch to add hue-chroma tool This patch updates the LCH Hue-Chroma patch to work with GIMP code through commit 7f33edea1b9e8d1876eef3b1eea091406895e722. If this patch is accepted I'll work on a new patch that adds an LCH color picker panel. Patch 346479 no longer applies and just replaces HSV rather than adding LCH as an additional option.
Created attachment 351253 [details] [review] hue-chroma tool patch Sorry, the previous patch accedentally included a folder of symbolic-inverted icons, not sure how that happened given that I already had remade the patch to avoid accidentally including those icons . . .
Thanks for the updated patch, it applies nicely :) However, there is still the issue of "range" and "overlap", we have two choices: - drop them completely, in which case the op can go right to GEGL and the GIMP part becomes a few-liner to add the GEGL op to the menus. - implement them and keep the op in GIMP and have a separate tool for it, because or the arrays of config values.
(In reply to Michael Natterer from comment #34) > Thanks for the updated patch, it applies nicely :) > > However, there is still the issue of "range" and "overlap", we have > two choices: > > - drop them completely, in which case the op can go right to GEGL > and the GIMP part becomes a few-liner to add the GEGL op to the > menus. > > - implement them and keep the op in GIMP and have a separate tool for > it, because or the arrays of config values. Eventually having something like range/overlap would be nice. But the same thing can be accomplished using a layer mask, if we also have a LCH color picker. Except that the current code for picking a range of colors by hue/color/value/red/green/etc using the color picker doesn't work very well - there is no gradual drop-off/exclusion/inclusion of colors such as is provided by PhotoShop's "pick color range" tool (https://helpx.adobe.com/photoshop/using/selecting-color-range-image.html). I've been thinking about how to implement picking colors by range with something like a linear or bell-curve drop-off with a controllable slope and end-points. This would make hue-chroma range/overlap completely controllable by a mask, allowing for the kind of subsequent modifications/fine-tuning you can't do with "one time range/overlap settings" in a hue-chroma or hue-saturation tool. So personally I'd say drop the range/overlap completely and make it a GEGL op if that's easier, and leave the functionality of "range/overlap" as an enhancement for the color picking tools.
About selecting color ranges, have you looked at pixel_difference() in gimppickable-contiguous-region.c? We can easily extend the enum to allow for other "color channel" types.
(In reply to Michael Natterer from comment #36) > About selecting color ranges, have you looked at pixel_difference() > in gimppickable-contiguous-region.c? We can easily extend the enum > to allow for other "color channel" types. I looked at that code, but I'm not what it does, would have to do some testing and adding comments to see when it's activated. The PS range color picker allows for selections and masks that have shades of gray indicating the "degree of matching" the target color. As far as I can tell using GIMP, the only way to get shades of gray when selecting by color is to use "Feather edges". But "Feather edges" can and usually does include colors that are quite outside the range of desired colors. Well, sometimes there are patches of shades of gray when not using "Feather edges", but the placement seems more or less random - most edges are hard edges, "all or nothing". Regarding the LCH Hue-Chroma tool, is there anything I can/should do to help along the process of getting the code committed to GIMP or GEGL?
I have already hacked up the GEGL op and its GIMP part. About selecting selecting things that match a color, that's what "Select by color" does, and color similarity is determined by the "Select Criterion" combo, and the code for it is in pixel_difference()
The GEGL op: commit 78c84f94a6633c5d79503d2c1de8125b307c705f Author: Michael Natterer <mitch@gimp.org> Date: Sun May 7 18:26:35 2017 +0200 Bug 749902 - Add Hue-Chroma operation/tool and LCH color selector Add hue-chroma operation. operations/common/Makefile.am | 3 +- operations/common/hue-chroma.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ po/POTFILES.in | 1 + 3 files changed, 117 insertions(+), 1 deletion(-)
And adding it to the GIMP menus: commit e5e9e4b6c8851b13f88b0e707f1de3e2078e305b Author: Michael Natterer <mitch@gimp.org> Date: Sun May 7 18:32:45 2017 +0200 Bug 749902 - Add Hue-Chroma operation/tool and LCH color selector Add gegl:hue-chroma to the "Colors" menu. app/actions/filters-actions.c | 6 ++++++ app/sanity.c | 1 + app/tools/gimpgegltool.c | 1 + app/widgets/gimphelp-ids.h | 1 + menus/image-menu.xml.in | 1 + 5 files changed, 10 insertions(+)
(In reply to Michael Natterer from comment #40) > And adding it to the GIMP menus: Thanks hugely much! That seems to work perfectly!
Comment on attachment 351253 [details] [review] hue-chroma tool patch I'm sorry that large patch is now obsolete, but it is...
(In reply to Michael Natterer from comment #42) > Comment on attachment 351253 [details] [review] [review] > hue-chroma tool patch > > I'm sorry that large patch is now obsolete, but it is... I'm not sorry! I'm delighted! Now on to the LCH color picking tool! for which I'm working on a rewrite that keeps HSV and adds LCH.
Created attachment 351384 [details] Possible locations in UI for LCH color picker sliders Before working further on code to add the LCH color picker, it seemed like a good idea to ask for feedback as to a preferred location in the color picking UIs (FG/BG, Change Foreground/Background). The attached jpeg shows two possible placements for adding LCH color sliders. It also shows what the LCH sliders look like in my patched "anyrgb" version of GIMP.
I think we should initially go for the bottom version and simply add the LCH color sliders, until everything works fine. Then we can decide what to do with the HSV ones.
In the meantime, one more place to support LCH: commit 2766a9f807ba0bfe19f06d2a0844fa19452a9cd6 Author: Michael Natterer <mitch@gimp.org> Date: Mon May 8 21:33:04 2017 +0200 app: add an LCH mode to GimpColorFrame so picked colors can be displayed as LCH now. app/widgets/gimpcolorframe.c | 28 ++++++++++++++++++++++++++++ app/widgets/widgets-enums.c | 2 ++ app/widgets/widgets-enums.h | 1 + 3 files changed, 31 insertions(+)
(In reply to Michael Natterer from comment #45) > I think we should initially go for the bottom version and simply > add the LCH color sliders, until everything works fine. Then we > can decide what to do with the HSV ones. OK, thanks!
(In reply to Michael Natterer from comment #46) > In the meantime, one more place to support LCH: > > commit 2766a9f807ba0bfe19f06d2a0844fa19452a9cd6 > Author: Michael Natterer <mitch@gimp.org> > Date: Mon May 8 21:33:04 2017 +0200 > > app: add an LCH mode to GimpColorFrame > > so picked colors can be displayed as LCH now. Thank you!
And more (slooooow): commit 3f420614ffe4f751341381fc00ab1d6d5437df90 Author: Michael Natterer <mitch@gimp.org> Date: Tue May 9 22:02:19 2017 +0200 app, libgimpbase: allow to select colors by CIE L, C, and H Add the additional enum values to enum GimpSelectCriterion, and the few needed lines to gimppickable-contiguous-region.c. It's horribly slow, but works. app/core/gimppickable-contiguous-region.c | 19 +++++++++++++++++++ libgimpbase/gimpbaseenums.c | 6 ++++++ libgimpbase/gimpbaseenums.h | 19 +++++++++++-------- tools/pdbgen/enums.pl | 10 ++++++++-- 4 files changed, 44 insertions(+), 10 deletions(-)
(In reply to Michael Natterer from comment #49) > And more (slooooow): > > commit 3f420614ffe4f751341381fc00ab1d6d5437df90 > Author: Michael Natterer <mitch@gimp.org> > Date: Tue May 9 22:02:19 2017 +0200 > > app, libgimpbase: allow to select colors by CIE L, C, and H > > Add the additional enum values to enum GimpSelectCriterion, and > the few needed lines to gimppickable-contiguous-region.c. > > It's horribly slow, but works. > > app/core/gimppickable-contiguous-region.c | 19 +++++++++++++++++++ > libgimpbase/gimpbaseenums.c | 6 ++++++ > libgimpbase/gimpbaseenums.h | 19 +++++++++++-------- > tools/pdbgen/enums.pl | 10 ++++++++-- > 4 files changed, 44 insertions(+), 10 deletions(-) Should I be working on this code at all? I managed to get the LCH sliders into the Choose Foreground/Background dialog and also into the FG/BG dialog, alongside the HSV sliders. The code compiles and runs, but as soon as I move a slider, GIMP segfaults. The gdb backtrace is below. Should I attach a patch? or am I making a poor effort to duplicate what you are already working on? Program received signal SIGSEGV, Segmentation fault. 0x00007ffff29aea2c in param_spec_pool_equals () from /usr/lib64/libgobject-2.0.so.0 (gdb) bt
+ Trace 237440
Sure you should, I'm staying out of your way and won't touch the color selector files. Please attach the patch for review.
(In reply to Michael Natterer from comment #51) > Please attach the patch for review. I'm merging the existing patched code with the latest commits to libgimpbase/gimpbaseenums.h/etc, and then I'll attach a patch.
Created attachment 351563 [details] [review] Patch to add LCH to the color picker - segfaults upon moving a slider The python plug-in isn't compiled because it would need modification to add lch to one of the functions. Somehow while fixing the patch to incorporate changes in git master, the alpha channel slider vanished. The code compiles and runs, but segfaults as soon as any slider is moved. Here is the backtrace: Reading symbols from ../../install/bin/gimp-2.9...done. (gdb) run Starting program: /home/elle/code/gimpdefault/install/bin/gimp-2.9 [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7fffe250b700 (LWP 24542)] [New Thread 0x7fffe1d0a700 (LWP 24543)] This is a development version of GIMP. Debug messages may appear here. [New Thread 0x7fffc9d97700 (LWP 24545)] [New Thread 0x7fffc8f81700 (LWP 24546)] [Thread 0x7fffc8f81700 (LWP 24546) exited] gimp_display_shell_profile_update gimp_display_shell_profile_update gimp_display_shell_profile_update Missing fast-path babl conversion detected, Implementing missing babl fast paths accelerates GEGL, GIMP and other software using babl, warnings are printed on first occurance of formats used where a conversion has to be synthesized programmatically by babl based on format description *WARNING*: missing babl fast path(s) between formats "RGB double" and "CIE LCH(ab) double" *WARNING*: missing babl fast path(s) between formats "CIE LCH(ab) double" and "RGB double" Program received signal SIGSEGV, Segmentation fault. 0x00007ffff29c7bb0 in g_type_check_instance_cast () from /usr/lib64/libgobject-2.0.so.0 (gdb) bt
+ Trace 237442
Thanks :) I have only a minute before I have to run, so just one thing: you can't change any public function, these "lch" parameters in the set_color() functions and signals etc. must vanish.
How is one to add an LCH color picker to the main color-picking widget/panel thingy, if one cannot use lch parameters in the set_color() functions?
(In reply to Elle Stone from comment #55) > How is one to add an LCH color picker to the main color-picking widget/panel > thingy, if one cannot use lch parameters in the set_color() functions? Maybe by adding a second parallel function for lch, for every set_color function?
Created attachment 351583 [details] [review] Add lch color sliders without adding lch to set_color functions Still segfaults upon moving a slider (even a "not lch" slider). Still no alpha channel slider. There is no code that changes the lch sliders. Here's a backtrace for the sefgfault: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff26b902f in g_hash_table_lookup () from /usr/lib64/libglib-2.0.so.0 (gdb) bt
+ Trace 237466
---Type <return> to continue, or q <return> to quit---q
Created attachment 351589 [details] [review] Add non-funtioning lch sliders, no segfaults Well, at least it doesn't segfault. My apologies for all the patches. Hopefully now I'll be able to add functions to make the lch sliders actually function. Please let me know if I'm on totally the wrong coding tangent!
(In reply to Elle Stone from comment #58) > Hopefully now I'll be able to add functions to make the lch sliders actually > function. Please let me know if I'm on totally the wrong coding tangent! Making progress! After adding a couple of parallel lch functions, now moving the RGB sliders also correctly updates the LCH (and HSV) sliders. But moving the HSV sliders still doesn't update the LCH sliders. So I'll continue adding parallel lch functions and hopefully have a patch by the end of the day.
I have edited the patch a bit too and stopped it from crashing, no parallel functions needed. There is also one big problem in the patch: you can't add the LCH enum values in the middle of the enum, they *must* be appended to the enum. That brings some issues with the order of the sliders in GimpColorScales, but for simplifity they should be at the end for now (in enum value order), adding that abstraction at the same time would be too much. Want me to attach my modified patch?
(In reply to Michael Natterer from comment #60) > Want me to attach my modified patch? Yes please!
Created attachment 351670 [details] [review] Cleaned up patch - Removes traces of public LCH API - Removes clamping from GimpRGB value validation - Reorders enums to append in a binary-compatible way - Various cleanups
(In reply to Michael Natterer from comment #62) > Created attachment 351670 [details] [review] [review] > Cleaned up patch > > - Removes traces of public LCH API > - Removes clamping from GimpRGB value validation > - Reorders enums to append in a binary-compatible way > - Various cleanups Thanks! that seems to work perfectly! and with fewer modified files! I'm going to try to add code to make the LCH sliders dynamically show black for out of range colors. This feature was requested (loudly and vehemently) by several people using my patched "anyrgb" version of GIMP, and it turns the LCH/RGB sliders into a "color education in a box".
Created attachment 351728 [details] [review] Make LCH sliders indicate out of range colors Apply after applying Mitch's patch. This patch adds two lines to libgimpwidgets/gimpcolorscale.c, that make the LCH sliders indicate out of range colors.
Ah that's what those lines were for :) I removed them from your original patch because they didn't make any sense to me.
Created attachment 351774 [details] "at the hull" slider artifact After having people ask for an out-of-range indicator, and providing same, the next complaint I got (from two, maybe three people) was that the LCH sliders do show deliberately selected max or min R or G or B as producing an "out of range" LCH color when really the selected color is "at the hull" of the color space. For example, for the color in the screenshot, setting Blue to 100.0, the Hue slider's pointer position indicates "out of range". Also there appears to be a bit of "slop" in the result of the comparison to determine when to use black for the slider color. Typing in 99.9 for the Blue slider still doesn't move the Hue slider all the way out of the black area, but 99.8 does move the Hue slider enough to show "in gamut". It's not just the Hue slider that's affected, and sometimes the gap is larger than in the attached jpeg. I told people that the problem is the slider position code is integer-based and I didn't know how to make that code be floating point-based (I tried twice and failed each time). imho, having this "at the hull of the color space artifact" is much, much better than not having a visually obvious out-of-gamut warning.
Having only floating point in these widgets (no integer) is also part of making bug 770054 go away. That's a broader scope, but will take care of the issue you mention too. I'm out of town currently but will still try to get things forward here, can't promise a timing tho.
For out-of-gamut indication we should probably use the out-of-gamut color from preferences, all color selector widgets have access to the GimpColorConfig object and can just get the color from there.
(In reply to Michael Natterer from comment #68) > For out-of-gamut indication we should probably use the out-of-gamut > color from preferences, all color selector widgets have access to > the GimpColorConfig object and can just get the color from there. Do you mean selecting "Preferences/Color Management/soft proofing", selecting an sRGB color space profile from disk, and activating the out of gamut indicator? I'm not sure how this would help a person when the person is pushing the LCH sliders around to choose a color to paint with. What is needed is immediate feedback directly from the LCH sliders. FWIW, my "out of range indicating" code doesn't just indicate "out of range", but also shows roughly how far out of gamut/range, in the sense that the further from "in gamut/in range" on the slider, the more out of gamut the color is, putting integer artifacts to one side.
(In reply to Elle Stone from comment #69) > (In reply to Michael Natterer from comment #68) > > For out-of-gamut indication we should probably use the out-of-gamut > > color from preferences, all color selector widgets have access to > > the GimpColorConfig object and can just get the color from there. > > Do you mean selecting "Preferences/Color Management/soft proofing", > selecting an sRGB color space profile from disk, and activating the out of > gamut indicator? > Oh, after rereading for the 27th time, I think you mean use the *color* specified in Preferences (which might be black), instead of using black regardless of the color specified in Preferences. That makes sense.
I also had to read your comment 69 at least 27 times... Communication is hard, but it seems we agree after all :)
Created attachment 351829 [details] lcms already shows out of gamut colors, sometimes accurately, for some source-destination profile combinations (In reply to Michael Natterer from comment #71) > I also had to read your comment 69 at least 27 times... Well, the 27th reading still should have made no sense as I was misremembering what is shown in the attached jpeg. When using soft proofing with the "mark out of gamut" setting, LCMS already does automatically mark out of gamut colors in the color sliders, sometimes even accurately. But LCMS doesn't mark out of gamut colors for cases where the source color space is unbounded sRGB and the destination color space is bounded sRGB (for a bounded sRGB destination color space select a V2 sRGB profile from disk). Nor does LCMS accurately mark out of gamut colors for linear gamma source color spaces.
Created attachment 351889 [details] [review] Next patch iteration, please test Yeah, the LCMS code won't do it. And I think the configured oog color is not a good idea of mine either... New patch: - Removed GimpLCH from the public API (color spaces in libgimpwidgets are a thing of the past). - Merged your patch, but did the same to GimpColorSelect, and made it white for "too dark" colors and black for "too bright" so there is always a contrast when the widgets goes oog. - Changed the way GimpColorSelect renders to be much less CPU intensive.
Created attachment 351905 [details] black and white out of range indicators (In reply to Michael Natterer from comment #73) > Created attachment 351889 [details] [review] [review] > Next patch iteration, please test > - Merged your patch, but did the same to GimpColorSelect, and made it > white for "too dark" colors and black for "too bright" so there is > always a contrast when the widgets goes oog. Hmm, I can see the logic for using black and white. But personally I think "always black when out of gamut" is easier to visually grasp. "Black or white" does add an additional visual cue indicating "negative RGB" or "RGB>1.0f". But I think maybe that's too much information for people to absorb visually - with the current patch I find myself trying to decipher "what does white/black really mean". Also, it's entirely possible to have a color that has at least one negative channel values and also at least one channel value > 1.0f. Perhaps bright magenta would be better than solid black OR black and white - magenta being the color most commonly used for out of gamut colors when soft proofing or when checking for blown highlights, not just in LCMS but also in many raw processors. This might be a very clear-cut visual indicator that people are already used to interpreting as "out of gamut warning". And then all they have to do is look at the RGB sliders to see "which direction(s)" the selected color is going out of gamut. A problem with using any given color as indicating "out of gamut selected color" is that the out of gamut color might be too close to the color the user wants to choose. The attached screenshot with magenta colors being a case in point as I was actually selecting magenta colors. But out there in the real world bright saturated magenta is hard to find, which is probably why it's the usual default "out of gamut indicator color". The advantage of allowing the user to select a custom "out of gamut indicator color" is exactly in cases where the user might be painting or editing a lot of magenta colors. I'm about to do some testing to make sure the LCH code does produce the right results. Previous spot-testing did indicate everything is good, and I can't imagine that there will be any issues, but it never hurts to double-check.
Created attachment 351917 [details] LCH Chroma can be higher than 100 even in the sRGB color space Hmm, took a while to figure out why the LCH Chroma value was coming out wrong, "topping out" at 100, somehow totally overlooked that Chroma slider only runs to 100. Some "real world" greens can have chromas as high as around 240 - anything higher than that is in the realm of imaginary colors. In the sRGB color space, greenest green tops out at Chroma=114. In my patched GIMP I set the highest chroma arbitrarily at 200 on the LCH chroma sliders. In the digital darkroom, even in the very wide gamut standard color spaces, it's not too likely that users will ever need a chroma value greater than 200.
I agree on both your points: - using the configured oog color -> really the better idea - range of chroma -> absolutely However, the chroma problem exists for all channels, particularly for oog RGB values. While we cannot display these colors, we must absolutely display the values, for all channels, no matter how much oog they might be. I suggest we change the oog color to what's configured in prefs, and solve the problem with oog *numeric* values to display after that, because it touches bug 770054 where we have to change value ranges and display anyway.
This is almost done and should and will go into 2.10.
I went ahead and pushed the next version of the patch, without any extended range just yet: commit 2b167d63372e79a990383f7f9f52ccb8ca730e13 Author: Michael Natterer <mitch@gimp.org> Date: Wed May 17 19:28:40 2017 +0200 Bug 749902 - Add Hue-Chroma operation/tool and LCH color selector Add LCH to the color selectors, patch by Elle Stone and myself. - Extend enum GimpColorSelectorChannel by LCH channels - Support them in GimpColorScale, GimpColorScales and GimpColorSelect - Did not yet remove the HSV channels until things are working 100% ok - Change drawing in GimpColorSelect to be much faster, to compensate for babl_process() making the LCH modes pretty slow - Clean up stuff in GimpColorSelect This is WIP and should not be considered finished, biggest TODO is displaying out-of-gamut values in GimpColorScales' spinbuttons. libgimpwidgets/gimpcolorscale.c | 138 ++++++++-- libgimpwidgets/gimpcolorscales.c | 140 ++++++---- libgimpwidgets/gimpcolorselect.c | 743 +++++++++++++++++++++++++++++++++++++++++++++------ libgimpwidgets/gimpwidgetsenums.c | 12 +- libgimpwidgets/gimpwidgetsenums.h | 34 ++- 5 files changed, 898 insertions(+), 169 deletions(-)
Comment on attachment 351889 [details] [review] Next patch iteration, please test Pushed with some more modifications.
(In reply to Michael Natterer from comment #79) > Comment on attachment 351889 [details] [review] [review] > Next patch iteration, please test > > Pushed with some more modifications. I'm not sure what you would like tested - attachment 351889 [details] [review] looks like the patch from a couple of days ago and doesn't apply after pulling in commit 2b167d63372e79a990383f7f9f52ccb8ca730e13 After updating to https://git.gnome.org/browse/GIMP/commit/?id=2b167d63372e79a990383f7f9f52ccb8ca730e13 from comment 78, everything looks wonderful except that the Chroma slider still can only show Chroma values up to 100, but I suspect this is part of the code for showing extended range values on the sliders? Anyway: * The configurable "out of gamut" color is really nice - first I had gray, then changed to magenta, worked perfectly. * When the L, C, or H sliders are selected, the in-gamut vs out-of-gamut colors in the color block next to the sliders is awesome. * All the vertical bars next to the color block have nice color gradients, which is really cool. * I tried quite a few examples of changing various sliders and making sure the resulting LCH values were correct, and they always were. Regarding removing HSV, it might be nice to keep HSV sliders around for awhile, even after LCH sliders are just right, until users get used to working with LCH as well as HSV. HSV has the advantage of familiarity. Also, it might be the case that some users will never reconcile themselves to not having access to HSV "somewhere" in the color selecting dialogs.
Went for the larger ranges now, rather than doing it together with bug 770054, can we consider this FIXED now? commit 6ab57a12cfde91fe1c0fb178a12c331b493d4be2 Author: Michael Natterer <mitch@gimp.org> Date: Thu May 18 19:53:47 2017 +0200 Bug 749902 - Add Hue-Chroma operation/tool and LCH color selector Last bit for now: allow GimpColorScales' spinbuttons to go further than its color scales. Please review the new min/max values of the individual channels. libgimpwidgets/gimpcolorscales.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
Min/max values are something that can be modified if someone comes up with reasonable use cases. But for now the current ranges seem just fine to me. It's interesting to finally see RGB values outside the "in gamut" range displayed in the color sliders. As far as I'm concerned this bug is fixed. And in addition the LCH color picker has wonderful additional features beyond just bare support for LCH sliders - thanks!
Great :) Away with this bug, then...
Created attachment 352986 [details] [review] patch to change max value for chroma slider to 200 My apologies for not realizing the implications of having the chroma slider maximum be 100 instead of 200. The problem is two-fold: 1. Quite a few sRGB colors have LCH chroma values that are greater than 100. For example reddest red has a chroma of 107, and bluest blue has a chroma of 131. So it's inconvenient to have to deal with a Chroma slider limit of 100. 2. Also, the current Chroma slider doesn't properly show out of gamut areas on the Chroma slider. So for example picking a given LCH Hue and then moving the Lightness slider doesn't allow to see which Lightness value allows for choosing the maximum in-gamut Chroma for the chosen Hue.
Created attachment 352988 [details] Color sliders before and after applying the patch The patch allows the Chroma slider to properly display oog areas for any given combination of Lightness and Hue.
I completely forgot about this one. Pushed to master, thanks for the patch :) commit d13fa3900a40cbbc9da321205b499c80ee84cfca Author: Elle Stone <ellestone@ninedegreesbelow.com> Date: Thu Jun 1 04:38:19 2017 -0400 Bug 749902 - Add Hue-Chroma operation/tool and LCH color selector This patch increases the LCH Chroma slider maximum value from 100 to 200 and also makes the Chroma slider properly display out of gamut Chroma selections for any given Hue/Lightness combinations. The current Chroma slider only runs to 100. But quite a few sRGB colors have LCH chroma values that are greater than 100. For example reddest red has a chroma of 107, and bluest blue has a chroma of 131. So it's inconvenient to have to deal with a Chroma slider limit of 100. Also, the current Chroma slider doesn't properly show out of gamut areas on the Chroma slider. So for example picking a given LCH Hue and then moving the Lightness slider doesn't allow to see which Lightness value allows for choosing the maximum in-gamut chroma for the chosen Hue. libgimpwidgets/gimpcolorscale.c | 2 +- libgimpwidgets/gimpcolorscales.c | 2 +- libgimpwidgets/gimpcolorselect.c | 18 +++++++++--------- 3 files changed, 11 insertions(+), 11 deletions(-)