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 more color spaces: XYZ, LAB, polarLAB, LUV, polarLUV #4

Merged
merged 21 commits into from Oct 16, 2019

Conversation

Vindaar
Copy link
Contributor

@Vindaar Vindaar commented Aug 5, 2019

I originally just wanted to add HCL (polar LUV) support to the library (as done in commit: c1d88d6).

However, while trying to add the inverse transformation RGB -> HCL, I realized it'd be nicer to add the required colorspaces that serve as the intermediate representations too. So I went and ported the relevant C code from the R library colorspaces (see here:
https://github.com/cran/colorspace/blob/master/src/colorspace.c)
to Nim.

I did some refactoring to keep the code somewhat clean. However, doing so probably has broken the docs somewhat.

There are now transformation procs according to <space1>_to_<space2> found in transformations.nim. These are called in as<space>, which convert SomeColor to <space>. Finally those are combined in a user faced to(c: SomeColor, toColor: typedesc) proc.

For convenience the old color space naming procs, e.g. hsl, rgb and their inverse color procs are still available, but are now generated using a macro.

Conversion from RGB to HCL is still missing, plus some explanations.

Code is taken straight from R's grDevices module in color.c:
https://svn.r-project.org/R/trunk/src/library/grDevices/src/colors.c
Some stuff is still broken and refactoring is necessary.
The definitions for the existint ColorHSL and the added HLS_to_RGB and
RGB_to_HLS expect different color ranges. The latter taken from
colorspaces.c (the R package's C code) takes the ranges from 0 to 1,
for L and S, whereas the ColorHSL uses 0 to 100.
The ported code now states as such and is separated from the code
already contained in chroma or new code.
@Vindaar Vindaar changed the title Add more color spaces: XYZ, LAB, polarLAB, LUV, polarLUV [WIP] Add more color spaces: XYZ, LAB, polarLAB, LUV, polarLUV Aug 9, 2019
@Vindaar
Copy link
Contributor Author

Vindaar commented Aug 9, 2019

I just noticed that at least some of the color transformations behave weirdly. I'll have to take a look at that, before I consider this ready to be merged.

Now also generate procs to define a color directly from the fields
without having to create a Color<...> object first, e.g.
```nim
proc hsl(h, s, l: float32): ColorHSL
```
Started comparing directly with the output of R colorspaces. Taking
some 10 colors and calculate values in different color space. Then
compare those.
@Vindaar
Copy link
Contributor Author

Vindaar commented Aug 10, 2019

I'll add more tests comparing the transformations with output from the R colorspace library in the next few days.
I fixed the main bug. I missed the linearization when converting RGB to XYZ values. But maybe more are lurking.

I also added an additional convenience proc, that mimics the existing rgb(r, g, b: uint8) proc for all types. For now I commented the equivalent rgb and rgba procs out, because otherwise we get a redefinition error. I wasn't sure whether you'd like to have those (for the sake of the docs) still in there or not. All macro generated procs obviously won't show up in the documentation.

Also, if you want I can add a travis script to run CI to this PR. Then you'd only have to activate the CI for the repository.

This proc is not automatically generated from the macro, because
ColorHCL is an alias for ColorPolarLUV.
Sorry for the macro code here in a test case :/
It generates almostEqual comparisons for the `asXXX.asYYY...`
transformations, `to(...).to(...)` trafos and a comparison of the
`as...` with the `to...` transformation.
@Vindaar Vindaar changed the title [WIP] Add more color spaces: XYZ, LAB, polarLAB, LUV, polarLUV Add more color spaces: XYZ, LAB, polarLAB, LUV, polarLUV Aug 11, 2019
@Vindaar
Copy link
Contributor Author

Vindaar commented Aug 11, 2019

Ok, I fixed everything I could find for now. Every added transformation (plus HSL, HSV) now has a test case comparing it with the output of R's colorspace module and all match.

@treeform
Copy link
Owner

Wow this is big. This might take some time to review, but its going in.

Copy link
Owner

@treeform treeform left a comment

Choose a reason for hiding this comment

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

I think it's my bug, some tests fail in JavaScript mode:

nim js -r test_colors.nim 
[Suite] functions
  [FAILED] darken
  [OK] lighten
  [OK] saturate
  [OK] desaturate
  [FAILED] spin
  [OK] mix

But they pass in c mode - so maybe thats fine?

src/chroma.nim Outdated
result.b = float32(c.b) / 255
result.a = 1.0

#proc rgb*(r, g, b: uint8): ColorRGB =
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe just remove it if we don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, only left it in to not forget discussing this. Will remove it later.

src/chroma.nim Outdated
## * rgba(0,0,255) -> blue
## * rgba(0,0,0,255) -> opaque black
## * rgba(0,0,0,0) -> transparent black
#proc rgba*(r, g, b, a: uint8): ColorRGBA =
Copy link
Owner

Choose a reason for hiding this comment

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

same remove it?

b*: float32 ## blue (0-1)
a*: float32 ## alpha (0-1, 0 is fully transparent)

InvalidColor* = object of Exception
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe move this to the bottom, so that all color objects are in the same place?

@@ -117,6 +119,223 @@ suite "spaces":
#echo "YUV", c, " -> ", yuv(c)
assert c.almostEqual(yuv(c).color())

test "Space transformations using `as*` procs":
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for writing test.

@Vindaar
Copy link
Contributor Author

Vindaar commented Aug 13, 2019

Ok, I've addressed your comments regarding the procs and the exception.

I haven't checked the broken tests for the JS target yet. Do they work on the master branch?

@treeform
Copy link
Owner

I have checked the JS target and yes they are broken on master as well. Don't worry about them for now.

@Vindaar
Copy link
Contributor Author

Vindaar commented Sep 9, 2019

@treeform: anything else you'd like me to do for this? :)

@treeform treeform merged commit 415a4f2 into treeform:master Oct 16, 2019
@treeform
Copy link
Owner

Hey @Vindaar,

Using simple

proc rgba*(c: Color): ColorRGBA {.inline.} =
  ## Convert Color to ColorRGBA
  result.r = uint8(c.r * 255)
  result.g = uint8(c.g * 255)
  result.b = uint8(c.b * 255)
  result.a = uint8(c.a * 255)

proc color*(c: ColorRGBA): Color {.inline.} =
  ## Convert ColorRGBA to Color
  result.r = float32(c.r) / 255
  result.g = float32(c.g) / 255
  result.b = float32(c.b) / 255
  result.a = float32(c.a) / 255

Result in over 50% speed improvement over the macro generated code.

I tried adding inline to the macro generated code and its still kind of slow.

Do you have any ideas on how to improve performance of macro generated code?

@Vindaar
Copy link
Contributor Author

Vindaar commented Nov 30, 2020

Hm, this is peculiar. Sorry about that.

Not sure what's going on, but I suppose it's rather related to the fact that we have the intermediate to proc?

I'll try to take a look at it in the next few days. Please ping me again, if I forget.

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

2 participants