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

Add support for read and write of Photoshop Color Swatches #589

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

warrengalyen
Copy link
Contributor

No description provided.

@flabbet
Copy link
Member

flabbet commented Dec 14, 2023

Copic 358 Color Swatch Photoshop.zip

I tested with above palettes and parser doesn't seem to work with them.

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Dec 14, 2023

Sorry about that, I had a typo my last commit. I forgot to double check loading after refactoring. Fixed.

@Equbuxu Equbuxu self-requested a review December 16, 2023 19:10
Copy link
Member

@Equbuxu Equbuxu left a comment

Choose a reason for hiding this comment

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

  • I'm getting an IndexOutOfRangeException when saving any palette as .aco. The colorValues array is created with a size of 3 but a few lines later the code attempts to update it's 4th element.

  • Despite of the error message being displayed after the exception is caught, a (broken) file is still written to the hard drive.

  • I exported a few of the default palettes from photoshop an imported them into PixiEditor, and some of them didn't get loaded correctly. The issue seems to be related to non-rgb color spaces. Here are the files:
    testpalettes.zip

In paint samples.aco a yellow (#ffba01 according to photoshop) HSB color is interpreted as pure white, this is likely because you are using a function that works with HSL colors (HSL and HSB aren't the same color spaces).
All other palettes aren't loaded correctly, the colors are wrong.

  • It seems like you are using code from this page as reference. It's totally fine, but if I'm correct could you please add that link somewhere in the code. This applies to other PRs as well

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Dec 16, 2023

Thanks for reviewing! I forget to change the array initialization in the save function as well. Adobe formats requires four color values to be written out. :P As for the color spaces, you are absolutely right. It's easy to get those color spaces mixed up sometimes. HSB only carries the blackness, while HSL carries both white and black . Very similar but slightly different calculations to RGB.
I've created a new class to handle the colorspace. The file you mentioned paint samples.aco is almost loading correctly except it's now showing that yellow color as off by 1 digit as you see below. Not sure what is happening, my calculations look correct but there maybe a floating point conversion issue I'm not seeing. I don't actually have a Photoshop license but use GIMP instead and it's showing that color correctly.

image

I noticed there is an issue too with loading your test palettes from a LAB color space. Every color is being translated to black. I'll have to investigate this, as it was working with my test files. May have to revise my CIELab and XYZ implementations.

Also thank you pointing out the missing reference, I overlooked those those in the file headers. Will get them all added in ASAP.

@flabbet flabbet requested a review from Equbuxu January 3, 2024 17:27
@flabbet
Copy link
Member

flabbet commented Jan 3, 2024

@Equbuxu Mind looking at that? I don't have Photoshop atm :x

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Jan 3, 2024

@flabbet Sorry, forgot to update the status after my changes. I also don't have Photoshop, relying on GIMP for testing. @Equbuxu The new conversion code appears ot be working correctly with CMYK and LAB. HSB looks to be working too except for that one color you mentioned is still off. I can't seem to figure out what is going on. Doesn't make sense. I checked my HSB algorithm and it's inline with other implementations I've found.

@Equbuxu
Copy link
Member

Equbuxu commented Jan 13, 2024

I've pushed some changes, fixing some of the issues:

  • HSB values weren't scaled right, the algorithm expects 0-1 instead 0-100
  • Negative Lab values were interpreted as large positive ones (the values are read from stream as unsigned shorts, but in this case they should be interpreted as signed shorts)
  • The constructor of PaletteColor accepts values in order of R, G, B, but the actual values passed were as B, G, R (wrong order)
  • The colors returned from the LabToRGB function were quite a bit off from what you can see in photoshop. I tweaked it to no end, some of the issues were caused by incorrect rounding, but that wasn't the biggest problem. As far as I can tell, photoshop doesn't use the most common standard of Lab (D65, 2 degrees observer). I changed it to D50 with 2 degrees (also somewhat common according to wikipedia), now the colors are almost right. I suspect that the observer value needs to be changed from 2 to 10 degress to make it fully align with photoshop, but I can't wrap my head around the math required to calculate the conversion matrix
  • CMYK values are currently also nowhere near what's in photoshop, I haven't looked into them

Note: I tried importing the palettes into gimp, and it skips some of the colors (I think Lab colors are the ones it skips?). The CMYK colors are also different compared to photoshop, so GIMP doesn't seem like the best reference

Useful links:
A great color space converter, useful as reference: https://www.easyrgb.com/en/convert.php#inputFORM
A Lab matrix calculator: https://www.russellcottrell.com/photo/matrixCalculator.htm
Some pre-calculated matrices: http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html
Reference white values for D50 10 degrees (I can only hope they are correct): https://stackoverflow.com/questions/76196116/adapt-a-cie-xyz-color-from-an-observer-illuminant-to-another-one

Copy link
Member

@Equbuxu Equbuxu left a comment

Choose a reason for hiding this comment

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

see above

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Jan 14, 2024

Thanks for taking a look and fixing! I finally installed a trial of Photoshop and can see where the LAB and CMYK colors are still quite a bit off. I tried adjusting the calculations using new white points for D50 10° (still pretty sure it uses 2° though), but the corresponding colors still were off from Photoshop. I think this largely has to do with the fact that its CMS handles all color conversions.

I was hoping doing a manual conversion would work but I honestly don't think this is the best approach now. It would be better to implement a color manage system to achieve accurate color reproduction, particularly with LAB and CMYK color spaces. I'm going to do a bit of experimenting with Windows ICM to see if I can achieve better results. I would suggest going with Little CMS though, since it's pretty robust and open source. This is the same CMS Krita uses. I can even create .NET bindings if needed since it's a C library.

@Equbuxu
Copy link
Member

Equbuxu commented Jan 14, 2024

but the corresponding colors still were off from Photoshop.

Something really important I forgot to mention, you MUST change the color space of your current document in photoshop from Adobe RGB (the default for some reason) to sRGB 61966-2-1:1999. I don't know why photoshop defaults to their own RGB (even for pngs created in other apps, resulting in them looking different in photoshop), but it causes A LOT of the colors to be off. In my last commit, all Lab colors I tested lined up, except for a single one, which was off by one (if I recall correctly it was a green color from photo filters.aco, with one value being 180 vs 181)

@Equbuxu
Copy link
Member

Equbuxu commented Jan 14, 2024

Windows ICM

I haven't heard about Windows ICM, but since we are going cross platform in #573, please don't introduce new dependencies to stuff that only runs on windows (of course I'm only assuming that Windows ICM is windows-specific).

I also haven't heard about Color management systems in general, but on the surface level it does sound like something that could be useful both here and in other places in the app

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Jan 14, 2024

The current version of Photoshop already defaults to that sRGB working space on my system, but still seeing colors off. I have some experience working with color management systems, so familiar with ICC profiles. I was only suggesting Windows ICM for testing as it's easier to use, but I would use Little CMS as an application-wide cross platform solution in the future. This is a good use anywhere there is color conversion being performed.

If we want to move forward with ACO support, I can simply skip LAB and CMYK color mode support for the time being.

@Equbuxu
Copy link
Member

Equbuxu commented Jan 14, 2024

The current version of Photoshop already defaults to that sRGB working space on my system, but still seeing colors off.

I see, interesting

If we want to move forward with ACO support, I can simply skip LAB and CMYK color mode support for the time being.

I'd say it would be better to implement the format fully, but it's open to discussion

I have some experience working with color management systems, so familiar with ICC profiles. I was only suggesting Windows ICM for testing as it's easier to use, but I would use Little LMS as an application-wide cross platform solution in the future. This is a good use anywhere there is color conversion being performed.

This sounds like a good idea to me, @flabbet what do you think

By the way, almost all development discussions happen on our discord server (in a less formal way, in chat), so if you use discord, you should join the server, everyone will see messages and respond a lot quicker there

@flabbet
Copy link
Member

flabbet commented Jan 14, 2024

Using proper manager for color management sounds good, although I can't find any info about Windows ICM or Little LMS? Do you mind sharing some resources?

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Jan 14, 2024

https://littlecms.com/color-engine/ (recommended since it's not windows dependent)
I would be happy to write an interface class for the C API.

@warrengalyen
Copy link
Contributor Author

warrengalyen commented Jan 15, 2024

@Equbuxu I did quite a bit of experimenting with CMYK and was able to improve the algorithm and achieve what I believe to be quite a bit closer results to PS without use of an ICC profile. But the colors will never match 1:1 even with profiles because of the nature of the color spaces.

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.

None yet

3 participants