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

Gradient Part 1 - Color rework #2171

Merged
merged 30 commits into from Sep 19, 2023
Merged

Gradient Part 1 - Color rework #2171

merged 30 commits into from Sep 19, 2023

Conversation

Dherse
Copy link
Sponsor Collaborator

@Dherse Dherse commented Sep 17, 2023

Tracking issue: #2282

This PR does some fundamental changes to the way colors work, as well as introducing several breaking changes. Following is the list of changes in no particular order:

  • Colors are now all with 64-bit precision: this avoids some f64 -> f32 -> f64 precision issues (when converting from a color to an array) and allows for maximum precision when converting between color spaces
  • Added the following color spaces: HSV, HSL, Oklab, and linear-RGB (non gamma-corrected RGB)
  • Added the following PostScript functions in PDF export: Oklab to linRGB, HSV to sRGB, HSL to sRGB
  • Added the following ICC profiles in PDF export: sRGB and Grayscale (this should fix colors looking different based on the reader), they both fallback to DeviceRGB and DeviceGray with the correct D65 white point
  • New dependency on palette, a pure-rust crate that provides tons of color conversion utilities
  • Added the following color constructs: oklab, linear-rgb, hsl, and hsv
  • Added the following method to color: components which replaces the old to-rgba, to-cmyk, and to-luma. This is a breaking change. These return an array of the color components in the target color space (i.e luma, cmyk, etc.).
  • Color space conversion can now be done using the constructors i.e they take color components or a color: rgb(hsv(10deg, 10%, 40%)) converts an HSV color into an RGB color.
  • lighten, darken are now based on the perceived lightness value and handled directly inside of palette.
  • Added the following method to color: rotate which allows the rotation of the hue by a given angle
  • Mix now allows more color spaces for mixing of colors and keeps the color in the mixing space: this change avoids chained inaccuracies when converting from colors back and forth. This is a breaking change.
  • PDF export now encodes colors in their target color space, using ICC profiles and/or PostScript function for correct color conversions (writing the PostScript code was... difficult)
  • The default colors Black, White, and Gray are now based on a LumaColor instead of being RGB, this should save a small amount of PDF file size, which may be quite large over an entire document
  • Added relevant tests for all methods and for changes
  • Changed PDF export to only include the necessary color spaces instead of always containing them all, slight file size optimization
  • Added dedicated SVG color export for the following spaces: sRGB, linear-RGB, oklab, and HSL since those are natively supported by the SVG spec. The other color space fallback to a hex sRGB value.
  • Added saturate and desaturate methods on colors, they increase/decrease the saturation based on a percent value.

Color precision

It was quite hard to find a good balance for color precision, in the end, the solution that worked best was f64 colors simply because typst's floats are also f64s and it solves some issues when comparing numbers. However, in general, I would highly recommend against comparing color values purely based on the numerical values as colors are converted lossily

ICC profiles

The ICC profiles are tiny (especially once delfated) accounting for at most ~500 bytes

PostScript function

Have been tested to be correct (although automated testing using GhostScript might be desirable), are written with comments. The comments, line breaks, etc. are stripped before being deflated for size optimization

Gradients

This is the first PR towards gradient, and part of a series of 5 PRs (Colors, Layout & Export, Shape gradients, Text gradients, Fine control of gradients) that will allow gradients to be added to typst.

Printing

On my printer (a cheapo Brother), all color spaces print just fine.

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 17, 2023

As an example of color precision issues, here is the falling test at the time of writing:
image

It works just fine on my machine which is a x64 AMD CPU, why it fails I do not know. I will edit tests to work at a purposefully lower precision

@freundTech
Copy link
Contributor

Do you think it would make sense to put all the color constructors into a new module color instead of having them as top level functions?

There's quite a few of them (oklab, rgb, linear-rgb, cmyk, hsl, and hsv)

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 17, 2023

I actually agree with you that the most important ones should be at the top level (imo that would be rgb, oklab, cmyk, and luma) relegating "second-class" constructors (so linear-rgb, hsv, and hsl) inside of the module. The reasoning is that they are terrible color spaces to use directly, and are more intended to be used inside of the gradients (some gradients work really well using just hue rotation).

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 17, 2023

Before merging: there is a mistake in the HSL PostScript code, some ranges are mixed up, I'll fix it later today

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 17, 2023

Okay, I have fixed all the PostScript code and it should be pretty much ready for review!

@istudyatuni
Copy link
Contributor

Suggestion: instead of having many as- functions, add to color components() method (or similar)

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 17, 2023

Suggestion: instead of having many as- functions, add to color components() method (or similar)

Initially I wanted to call them rgba etc. but perhaps simply components would work well

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 17, 2023

Suggestion: instead of having many as- functions, add to color components() method (or similar)

@istudyatuni I ended up implementing this change as I thought it was a much better implementation. To obtain the same effect as the as, the color conversion is now explicit luma(40).to-rgba().components() and also allows the user to select whether they care about the alpha channel or not!

@reknih
Copy link
Member

reknih commented Sep 18, 2023

lighten and darken should be perceptual for all color spaces.

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 18, 2023

lighten and darken should be perceptual for all color spaces.

What I meant by that is that I didn't change it for RGB, etc. and I am not sure whether those implementation were perceptual *

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 18, 2023

Following some discussion on Discord, I removed the to-<color-space>() methods, instead allowing the constructors to perform that conversion like rgb(hsl(10deg, 20%, 30%)) to convert from HSL to RGB.

Copy link
Member

@laurmaedje laurmaedje left a comment

Choose a reason for hiding this comment

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

Wow! Looks really good overall, I just have some minor nits + a hope (that will probably soon be shattered) that generics could drastically reduce the amount of code in color.rs: The whole file has quite a lot of methods for conversion / modification and dispatch to subcolors. However, palette has a quite powerful trait-based interface from the looks of it. Could we build on that to prevent this n^2 code size (n = number of color types). E.g. by bounding the ColorExt trait on Desaturate + FromColor<HsvColor> + ...?

crates/typst-library/src/lib.rs Outdated Show resolved Hide resolved
crates/typst/Cargo.toml Outdated Show resolved Hide resolved
crates/typst/src/export/pdf/color.rs Outdated Show resolved Hide resolved
crates/typst/src/export/pdf/color.rs Outdated Show resolved Hide resolved
crates/typst/src/export/pdf/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 18, 2023

@laurmaedje in tests, I added a special test-repr method to avoid having to write test(repr(a), repr(b)) all the time for colors, it makes tests a lot more understandable.

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 18, 2023

Regarding improving things using trait, I have tried A L-O-T on an uncommitted version, and the best I could achieve was a 100-ish line reduction. The main reason why I am not using the palette structs directly is because they're not Hash nor Eq. And these are required for Value. The biggest gains are just by replacing all of the length match self in the main Color struct with a macro to delegate which does help quite a bit.

crates/typst/src/export/pdf/color.rs Outdated Show resolved Hide resolved
crates/typst/src/export/pdf/color.rs Outdated Show resolved Hide resolved
crates/typst/src/export/pdf/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
tests/src/tests.rs Outdated Show resolved Hide resolved
tests/src/tests.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
crates/typst/src/geom/color.rs Outdated Show resolved Hide resolved
@laurmaedje
Copy link
Member

Also, now that I see them, the top level icc and post-script folders don't seem right in typst. I would be okay with one of two options:

  • Move both in typst/assets (I know I said PS code isn't an assets, but whatever)
  • Move them into src/export/pdf/{icc|postscript}
    I think postscript without a hyphen is okay too.

@laurmaedje laurmaedje merged commit 163c2e1 into typst:main Sep 19, 2023
3 checks passed
@laurmaedje
Copy link
Member

Thank you! I'm really happy with the result. :)

@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Sep 19, 2023

Thank you! I'm really happy with the result. :)

@laurmaedje I am really glad this big chunk of gradients is merged 🎉

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

5 participants