Colors.jl icon indicating copy to clipboard operation
Colors.jl copied to clipboard

Add Oklab and Oklch

Open eprovst opened this issue 1 year ago • 3 comments

Similar to pull request #518, using the terminology of CSS instead and somewhat more complete. In the sense that there are (some) tests and all conversions seem to dispatch correctly.

Potential further work not in this pull request:

  • Some of the algorithms using CIE Lab/Luv could maybe benefit from a version using Oklab.
  • As CIE Lab (wrt. D65) and Oklab are now part of CSS, colorant string parsing could maybe be extended to support the CSS syntax.

I do get a fail of isempty(detect_ambiguities(Colors)), which I do not completely understand, so help there would be appreciated.

Failing CI is related to the fact that ColorTypes v0.12 is not released.

eprovst avatar Aug 01 '23 17:08 eprovst

The failure of isempty(detect_ambiguities(Colors)) seems related to upgrading to ColorTypes v0.12 and not to the other changes in this pull request, so removing the WIP status.

eprovst avatar Aug 07 '23 15:08 eprovst

Modified description to more clearly separate potential improvements from actual contents. I am however not currently planning to add the former, so as far as I am concerned this pull request is complete. (Although someone should maybe verify the detect_ambiguities error first.)

eprovst avatar Aug 13 '23 15:08 eprovst

This would be a great addition (I found out about OKxxx in a recent discussion).

@eprovst, if maintainers are not responding, I wonder if you could just put this in a separate package, called ColorsOK.jl or something, depending on ColorTypes.jl and Colors.jl.

tpapp avatar Feb 05 '24 11:02 tpapp

from a Slack thread: https://julialang.slack.com/archives/CB1R90P8R/p1715468813588759

kimikage Since many of the changes I made 3 years ago will require manual merging, I do not plan to make any changes in v0.12 other than fatal bug fixes and fixing broken tests.

kimikage In other words, I believe there is a demand for Oklab and Oklch support to be backported to v0.12, but I would like to cancel the idea.

Of course, it will do not interfere with others doing it properly. In any case, this PR should be merged into v0.13.0-DEV first, without worrying about the backporting or ColorTypes v0.11.

kimikage avatar May 11 '24 23:05 kimikage

That seems like a reasonable order of actions. If there is anything I should still change, please let me know!

eprovst avatar May 12 '24 21:05 eprovst

I have made a fix so that the CI tests pass, so please rebase and make sure all CI results are green. As for [compat], I think it is better to fit the current status for the convenience, i.e.,:

ColorTypes = "0.11.5"

We will revisit it at the release timing of ColorTypes v0.12 or Colors v0.13, whichever comes first.

Once this PR is merged, I would like to add the charts to the document and visually verify that the conversion works. Let's further extend parse after that.

kimikage avatar May 13 '24 06:05 kimikage

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.91%. Comparing base (f0a508b) to head (ab1590d). Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   98.82%   98.91%   +0.08%     
==========================================
  Files          10       10              
  Lines        1281     1289       +8     
==========================================
+ Hits         1266     1275       +9     
+ Misses         15       14       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 15 '24 06:05 codecov[bot]

~~Suddenly convert(RGB, Oklab{Float64}(0.0,1.0,0.5)) fails... Investigating further.~~ I was on the main branch...... Never mind.

eprovst avatar May 15 '24 06:05 eprovst

I have made a fix so that the CI tests pass, so please rebase and make sure all CI results are green. As for [compat], I think it is better to fit the current status for the convenience, i.e.,:

ColorTypes = "0.11.5"

We will revisit it at the release timing of ColorTypes v0.12 or Colors v0.13, whichever comes first.

Once this PR is merged, I would like to add the charts to the document and visually verify that the conversion works. Let's further extend parse after that.

I already took the liberty to add the charts, which seem alright. Tests also seem to pass.

eprovst avatar May 15 '24 07:05 eprovst

I made the tests a bit stricter. I'll leave organization of the documentation to your discretion :slightly_smiling_face:

eprovst avatar May 15 '24 13:05 eprovst

I am going to merge this. It has been over 2 years counting from PR #518. I apologize for that and express thanks to all involved.

kimikage avatar May 19 '24 07:05 kimikage