Add Oklab and Oklch - Githubissues

eprovst commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.)

tpapp commented 8 months ago

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.

kimikage commented 4 months ago

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.

eprovst commented 4 months ago

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

kimikage commented 4 months ago

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.

codecov[bot] commented 4 months ago

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

```diff @@ 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.

eprovst commented 4 months ago

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

eprovst commented 4 months ago

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 commented 4 months ago

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

kimikage commented 4 months ago

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.

JuliaGraphics / Colors.jl

Add Oklab and Oklch #533

Codecov Report