Populate first draft of color crate #3

raphlinus · 2024-11-04T19:08:35Z

This is copied from Raph's "koloro" local repository, with lots of fiddly changes to satisfy clippy.

raphlinus · 2024-11-04T20:22:50Z

A few more things to say about this. A few things are still in flux, though overall I'm happy with the direction it's going. I am especially curious to learn whether it meets the needs of potential users, or has some gaps.

Not all the CSS color spaces are done. Adding those should be fairly straightforward.

This crate would benefit from extensive testing. There are a bunch of tests in the CSS Color 4 spec, and likely many of those could be adapted without much trouble. In general, the color.js library can be considered a source of truth, and perhaps tests could be written to test whether this library gives consistent results (I'm thinking of the extremely extensive fuzz testing against the reference cmark implementation in pulldown-cmark, which also tests against a JS ref).

The TaggedColor and CssColor types should almost certainly be merged, I don't think there's significant value in having them separate. The main difference is that the former doesn't have the concept of missing channels. I think the runtime cost of supporting them is minimal.

There are a few other todos and discussion questions in the code.

Discussion is welcome! There's more than a bit of background in a Zulip thread.

xStrom

I did a quick pass and didn't find any obvious faults. Did not verify magic numbers or color theory.

Given the fresh nature of the repo, I think it's fine to merge and do improvements in follow-up PRs.

xStrom · 2024-11-04T20:29:35Z

color/src/color.rs

+    #[expect(
+        clippy::suspicious_arithmetic_impl,
+        reason = "somebody please teach clippy about multiplicative inverses"
+    )]
+    fn div(self, rhs: f32) -> Self {
+        self * rhs.recip()
+    }


DJMcNab

I tend to agree that landing is probably reasonable.
I do intend to do a proper code style review pass tomorrow.

The one other thing to check is whether the AUTHORS file matches your expectations

raphlinus · 2024-11-04T21:57:39Z

Great, I'll merge as-is, then expect to do quite a bit of followup in other PRs.

The AUTHORS file seems fine to me. That said, in the various README and docstring (which is known incomplete) I do want to acknowledge the inspiration of other libraries, especially bevy_color, but also palette and color.js.

DJMcNab

A single-pass review sweep.

DJMcNab · 2024-11-05T08:47:42Z

color/src/color.rs

+/// that "opaque" refers to the color, not the representation; the components
+/// are publicly accessible.
+#[derive(Clone, Copy, Debug)]
+pub struct OpaqueColor<T> {


I wonder if we should use more descriptive generic names here (e.g. OpaqueColor<Space>)

I agree T is bad here, as it's not a generic container, but I wonder whether CS would be just as clear and more concise. I'll put CS in my PR, then we can bikeshed.

DJMcNab · 2024-11-05T08:48:59Z

color/src/color.rs

+
+/// The hue direction for interpolation.
+///
+/// This type corresponds to `hue-interpolation-method` in the CSS Color


These sorts of references should have an MDN link - https://developer.mozilla.org/en-US/docs/Web/CSS/hue-interpolation-method

DJMcNab · 2024-11-05T09:02:21Z

color/src/color.rs

+    }
+}
+
+pub(crate) const fn split_alpha(x: [f32; 4]) -> ([f32; 3], f32) {


This could potentially use [x, y, z, a]: [f32; 4]. This should only be a code-style thing.

DJMcNab · 2024-11-05T09:12:58Z

color/src/color.rs

+    /// Interpolate colors.
+    ///
+    /// Note: this function doesn't fix up hue in cylindrical spaces. It is
+    /// still be useful if the hue angles are compatible, particularly if


It is still be useful

DJMcNab · 2024-11-05T09:15:21Z

color/src/color.rs

+}
+
+// Arithmetic traits. A major motivation for providing these is to enable
+// weighted sums, which are well defined when the weights sum to 1. In


I don't have the domain knowledge here, but this sentence suggests that the operations implemented here are potentially suspect, and might be better as methods with documentation and caveats.

At the very least, the impls themselves probably should have documentation explaining some of this.

I've thought about this deeply and feel that arithmetic operations are worthwhile to include. Basically, it enables spline interpolation with a generic spline implementation that has the arithmetic traits as bounds, and to me that upside outweighs any downside. Other color crates impl these as well.

I have moved this comment to documentation.

Seems reasonable. An alternative curve bending design might be to have an Arithmetic<T> wrapper, where you're opting in to thinking carefully about this.

DJMcNab · 2024-11-05T09:33:15Z

color/src/colorspace.rs

+#[derive(Clone, Copy, Debug)]
+pub struct Oklab;
+
+// Matrices taken from Oklab blog post, precision reduced to f32


DJMcNab · 2024-11-05T09:35:04Z

color/src/colorspace.rs

+
+    const LAYOUT: ColorspaceLayout = ColorspaceLayout::HueThird;
+
+    fn from_linear_srgb(src: [f32; 3]) -> [f32; 3] {


In bevy_color, I believe it is possible to convert between Oklab and Oklch directly. Would this be a useful operation to support?

I was wrestling between code size and runtime efficiency. But on reflection, I think it makes sense to have a convert<TargetCS: ColorSpace> method with a default impl. Probably I'll do that in a separate PR to keep the next one focused on cleanup.

DJMcNab · 2024-11-05T09:40:17Z

color/src/css.rs

+impl Interpolator {
+    pub fn eval(&self, t: f32) -> CssColor {
+        let premul = [
+            self.premul1[0] + t * (self.premul2[0] - self.premul1[0]),


It doesn't look like premul2 is ever used except to compute the same difference value here.

Should this difference instead be stored?

As a bit of super-geekery, there are two basic strategies for implementing lerp. One is a + t * (b - a), the other is a * (1 - t) + b * t. The latter has the advantage of zero rounding error at t = 1, but I think for the purposes of this crate one ulp at f32 is completely acceptable.

glam ended up switching to the precise lerp (reported in bitshifter/glam-rs#275, fixed in bitshifter/glam-rs#548 (but some of the concerns about differences in magnitude are different in color code vs generic vectors ...)

Right, for geometry sometimes you really care about watertightness, but for color close is pretty much always good enough.

DJMcNab · 2024-11-05T09:41:56Z

color/src/floatfuncs.rs

+macro_rules! define_float_funcs {
+    ($(
+        fn $name:ident(self $(,$arg:ident: $arg_ty:ty)*) -> $ret:ty
+        => $lname:ident/$lfname:ident;


lname doesn't seem to be used?

Yup, these were adapted from kurbo, but I think we can simplify and only include f32 versions.

DJMcNab · 2024-11-05T09:43:04Z

color/src/gradient.rs

+    end_color: PremulColor<CS>,
+}
+
+pub fn gradient<CS: Colorspace>(


More evidence in favour of ColorSpace in this generic name

In response to feedback on #3.

In response to feedback on #3. One comment that was not addressed was direct conversion between color spaces, as opposed to always going through linear rgb.

raphlinus added 3 commits November 4, 2024 11:06

Populate first draft of color crate

7edce38

This is copied from Raph's "koloro" local repository, with lots of fiddly changes to satisfy clippy.

Add copyright headers

8963546

Wire up nostd in color_operations

36d9223

raphlinus mentioned this pull request Nov 4, 2024

Make suspicious_arithmetic_impl understand multiplicative inverses rust-lang/rust-clippy#13652

Open

xStrom approved these changes Nov 4, 2024

View reviewed changes

DJMcNab approved these changes Nov 4, 2024

View reviewed changes

raphlinus added this pull request to the merge queue Nov 4, 2024

Merged via the queue into main with commit 3502072 Nov 4, 2024
15 checks passed

raphlinus deleted the color branch November 4, 2024 21:59

DJMcNab reviewed Nov 5, 2024

View reviewed changes

raphlinus added a commit that referenced this pull request Nov 5, 2024

Cleanups and renames

b694ace

In response to feedback on #3.

raphlinus mentioned this pull request Nov 5, 2024

Cleanups and renames #7

Merged

github-merge-queue bot pushed a commit that referenced this pull request Nov 5, 2024

Cleanups and renames (#7)

1038a3b

In response to feedback on #3. One comment that was not addressed was direct conversion between color spaces, as opposed to always going through linear rgb.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate first draft of color crate #3

Populate first draft of color crate #3

raphlinus commented Nov 4, 2024

raphlinus commented Nov 4, 2024

xStrom left a comment

xStrom Nov 4, 2024

DJMcNab left a comment

raphlinus commented Nov 4, 2024

DJMcNab left a comment

DJMcNab Nov 5, 2024

raphlinus Nov 5, 2024

DJMcNab Nov 5, 2024

DJMcNab Nov 5, 2024

DJMcNab Nov 5, 2024

DJMcNab Nov 5, 2024

raphlinus Nov 5, 2024

DJMcNab Nov 5, 2024

DJMcNab Nov 5, 2024

DJMcNab Nov 5, 2024

raphlinus Nov 5, 2024

DJMcNab Nov 5, 2024

raphlinus Nov 5, 2024

waywardmonkeys Nov 5, 2024

raphlinus Nov 5, 2024

DJMcNab Nov 5, 2024

raphlinus Nov 5, 2024

DJMcNab Nov 5, 2024


		const LAYOUT: ColorspaceLayout = ColorspaceLayout::HueThird;

		fn from_linear_srgb(src: [f32; 3]) -> [f32; 3] {

Populate first draft of color crate #3

Populate first draft of color crate #3

Conversation

raphlinus commented Nov 4, 2024

raphlinus commented Nov 4, 2024

xStrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

raphlinus commented Nov 4, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment