Tukukkk

I wrote a record which encapsulates conversion of colors between TColor, RGB values, CMYK values, and HSV/HSB values. It seems to work (at least for the purpose I made it), but I'm not too confident about the rest. Specifically the conversion between RGB and HSV. This record supports implicit conversion with TColor. I originally made it to read the HSV values from a TColor but figured I'd go all out and build something even more useful (CMYK was an added bonus).

Can you find anything wrong with how this works? Any changes I can make to improve its performance? I know there are some places in the code where the formatting is off, I have yet to clean all that up (mostly mix of uppercase/lowercase).

(*

  TColorRec - Helper record for TColor to easily interchange for simplicity of reading not only RGB but also CMYK and HSV.

  Supports directly assigning a TColor using implicit class operators.

*)



type

  TColorRec = record

  private

    FRed: Byte;

    FGreen: Byte;

    FBlue: Byte;

    function GetBrightness: Double;

    function GetHue: Double;

    function GetSaturation: Double;

    procedure SetBrightness(const Value: Double);

    procedure SetHue(const Value: Double);

    procedure SetSaturation(const Value: Double);

    function GetBlack: Integer;

    function GetCyan: Integer;

    function GetMagenta: Integer;

    function GetYellow: Integer;

    procedure SetBlack(const Value: Integer);

    procedure SetCyan(const Value: Integer);

    procedure SetMagenta(const Value: Integer);

    procedure SetYellow(const Value: Integer);

  public

    class operator implicit(Value: TColorRec): TColor;

    class operator implicit(Value: TColor): TColorRec;

    property Red: Byte read FRed write FRed;

    property Green: Byte read FGreen write FGreen;

    property Blue: Byte read FBlue write FBlue;

    property Hue: Double read GetHue write SetHue;

    property Saturation: Double read GetSaturation write SetSaturation;

    property Brightness: Double read GetBrightness write SetBrightness;

    property Cyan: Integer read GetCyan write SetCyan;

    property Magenta: Integer read GetMagenta write SetMagenta;

    property Yellow: Integer read GetYellow write SetYellow;

    property Black: Integer read GetBlack write SetBlack;

  end;



implementation



function RGBToHSV(R, G, B: Byte; var H, S, V: Double): Boolean;

var

  minRGB, maxRGB, delta: Double;

begin

  h := 0.0;

  minRGB := Min(Min(R, G), B);

  maxRGB := Max(Max(R, G), B);

  delta := (maxRGB - minRGB);

  V := maxRGB;

  if (maxRGB <> 0.0) then

    S := 255.0 * delta / maxRGB

  else

    S := 0.0;

  if (S <> 0.0) then begin

    if R = maxRGB then

      H := (G - B) / delta

    else if G = maxRGB then

      H := 2.0 + (B - R) / delta

    else if B = maxRGB then

      H := 4.0 + (R - G) / delta

  end else

    H := -1.0;

  H := h * 60;

  if H < 0.0 then

    H := H + 360.0;



  //S := S * 100 / 255;

  //V := B * 100 / 255;

  S := S / 255;

  V := V / 255;



  Result:= True;

end;



function HSVToRGB(H, S, V: Double; var R, G, B: Byte): Boolean;

var

  i: Integer;

  f, p, q, t: Double;

  procedure CopyOutput(const RV, GV, BV: Double);

  const

    RGBmax = 255;

  begin

    R:= Round(RGBmax * RV);

    G:= Round(RGBmax * GV);

    B:= Round(RGBmax * BV);

  end;

begin

  Assert(InRange(H, 0.0, 1.0));

  Assert(InRange(S, 0.0, 1.0));

  Assert(InRange(V, 0.0, 1.0));

  if S = 0.0 then begin

    // achromatic (grey)

    CopyOutput(B, B, B);

    Result:= True;

    exit;

  end;

  H := H * 6.0; // sector 0 to 5

  i := floor(H);

  f := H - i; // fractional part of H

  p := V * (1.0 - S);

  q := V * (1.0 - S * f);

  t := V * (1.0 - S * (1.0 - f));

  case i of

    0: CopyOutput(V, t, p);

    1: CopyOutput(q, V, p);

    2: CopyOutput(p, V, t);

    3: CopyOutput(p, q, V);

    4: CopyOutput(t, p, V);

    else CopyOutput(V, p, q);

  end;

  Result:= True;

end;



{ TColorRec }



class operator TColorRec.implicit(Value: TColorRec): TColor;

begin

  with Value do

    Result:= RGB(Red, Green, Blue);

end;



class operator TColorRec.implicit(Value: TColor): TColorRec;

begin

  with Result do begin

    FRed:= GetRValue(Value);

    FGreen:= GetGValue(Value);

    FBlue:= GetBValue(Value);

  end;

end;



function TColorRec.GetHue: Double;

var

  H, S, V: Double;

begin

  RGBToHSV(FRed, FGreen, FBlue, H, S, V);

  Result:= H;

end;



function TColorRec.GetSaturation: Double;

var

  H, S, V: Double;

begin

  RGBToHSV(FRed, FGreen, FBlue, H, S, V);

  Result:= S;

end;



function TColorRec.GetBrightness: Double;

var

  H, S, V: Double;

begin

  RGBToHSV(FRed, FGreen, FBlue, H, S, V);

  Result:= V;

end;



function TColorRec.GetCyan: Integer;

begin

  Result:= GetCValue(RGB(FRed, FGreen, FBlue));

end;



function TColorRec.GetMagenta: Integer;

begin

  Result:= GetMValue(RGB(FRed, FGreen, FBlue));

end;



function TColorRec.GetYellow: Integer;

begin

  Result:= GetYValue(RGB(FRed, FGreen, FBlue));

end;



function TColorRec.GetBlack: Integer;

begin

  Result:= GetKValue(RGB(FRed, FGreen, FBlue));

end;



procedure TColorRec.SetBrightness(const Value: Double);

var

  H, S, V: Double;

begin

  RGBToHSV(FRed, FGreen, FBlue, H, S, V);

  V:= Value;

  HSVToRGB(H, S, V, FRed, FGreen, FBlue);

end;



procedure TColorRec.SetHue(const Value: Double);

var

  H, S, V: Double;

begin

  RGBToHSV(FRed, FGreen, FBlue, H, S, V);

  H:= Value;

  HSVToRGB(H, S, V, FRed, FGreen, FBlue);

end;



procedure TColorRec.SetSaturation(const Value: Double);

var

  H, S, V: Double;

begin

  RGBToHSV(FRed, FGreen, FBlue, H, S, V);

  S:= Value;

  HSVToRGB(H, S, V, FRed, FGreen, FBlue);

end;



procedure TColorRec.SetCyan(const Value: Integer);

begin

  Self:= CMYK(Value, Magenta, Yellow, Black);

end;



procedure TColorRec.SetMagenta(const Value: Integer);

begin

  Self:= CMYK(Cyan, Value, Yellow, Black);

end;



procedure TColorRec.SetYellow(const Value: Integer);

begin

  Self:= CMYK(Cyan, Magenta, Value, Black);

end;



procedure TColorRec.SetBlack(const Value: Integer);

begin

  Self:= CMYK(Cyan, Magenta, Yellow, Value);

end;

What is the purpose of these conversions? How are they going to be used? I ask because I typically have 2 use cases for HSV colors:

1. converting an image into the color space for some app or library that works in that space


2. converting an individual pixel into HSV (or whatever) to do a particular manipulation on it, then converting back to RGB

In the first case, there are generally constraints, such as that the resulting image must have 8-bits per channel. In the second case, I usually want floating point precision for doing the color manipulation, though. It seems very odd to have CMYK be Integers instead of Bytes like RGB. What's the reasoning there?

The answer to the above question (the purpose of the conversions) will determine what format you should return the values in. Right now, the HSV conversion takes 8-bit unsigned input values but returns double-precision floats. That's useful for #2. Is there any reason the input RGB values couldn't also be double-precision floats?

S and V are in the range 0-1, but H is 0-360. It's been years since I used Delphi, but most math functions in most other languages take radians. Are you sure you want hue to be in degrees? I have seen implementations where H is 0-1 (representing 0° to 360°) for consistency, but it's a pain to pass to math functions. In the HSVToRGB() function you assert that they're in the 0-1 range, which H definitely won't be!

If you do want S to be in the 0-1 range, then don't multiply it by 255 here:

if (maxRGB <> 0.0) then

    S := 255.0 * delta / maxRGB

and then divide by 255 here:

  S := S / 255;

Also, why is RGBToHSV()a function? It always returns True. Same with HSVToRGB().

The procedure name CopyOutput() is confusing. You aren't copying any values. You're scaling the inputs, so I'd call it ScaleComponents() or something like that. In this case:

if S = 0.0 then begin

    // achromatic (grey)

    CopyOutput(B, B, B);

    Result:= True;

    exit;

end;

You're using the blue channel, which hasn't been set yet, to set R, G, and B. Shouldn't that be:

CopyOutput(V, V, V);

Further down in HSVToRGB() you write:

H := H * 6.0; // sector 0 to 5

If H is in the 0-360 range, it should be:

H := H / 60.0;

It seems wasteful to have setters and getters for single channels but not have them for all 3 (or 4) channels at a time. For example, if a caller is asking for Hue, it's likely they'll ask for Saturation and Value, too. So it would be nice to have some way to convert all 3 at once. Otherwise, the conversion is done 3 times and there's 3 times the function-call overhead.

For the methods SetHue(), SetSaturation(), and SetBrightness(), I wouldn't call the argument Value since it can be confused with the V component of HSV. I would name the argument NewHue, NewSaturation and NewBrightness.

It looks like you're using inherited conversions for CMYK. Am I understanding that correctly? Since I don't know the TColor class, I don't have many thoughts on that. If it's currently working for you, then I guess it probably works OK.

Putting this statement before S := S / 255, makes the record consistent for handling HSV:

H := H/360.0; //for making the 0..360 degrees map to 0..1 as required in the assert