Bug 467574

Summary: Problems while changing the Saturation value in HSL color space.
Product: [Fedora] Fedora Reporter: Philippe Rigault <prigault>
Component: inkscapeAssignee: Lubomir Rintel <lkundrak>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: 10CC: lkundrak
Target Milestone: ---Keywords: Triaged
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-12-18 06:35:51 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Philippe Rigault 2008-10-18 20:09:40 UTC
Description of problem:
Changing the Saturation value in the HSL color space is problematic due to odd interactions between color channel values.

Version-Release number of selected component (if applicable):
inkscape-0.46-5.fc10.x86_64
inkscape-0.46-6.fc10.x86_64

How reproducible: Always


Steps to Reproduce:
1. Create an object
2. Open 'Fill and Stroke' dialog to set its color
3. Select HSL color space
4. Set color to:
 H: X (any X > 0)
 S: 255
 L: Y (any Y > 0)
5. Decrease S by clicking repeatidely on the slider 'down' arrow

Actual results:
 First click:   S goes 255 -> 254 (correct)
 Second click:  S goes 254 -> 255 (bug, should go down to 253) AND H/L values are modified (bug, should not be altered)
 Subsequent clicks: S alternates between 254 and 255 (bug, should decrease by 1 at each click) AND H/L values are modified (bug, should not be altered)

The same kind of odd behaviour happens if the S value is modified by entering text. Example:
  type 250 in S component instead of 255, then RETURN -> value back to 255.

Expected results: Upon incrementing/decrementing (mouse click or enter text) the value of S, only this value should be modified accordingly, and values of H and L should remain unchanged.


Additional info:
This was _not_ introduced by the change from 0.46-5 to 0.46-6 (that fixed bug 467431).

I think some callbacks/signals updating the values must update/reset the wrong component somewhere.

Cheers.

Comment 1 Lubomir Rintel 2008-10-19 09:00:24 UTC
Thanks for the report.

I'm able to reproduce locally (inkscape-0.46-2.el5.1).
Building the svn head to check whether it is fixed upstream.

Comment 2 Lubomir Rintel 2008-10-19 13:23:30 UTC
On a second thought -- this might be due to the fact RGB is always used internally and not all HSL or CMYK combinations have corresponding 24bit RGB values.

Comment 3 Philippe Rigault 2008-10-19 14:54:36 UTC
The problem may come from the fact that the min/max values for HSL sliders (sorry I said HSV, inkscape uses the slightly different HSL color space), together with their mapping to real (H,S,L) values are simply _wrong_, and maybe the conversion RGB/HSL functions too.

HSL components are NOT addititive components on the same scale like RGB components are. Instead, HSL components are:
 H is a real number in degrees and should be in [0,360] (360 = 0)
 S is a real number in [0,1]
 L is a real number in [0,1] (Same thing for V in HSV color space BTW)

What applications (e.g gimp) use the following values in their color selection tool:
 H: integer in [0,360]
 S: integer in [0,100] (% Saturation)
 L: integer in [0,100] (% Lightness)

Inkscape uses [0,255] for all of them, which is valid ONLY if they map discrete values to real values as follows:
 H: [0,255] -> [0,360]
 S: [0,255] -> [0,1]
 L: [0,255] -> [0,1]

I suspect that inkscape has this all screwed up, as min/max values of [0,255] for HSL is _very_ uncommon and absolutely unintuitive.

I am not familiar with inkscape code. Could you check that the min/max values and conversion functions are correctly implemented in inkscape ?

Conversions functions are explained here:
http://en.wikipedia.org/wiki/HSL_color_space

Cheers.

Comment 4 Lubomir Rintel 2008-10-19 18:20:29 UTC
(In reply to comment #3)
...
> I suspect that inkscape has this all screwed up, as min/max values of [0,255]
> for HSL is _very_ uncommon and absolutely unintuitive.
> 
> I am not familiar with inkscape code. Could you check that the min/max values
> and conversion functions are correctly implemented in inkscape ?
> 
> Conversions functions are explained here:
> http://en.wikipedia.org/wiki/HSL_color_space

Thanks for the explanation. I myself are very unfamiliar with HSL and HSV, thoguh what you say makes pretty good sense to me. While googling more about those, I stumbled upon a bug report [1] filed against inkscape which seems to describe the very same issue.

[1] https://bugs.launchpad.net/inkscape/+bug/166622

Given I'm not very familiar with inkscape internals either I suggest you add a comment there similar to comment #3. I'd be happy to include eventual patch for more sane values for ranges in HSL color space in Fedora package.

Comment 5 Lubomir Rintel 2008-10-19 18:42:20 UTC
By the way, the relevant source file seems to be src/widgets/sp-color-scales.cpp [1], see the case after line 421 for how the sliders are set up (they all seem to have to be in the same range). Function sp_color_rgb_to_hsl_floatv() seems to do the conversion and look for uses of SP_COLOR_SCALES_MODE_HSV (actually a misnomer) macro for more HSL-related code.

http://inkscape.svn.sourceforge.net/viewvc/inkscape/inkscape/trunk/src/widgets/sp-color-scales.cpp?revision=17347&view=markup

Comment 6 Philippe Rigault 2008-10-20 02:27:46 UTC
I said I first though Inkscape used HSV and then HSL, but after looking at the code I am not sure they themselves know what they are using. Indeed, there seems to be confusion between HSV and HSL all over the place:
 - #define values all talk about HSV
 - functions all talk about hsl
as in:

<snip>
347 case SP_COLOR_SCALES_MODE_HSV:
348     sp_color_hsl_to_rgb_floatv (rgb, getScaled(_a[0]), getScaled(_a[1]), getScaled(_a[2]));
<snip>

If the goal is to confuse code readers, they couldn't do a better job.

As you mentioned, one needs to look at how the function
 sp_color_rgb_to_hsl_floatv()
does the conversion in order to know:
 - what really they are using: HSV or HSL 
 - if values between 1 and 255 for color components are correctly used.

What remains clear is that:
 1. There is a problem in setting colors in HSL sliders (bug that should be fixed).
 2. The interface is _very_ confusing in setting all sliders in [0,255] for HSL. Nobody does this, it does not make sense (undesirable feature that should be changed to avid confusing users).
 3. The code is also confusing as whether it is using HSL or HSV (a problem for developpers, but a true one that calls for more documentation).

Note finally that restricting (H,S,V) to integers in ([0,360],[0,100],[0,100]) respectively, while being much better that it is now (and conforming to what gimp does), still has one drawback: not all 24-bit RGB values are accessible from other color spaces.

The REALLY NICE answer to this, if RGB values are used internally and they are restricted to 24bit, would actually be to set:
 - R,G and B components to 8-bit unsigned integers (in [0,255])
 - H, S and L components to FLOAT numbers (decimal to be exact):
	- H in [0,360]
	- S and V/L in either [0,1] (absolute) or [0,100] (percent)
  and each time a value is set, convert it to the nearest (R,G,B) valid color.
This way, all possible (valid) RGB values would be accessible through HSL and CMYK also. 
But this may be too much to ask, in addition to being different from (but this time better than) what gimp does.

Cheers.

Comment 7 Bug Zapper 2008-11-26 03:58:58 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 10 development cycle.
Changing version to '10'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 8 Bug Zapper 2009-11-18 08:35:45 UTC
This message is a reminder that Fedora 10 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 10.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '10'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 10's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 10 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 9 Bug Zapper 2009-12-18 06:35:51 UTC
Fedora 10 changed to end-of-life (EOL) status on 2009-12-17. Fedora 10 is 
no longer maintained, which means that it will not receive any further 
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of 
Fedora please feel free to reopen this bug against that version.

Thank you for reporting this bug and we are sorry it could not be fixed.