Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gamutmax(LCHab) disagrees with the Colors.jl usage #125

Closed
kimikage opened this issue Sep 1, 2019 · 4 comments · Fixed by #254
Closed

gamutmax(LCHab) disagrees with the Colors.jl usage #125

kimikage opened this issue Sep 1, 2019 · 4 comments · Fixed by #254
Milestone

Comments

@kimikage
Copy link
Collaborator

kimikage commented Sep 1, 2019

The max saturation of LCHab is defined as 1 in ColorTypes.jl.

gamutmax(::Type{T}) where {T<:LCHab} = (100,1,360)
gamutmin(::Type{T}) where {T<:LCHab} = (0,0,0)

However, in Colors.jl, the saturation comes from the a and b of Lab which are mostly in [-100,100].
https://github.com/JuliaGraphics/Colors.jl/blob/657e265a4cccc9fc32584f7f3505611537bfc79d/src/conversions.jl#L575-L580

Therefore, for the sake of convenience, 100 (or 128) is better for the max saturation, though the actual gamut is not so simple. (See also: #84)

@timholy
Copy link
Member

timholy commented Sep 4, 2019

Another option is to remove support for ones without a clearly defined max/min?

@kimikage
Copy link
Collaborator Author

kimikage commented Sep 4, 2019

In the case of LCHab, the removal is probably OK, as no one has pointed this out.

However, few types have a clearly defined max/min, because the range of color space and the color gamut are based on different concepts.

BTW, I found this issue when I ran rand(LCHab).
I think gamutmax and gamutmin are practical when and only when they are used with rand() as historically they are introduced for such a purpose.
Perhaps what we should remove might be the term “gamut” rather than the function.

@timholy timholy added this to the 1.0 milestone Nov 22, 2019
@timholy timholy mentioned this issue Nov 22, 2019
5 tasks
@kimikage
Copy link
Collaborator Author

As mentioned above, I think that "color gamut" and rand should be separated.
Then, rand should be more comprehensive. For example, there is a problem with TransparentColor:

julia> rand(RGBA)
RGBA{Float64}(0.4054196785489861,0.5812337734513682,0.4739237429262706,0.35592317884458446)

julia> rand(Lab)
Lab{Float64}(49.35707185830977,54.78932461097497,-80.6404236285697)

julia> rand(LabA)
ERROR: MethodError: no method matching gamutmax(::Type{LabA{Float64}})

Although it is necessary to discuss whether ColorTypes.jl should specify the ranges (not gamuts) or not, it may be the duty of ColorTypes.jl to resolve the range of AlphaColor and ColorAlpha, given the range of color_type.

@kimikage
Copy link
Collaborator Author

kimikage commented Dec 7, 2019

BTW, gamutmin/gamutmax for YIQ are broken, too.

gamutmax(::Type{T}) where {T<:YIQ} = (1,0.5226,0.5226)
gamutmin(::Type{T}) where {T<:YIQ} = (0,-0.5957,-0.5957)

correct_gamut(c::YIQ{T}) where {T} = YIQ{T}(clamp(c.y, zero(T), one(T)),
                                     clamp(c.i, convert(T,-0.5957), convert(T,0.5957)),
                                     clamp(c.q, convert(T,-0.5226), convert(T,0.5226)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants