Doc string and operation of color-distance

Doc string and operation of color-distance

[Eli Zaretskii]

Mark, the new color-distance says something strange in its doc string:

  (color-distance COLOR1 COLOR2 &optional FRAME METRIC)

  Return an integer distance between COLOR1 and COLOR2 on FRAME.
  COLOR1 and COLOR2 may be either strings containing the color name,
  or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
  If FRAME is unspecified or nil, the current frame is used.
  If METRIC is unspecified or nil, a modified L*u*v* metric is used.

The last sentence should say "non-nil", I think, and it should
document that METRIC is supposed to be a function of 2 colors.

Also, this is unexpected:

  (color-distance "red" "blue" nil 'lcms-cam02-ucs)
  => (error "Invalid color" "red")

I think the new lcms2 functions should support colors specified as
strings, because all the other color-related functions do.

Re: Doc string and operation of color-distance

[Mark Oteiza]

On 14/09/17 at 09:45pm, Eli Zaretskii wrote:
> Mark, the new color-distance says something strange in its doc string:
> 
>   (color-distance COLOR1 COLOR2 &optional FRAME METRIC)
> 
>   Return an integer distance between COLOR1 and COLOR2 on FRAME.
>   COLOR1 and COLOR2 may be either strings containing the color name,
>   or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
>   If FRAME is unspecified or nil, the current frame is used.
>   If METRIC is unspecified or nil, a modified L*u*v* metric is used.
> 
> The last sentence should say "non-nil", I think

No, the 'nil' is correct--if no metric is given as an argument,
color_distance is used which, as the comments say, is a modified L*u*v*
metric.

> and it should
> document that METRIC is supposed to be a function of 2 colors.

True, that would be helpful.

> Also, this is unexpected:
> 
>   (color-distance "red" "blue" nil 'lcms-cam02-ucs)
>   => (error "Invalid color" "red")
> 
> I think the new lcms2 functions should support colors specified as
> strings, because all the other color-related functions do.

I thought (but did not test) the body of color-distance did the lookups.
Now I see what's going on.

Re: Doc string and operation of color-distance

[Eli Zaretskii]

> Date: Thu, 14 Sep 2017 15:12:26 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: emacs-devel@gnu.org
> 
> On 14/09/17 at 09:45pm, Eli Zaretskii wrote:
> > Mark, the new color-distance says something strange in its doc string:
> > 
> >   (color-distance COLOR1 COLOR2 &optional FRAME METRIC)
> > 
> >   Return an integer distance between COLOR1 and COLOR2 on FRAME.
> >   COLOR1 and COLOR2 may be either strings containing the color name,
> >   or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
> >   If FRAME is unspecified or nil, the current frame is used.
> >   If METRIC is unspecified or nil, a modified L*u*v* metric is used.
> > 
> > The last sentence should say "non-nil", I think
> 
> No, the 'nil' is correct--if no metric is given as an argument,
> color_distance is used which, as the comments say, is a modified L*u*v*
> metric.

Yes, but the comments also reference the article which describes the
metric, whereas the doc string doesn't.

I think we should either leave the nil case alone -- it wasn't
documented before in such detail -- or add more explanations and
perhaps the reference.

What's important is to describe the non-nil case.

Re: Doc string and operation of color-distance

[Mark Oteiza]

On 15/09/17 at 04:30pm, Eli Zaretskii wrote:
> > Date: Thu, 14 Sep 2017 15:12:26 -0400
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Cc: emacs-devel@gnu.org
> > 
> > On 14/09/17 at 09:45pm, Eli Zaretskii wrote:
> > > Mark, the new color-distance says something strange in its doc string:
> > > 
> > >   (color-distance COLOR1 COLOR2 &optional FRAME METRIC)
> > > 
> > >   Return an integer distance between COLOR1 and COLOR2 on FRAME.
> > >   COLOR1 and COLOR2 may be either strings containing the color name,
> > >   or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
> > >   If FRAME is unspecified or nil, the current frame is used.
> > >   If METRIC is unspecified or nil, a modified L*u*v* metric is used.
> > > 
> > > The last sentence should say "non-nil", I think
> > 
> > No, the 'nil' is correct--if no metric is given as an argument,
> > color_distance is used which, as the comments say, is a modified L*u*v*
> > metric.
> 
> Yes, but the comments also reference the article which describes the
> metric, whereas the doc string doesn't.
> 
> I think we should either leave the nil case alone -- it wasn't
> documented before in such detail -- or add more explanations and
> perhaps the reference.
> 
> What's important is to describe the non-nil case.

OK, I agree.  How about the following?

diff --git a/src/xfaces.c b/src/xfaces.c
index 012de4e7af..b309c16127 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -4093,7 +4093,8 @@ DEFUN ("color-distance", Fcolor_distance, Scolor_distance, 2, 4, 0,
 COLOR1 and COLOR2 may be either strings containing the color name,
 or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
 If FRAME is unspecified or nil, the current frame is used.
-If METRIC is unspecified or nil, a modified L*u*v* metric is used.  */)
+If METRIC is specified, it should be a function that accepts
+two lists of the form (RED GREEN BLUE) aforementioned. */)
   (Lisp_Object color1, Lisp_Object color2, Lisp_Object frame,
    Lisp_Object metric)
 {
@@ -4112,7 +4113,13 @@ If METRIC is unspecified or nil, a modified L*u*v* metric is used.  */)
   if (NILP (metric))
     return make_number (color_distance (&cdef1, &cdef2));
   else
-    return call2 (metric, color1, color2);
+    return call2 (metric,
+                  list3 (make_number (cdef1.red),
+                         make_number (cdef1.green),
+                         make_number (cdef1.blue)),
+                  list3 (make_number (cdef2.red),
+                         make_number (cdef2.green),
+                         make_number (cdef2.blue)));
 }
 
 \f

Re: Doc string and operation of color-distance

[Eli Zaretskii]

> Date: Fri, 15 Sep 2017 12:45:43 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: emacs-devel@gnu.org
> 
> > I think we should either leave the nil case alone -- it wasn't
> > documented before in such detail -- or add more explanations and
> > perhaps the reference.
> > 
> > What's important is to describe the non-nil case.
> 
> OK, I agree.  How about the following?

LGTM, thanks.

Re: Doc string and operation of color-distance

[Mark Oteiza]

Eli Zaretskii <eliz@gnu.org> writes:

> Mark, the new color-distance says something strange in its doc string:
>
>   (color-distance COLOR1 COLOR2 &optional FRAME METRIC)
>
>   Return an integer distance between COLOR1 and COLOR2 on FRAME.
>   COLOR1 and COLOR2 may be either strings containing the color name,
>   or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
>   If FRAME is unspecified or nil, the current frame is used.
>   If METRIC is unspecified or nil, a modified L*u*v* metric is used.
>
> The last sentence should say "non-nil", I think, and it should
> document that METRIC is supposed to be a function of 2 colors.
>
> Also, this is unexpected:
>
>   (color-distance "red" "blue" nil 'lcms-cam02-ucs)
>   => (error "Invalid color" "red")
>
> I think the new lcms2 functions should support colors specified as
> strings, because all the other color-related functions do.

At the very least we should feed metrics the translated RGBs.

Beyond that, in order to use (for example) lcms2-cam02-ucs with
color-distance, one has to translate RGB to XYZ, so the metric would be

  (lambda (a b)
    (lcms2-cam02-ucs (color-srgb-to-xyz a) (color-srgb-to-xyz b))

diff --git a/src/xfaces.c b/src/xfaces.c
index 012de4e7af..9804c96c39 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -4093,7 +4093,9 @@ DEFUN ("color-distance", Fcolor_distance, Scolor_distance, 2, 4, 0,
 COLOR1 and COLOR2 may be either strings containing the color name,
 or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
 If FRAME is unspecified or nil, the current frame is used.
-If METRIC is unspecified or nil, a modified L*u*v* metric is used.  */)
+If METRIC is specified, it should be a function that accepts two
+lists of the form (RED GREEN BLUE).  The default metric uses a
+modified L*u*v* space. */)
   (Lisp_Object color1, Lisp_Object color2, Lisp_Object frame,
    Lisp_Object metric)
 {
@@ -4112,7 +4114,15 @@ If METRIC is unspecified or nil, a modified L*u*v* metric is used.  */)
   if (NILP (metric))
     return make_number (color_distance (&cdef1, &cdef2));
   else
-    return call2 (metric, color1, color2);
+    {
+      Lisp_Object rgb1 = list3 (make_number (&cdef1.red),
+                                make_number (&cdef1.green),
+                                make_number (&cdef1.blue)); 
+      Lisp_Object rgb2 = list3 (make_number (&cdef2.red),
+                                make_number (&cdef2.green),
+                                make_number (&cdef2.blue)); 
+      return call2 (metric, rgb1, rgb2);
+    }
 }
 
 \f

Re: Doc string and operation of color-distance

[Mark Oteiza]

On 14/09/17 at 03:43pm, Mark Oteiza wrote:
> +      Lisp_Object rgb1 = list3 (make_number (&cdef1.red),
> +                                make_number (&cdef1.green),
> +                                make_number (&cdef1.blue)); 
> +      Lisp_Object rgb2 = list3 (make_number (&cdef2.red),
> +                                make_number (&cdef2.green),
> +                                make_number (&cdef2.blue)); 

I guess these need RGB values to be shifted?

Re: Doc string and operation of color-distance

[Eli Zaretskii]

> Date: Thu, 14 Sep 2017 16:23:12 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: emacs-devel@gnu.org
> 
> On 14/09/17 at 03:43pm, Mark Oteiza wrote:
> > +      Lisp_Object rgb1 = list3 (make_number (&cdef1.red),
> > +                                make_number (&cdef1.green),
> > +                                make_number (&cdef1.blue)); 
> > +      Lisp_Object rgb2 = list3 (make_number (&cdef2.red),
> > +                                make_number (&cdef2.green),
> > +                                make_number (&cdef2.blue)); 
> 
> I guess these need RGB values to be shifted?

Why?  color_distance only shifts them because it wants to work on
8-bit color components, but you want 16-bit components, AFAIU.

Re: Doc string and operation of color-distance

[Eli Zaretskii]

> From: Mark Oteiza <mvoteiza@udel.edu>
> Date: Thu, 14 Sep 2017 15:43:28 -0400
> Cc: emacs-devel@gnu.org
> 
> At the very least we should feed metrics the translated RGBs.

Yes.  But it would be nicer if the lcms.c functions accepted colors in
any form supported by Emacs: a color name, a string RGB spec, or a
list of RGB values.  I think.

> Beyond that, in order to use (for example) lcms2-cam02-ucs with
> color-distance, one has to translate RGB to XYZ, so the metric would be
> 
>   (lambda (a b)
>     (lcms2-cam02-ucs (color-srgb-to-xyz a) (color-srgb-to-xyz b))

color-srgb-to-xyz accepts 3 arguments, not one.  Also, its doc string
says the components are floats between 0 and 1.  So this should be
amended accordingly.

Re: Doc string and operation of color-distance

[Mark Oteiza]

On 15/09/17 at 04:40pm, Eli Zaretskii wrote:
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Date: Thu, 14 Sep 2017 15:43:28 -0400
> > Cc: emacs-devel@gnu.org
> >
> > At the very least we should feed metrics the translated RGBs.
>
> Yes.  But it would be nicer if the lcms.c functions accepted colors in
> any form supported by Emacs: a color name, a string RGB spec, or a
> list of RGB values.  I think.
>
> > Beyond that, in order to use (for example) lcms2-cam02-ucs with
> > color-distance, one has to translate RGB to XYZ, so the metric would be
> >
> >   (lambda (a b)
> >     (lcms2-cam02-ucs (color-srgb-to-xyz a) (color-srgb-to-xyz b))
>
> color-srgb-to-xyz accepts 3 arguments, not one.  Also, its doc string
> says the components are floats between 0 and 1.  So this should be
> amended accordingly.

Ah, yes I forget about color.el's calling conventions…

(defun lcms-rgb16->rgb1 (color)
  (mapcar (lambda (x) (/ (lsh x -8) 255.0)) color))

(lambda (a b)
  (lcms-cam02-ucs (apply #'color-srgb-to-xyz (color-rgb16b->rgb1 a))
                  (apply #'color-srgb-to-xyz (color-rgb16b->rgb1 b))))

Re: Doc string and operation of color-distance

[Mark Oteiza]

On 15/09/17 at 04:40pm, Eli Zaretskii wrote:
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Date: Thu, 14 Sep 2017 15:43:28 -0400
> > Cc: emacs-devel@gnu.org
> > 
> > At the very least we should feed metrics the translated RGBs.
> 
> Yes.  But it would be nicer if the lcms.c functions accepted colors in
> any form supported by Emacs: a color name, a string RGB spec, or a
> list of RGB values.  I think.

But color-distance and color-values do that already, and it's just
a matter of converting 16bit RGB to another color space, and all those
utilities exist already in color.el

Re: Doc string and operation of color-distance

[Eli Zaretskii]

> Date: Fri, 15 Sep 2017 12:32:09 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: emacs-devel@gnu.org
> 
> > Yes.  But it would be nicer if the lcms.c functions accepted colors in
> > any form supported by Emacs: a color name, a string RGB spec, or a
> > list of RGB values.  I think.
> 
> But color-distance and color-values do that already, and it's just
> a matter of converting 16bit RGB to another color space, and all those
> utilities exist already in color.el

If we don't intend that lcms.c functions be used independently, then I
guess it's OK.