HSL conversions - Githubissues

avh4 commented 6 years ago

Collaborating on this PR:

Check the most-recent comments on this PR to see if anyone is actively working on it. If not, continue:
Post a commenting stating what you are starting work on
Make a PR against this branch (basic-conversions)
Post a comment when you are done doing active work so others can pick up from there (including a link to your PR against this branch)

[x] fromHsla : { hue : Float, saturation : Float, lightness : Float, alpha : Float } -> Color
[x] hsl : Float -> Float -> Float -> Color
[x] hsla : Float -> Float -> Float -> Float -> Color
[x] toHsla : Color -> { hue : Float, saturation : Float, lightness : Float, alpha : Float }
[x] is the fromHsla algorithm correct?

avh4 commented 6 years ago

I merged in 2mol's work from #7, and added a test suite from the HSL examples in the CSS spec. I'm done working on this for tonight. The remaining work is to make the conversion pass the test suite, and add toHsla.

2mol commented 6 years ago

Hi. Thank you very much for all the work, especially the tests!

I can't run the tests, from the error message it seems that the elm-test version on npm is still the 0.18 one? (it asks for an elm-package.json). I couldn't figure myself out how to get a newer version, could you tell me how?

avh4 commented 6 years ago

It should be set up where you can use npm install and then npm test to run the tests

On Sun, Sep 9, 2018, 12:15 AM 2mol notifications@github.com wrote:

Hi. Thank you very much for all the work, especially the tests!

I can't run the tests, from the error message it seems that the elm-test version on npm is still the 0.18 one? (it asks for an elm-package.json). I couldn't figure myself out how to get a newer version, could you tell me how?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/avh4/elm-color/pull/8#issuecomment-419695947, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAExoYqQBooqEyYWysw1zDz1XosGHU7ks5uZL_8gaJpZM4WgKh3 .

2mol commented 6 years ago

Nope, it fails, mentioning it needs an elm-package.json

~/code/elm-color $ npm test

> @ test /Users/juri/code/elm-color
> elm-test && elm make --docs=documentation.json && elm-format --validate . && (elm diff || true)

It looks like you're running elm-test from within your tests directory.

Please run elm-test from your project's root directory, where its elm-package.json lives!
npm ERR! Test failed.  See above for more details.

I can see from the CI logs that hsl is off only by a little, so I'll try to figure out where I round the wrong way, or if I have some off-by-one error.

2mol commented 6 years ago

After some investigating, I'm now 85% sure that the discrepancies come from the test suite. Unless there is some slightly idiosyncratic spec that I don't know of? Or a newer version?

The relevant points:

HSL with https://www.w3schools.com/colors/colors_converter.asp gives me the same RGB values as the currently implemented functions (If I use round. Is there a reason that you use floor in ColorTest.elm?).
I double checked the formulas with the ones on wikipedia, and they are pretty much 1 to 1 the same.

You are likely to have a lot more experience than I do with regards to css specs, so I'll defer to your judgement. Where does the json in CssHslReference.elm come from?

avh4 commented 6 years ago

Ah, interesting. There a link at the top of the CssHslReference.elm--it's from the official CSS3 spec.

It seems like there are many slightly-different algorithms fit the conversion... The CSS3 also provides a reference implementation (in a language called ABC). I guess I'm of the opinion we should try to implement that and get the exact conversion output that CSS specifies, since Elm is currently gonna be used alongside CSS.

On Sun, Sep 9, 2018, 5:10 AM 2mol notifications@github.com wrote:

After some investigating, I'm now 85% sure that the discrepancies come from the test suite. Unless there is some slightly idiosyncratic spec that I don't know of? Or a newer version?

The relevant points:

HSL with https://www.w3schools.com/colors/colors_converter.asp gives me the same RGB values as the currently implemented functions (If I use round. Is there a reason that you use floor in ColorTest.elm?).

I double checked the formulas with the ones on wikipedia, and they are pretty much 1 to 1 the same.

You are likely to have a lot more experience than I do with regards to css specs, so I'll defer to your judgement. Where does the json in CssHslReference.elm come from?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/avh4/elm-color/pull/8#issuecomment-419711691, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAExrOyQYeIHep-pFtyiA1F6vwfqSofks5uZQUhgaJpZM4WgKh3 .

2mol commented 6 years ago

Ok. I'll try to pick it up again next week.

For reference let me paste both the reference in ABC from https://www.w3.org/TR/css-color-3/#hsl-examples, as well as the one from https://www.w3schools.com/lib/w3color.js so that I have them side by side and we can try to figure out the difference.

At a first glance they look similar to each other, but they use more basic math than mine does (optimized for performance?):

HOW TO RETURN hsl.to.rgb(h, s, l):
   SELECT:
      l<=0.5: PUT l*(s+1) IN m2
      ELSE: PUT l+s-l*s IN m2
   PUT l*2-m2 IN m1
   PUT hue.to.rgb(m1, m2, h+1/3) IN r
   PUT hue.to.rgb(m1, m2, h    ) IN g
   PUT hue.to.rgb(m1, m2, h-1/3) IN b
   RETURN (r, g, b)

HOW TO RETURN hue.to.rgb(m1, m2, h):
   IF h<0: PUT h+1 IN h
   IF h>1: PUT h-1 IN h
   IF h*6<1: RETURN m1+(m2-m1)*h*6
   IF h*2<1: RETURN m2
   IF h*3<2: RETURN m1+(m2-m1)*(2/3-h)*6
   RETURN m1

W3schools js implementation:

function hslToRgb(hue, sat, light) {
  var t1, t2, r, g, b;
  hue = hue / 60;
  if ( light <= 0.5 ) {
    t2 = light * (sat + 1);
  } else {
    t2 = light + sat - (light * sat);
  }
  t1 = light * 2 - t2;
  r = hueToRgb(t1, t2, hue + 2) * 255;
  g = hueToRgb(t1, t2, hue) * 255;
  b = hueToRgb(t1, t2, hue - 2) * 255;
  return {r : r, g : g, b : b};
}
function hueToRgb(t1, t2, hue) {
  if (hue < 0) hue += 6;
  if (hue >= 6) hue -= 6;
  if (hue < 1) return (t2 - t1) * hue + t1;
  else if(hue < 3) return t2;
  else if(hue < 4) return (t2 - t1) * (4 - hue) + t1;
  else return t1;
}

avh4 commented 6 years ago

I'm picking this up for a bit to make an attempt at implementing the algorithm from the CSS spec.

avh4 commented 6 years ago

It appears the the examples in the CSS spec do not match the algorithm in the CSS spec. I filed an issue with the spec here: https://github.com/w3c/csswg-drafts/issues/3088

avh4 commented 6 years ago

I implemented the algorithm from the CSS spec, which gets within 3/255 of the RGB values in the spec examples, so I just set the tolerance of the tests to that for now.

avh4 commented 6 years ago

Okay, I'm done working on this for the morning.

2mol commented 6 years ago

That's great, thanks for your work!

avh4 commented 6 years ago

Picking this up again to try adding toHsla

avh4 commented 6 years ago

Done with this for now again :) I guess the remaining question is, is there an authoritative reference for HSL <-> RGB conversion?

avh4 commented 6 years ago

Got confirmation from w3c that the examples in the spec are wrong, but the algorithm is correct.
I've picked up working on a script to produce a correct reference file based on the algorithm.

avh4 commented 6 years ago

I think this all works now, though there are still probably improvements to the code and documentation to be made. But I'm going to merge this, and further discussion can be done on #9, or on PRs that make additional improvements.

avh4 / elm-color

HSL conversions #8