GNOME Bugzilla – Bug 748187
Change kappa and epsilon calculations, modify H range code
Last modified: 2015-05-21 13:31:59 UTC
Created attachment 302003 [details] [review] patch to modify the kappa/epsilon code and add to the H code Thomas Manni's LAB/LCH code uses the equations rather than the simplified calculated values for kappa and epsilon. I misinterpreted Lindbloom to mean use the simplified values rather than the equations, so that's what I put in CIE.c. But upon re-reading Lindbloom, in fact the right thing to do is use the equations. So this patch changes kappa and epsilon to be calculated. See http://brucelindbloom.com/index.html?LContinuity.html Also Manni's code has two parts for bringing the H value within range, which seems to make sense. So I added the second part to the CIE.c code. Manni uses #define statements for the calculations for kappa and epsilon. As these calculations are used in two different functions in CIE.c, should they be made into #define statements in CIE.c?
Comment on attachment 302003 [details] [review] patch to modify the kappa/epsilon code and add to the H code if (*to_H < 0.0) *to_H += 360; + else if (*to_H < 0.0) At first sight this else if seems useless
FWIW, the Lindbloom page does list both cases: http://brucelindbloom.com/index.html?Eqn_Lab_to_LCH.html "If H < 0°, add 360° to it. If H ≥ 360°, subtract 360° from it."
(In reply to Elle Stone from comment #2) > FWIW, the Lindbloom page does list both cases: > http://brucelindbloom.com/index.html?Eqn_Lab_to_LCH.html > "If H < 0°, add 360° to it. > If H ≥ 360°, subtract 360° from it." well but both if are for the < 0 case and so only the first is possibly executed
Created attachment 302008 [details] [review] revised patch with hopefully correct H code Sorry! I didn't look at the second lines of code closely enough. Try this patch.
(In reply to Elle Stone from comment #4) > Created attachment 302008 [details] [review] [review] > revised patch with hopefully correct H code > > Sorry! I didn't look at the second lines of code closely enough. Try this > patch. I thought it was sanitizing a user input, but https://git.gnome.org/browse/babl/tree/extensions/CIE.c#n280 it is only acting on the return value of atan2 which is always between -PI and PI so *to_H cannot be greater than 2*PI (360°) and so that check was probably not there on purpose
Created attachment 302016 [details] [review] patch without the added H code Try this patch. The H range-correcting code that was originally in CIE.c really is needed. I removed the new lines as not needed.
Review of attachment 302016 [details] [review]: extensions/CIE.c | 258 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 159 insertions(+), 99 deletions(-) that patch seems to do much more than I expected. Among other things it has diagnostic printf and indentation changes that suggest it is not the patch intended. Anyway if you think that kappa and epsilon should be computed as in the comments after their initialization that's fine for me.
Created attachment 302091 [details] [review] patch without the diagnostic printf statements Massimo, sorry, many apologies, I made the commit using the wrong CIE.c file. This one doesn't have the printf statements.
(In reply to Elle Stone from comment #0) > > Manni uses #define statements for the calculations for kappa and epsilon. As > these calculations are used in two different functions in CIE.c, should they > be made into #define statements in CIE.c? It is a matter of style, they could be made into #define statements or two static const global variables (foreseeing possible multiple uses with a common definition and so only one place to correct in case of typos) but for two uses it is good to keep as they are in your last patch.
This patch fixed the epsilon and kappa code to use the equations, and also resolved the "style" in favor of #define statements: https://bugzilla.gnome.org/show_bug.cgi?id=749628 So this bug report can be closed.
commit 3b14d5b08cce705eb84ea788d4259f99eab7602f Author: Thomas Manni <thomas.manni@free.fr> Date: Thu May 21 01:33:54 2015 +0200 CIE: minor cleanups Use macros for kappa, epsilon and D50 reference white in all places Remove duplicated comments commit f4cc927054d4e3554734456ebc14b2fdc2c895b6 Author: Thomas Manni <thomas.manni@free.fr> Date: Wed May 20 12:12:35 2015 +0200 CIE: new conversions to/from "CIE Lab alpha float" and "RGBA float"