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

Close #122 - Add CMYK representation #123

Merged
merged 1 commit into from
May 24, 2020

Conversation

aeter
Copy link
Contributor

@aeter aeter commented Apr 17, 2020

Adds a CMYK representation. References #122

Tested also with https://www.rapidtables.com/convert/color/rgb-to-cmyk.html

Tests:

$ pastel format cmyk 'rgb(255, 255, 255)'
cmyk(0, 0, 0, 0)
$ pastel format cmyk 'rgb(0, 0, 0)'
cmyk(0, 0, 0, 100)
$ pastel format cmyk 'rgb(19, 19, 1)'
cmyk(0, 0, 95, 93)
$ pastel format cmyk blue
cmyk(100, 100, 0, 0)
$ pastel format cmyk 'rgb(55, 55, 55)'
cmyk(0, 0, 0, 78)
$ pastel format cmyk 'rgb(136, 117, 78)'
cmyk(0, 14, 43, 47)
$ pastel format cmyk 'rgb(143, 114, 75)'
cmyk(0, 20, 48, 44)
$ pastel format cmyk 'rgb(143, 111, 76)'
cmyk(0, 22, 47, 44)

@aeter aeter force-pushed the feature/cmyk-representation branch 4 times, most recently from 4a5277a to 2e34be1 Compare April 17, 2020 14:46
@sharkdp sharkdp mentioned this pull request Apr 24, 2020
@sharkdp
Copy link
Owner

sharkdp commented Apr 24, 2020

Thank you for your contribution.

When working on pastel in the past, I have considered adding the CMYK color space, but it was my understanding that there is no such thing as "the RGB <=> CMYK conversion formula" [1]. Another problem is that the gamuts do not overlap with sRGB.

We face similar problems with CIELAB, but at least sRGB's gamut is a subset of the "CIELAB gamut".

Could you please clarify how these issues are addressed here? Where does the conversion formula come from?

[1] https://en.wikipedia.org/wiki/CMYK_color_model#Conversion

@aeter
Copy link
Contributor Author

aeter commented Apr 24, 2020

@sharkdp - as a formula reference during writing the code, I used both the bash script from ticket #122 and https://www.rapidtables.com/convert/color/rgb-to-cmyk.html.


The top SO answer here (https://stackoverflow.com/q/2426432) says -

The conversion from RGB to CMYK is dependent on the physical device/process being used to lay down the CMYK ink. 
These are represented in software as Color Profiles.
ICC and ICM color profiles of physical devices determine the resulting colors.

If you are not concerned with true representation on a physical device then use the direct conversion formulas in other posts.

The used formula does no apply any Color Profile.

I can find the same algorithm either used or mentioned at different places online -

@kyndder
Copy link

kyndder commented Apr 27, 2020

Adds a CMYK representation. References #122

Tested also with https://www.rapidtables.com/convert/color/rgb-to-cmyk.html

Thank you, @aeter !

When working on pastel in the past, I have considered adding the CMYK color space, but it was my understanding that there is no such thing as "the RGB <=> CMYK conversion formula" [1]. Another problem is that the gamuts do not overlap with sRGB.

@sharkdp , I'll follow @aeter and the references provided by him...

As at a first glance this feature will not be used for physical representation, direct conversion would be an entrance for future improvements...

As I mentioned in #122 , due to a very specific issue and for an "on screen" representation provided by a specific software, a direct conversion was all I needed...

But I understand your point... A better approach for physical conversion, would be using Hue but, even this way, we would still dependent on specific profiles to achieve a satisfactory result...

Thank you for the great work, guys!

EDIT

Just for clarification, for example, here's a sample from a device profile, from one of my printers;

DevProfile
My target profile is ISO 12647-2 (ISO Coated v2)...
My device's profile, is the SP7900HT_720x1440...

Based on my profile, theoretically, I can reproduce the whole CMYK gamut, but not sRGB's one...

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - thank you for your explanations!

Tests:
```
$ pastel format cmyk 'rgb(255, 255, 255)'
cmyk(0, 0, 0, 0)
$ pastel format cmyk 'rgb(0, 0, 0)'
cmyk(0, 0, 0, 100)
$ pastel format cmyk 'rgb(19, 19, 1)'
cmyk(0, 0, 95, 93)
$ pastel format cmyk blue
cmyk(100, 100, 0, 0)
$ pastel format cmyk 'rgb(55, 55, 55)'
cmyk(0, 0, 0, 78)
$ pastel format cmyk 'rgb(136, 117, 78)'
cmyk(0, 14, 43, 47)
$ pastel format cmyk 'rgb(143, 114, 75)'
cmyk(0, 20, 48, 44)
$ pastel format cmyk 'rgb(143, 111, 76)'
cmyk(0, 22, 47, 44)
```
@sharkdp sharkdp force-pushed the feature/cmyk-representation branch from 2e34be1 to bb5f180 Compare May 24, 2020 16:28
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2020

Codecov Report

Merging #123 into master will increase coverage by 0.30%.
The diff coverage is 83.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   69.28%   69.59%   +0.30%     
==========================================
  Files          32       32              
  Lines        2676     2723      +47     
==========================================
+ Hits         1854     1895      +41     
- Misses        822      828       +6     
Flag Coverage Δ
#macos_latest 69.64% <83.67%> (+0.30%) ⬆️
#ubuntu_latest 69.64% <83.67%> (+0.30%) ⬆️
#windows_latest 69.63% <83.67%> (+0.30%) ⬆️
Impacted Files Coverage Δ
src/cli/cli.rs 100.00% <ø> (ø)
src/lib.rs 86.49% <83.33%> (+0.02%) ⬆️
src/cli/commands/format.rs 92.68% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a45a7f5...bb5f180. Read the comment docs.

@sharkdp sharkdp merged commit cb4e5fe into sharkdp:master May 24, 2020
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 this pull request may close these issues.

4 participants