Merge TaggedColor and CssColor #25
Conversation
color/src/dynamic.rs
Outdated
| ColorSpaceTag::Oklab | ||
| | ColorSpaceTag::Oklch | ||
| | ColorSpaceTag::Lab | ||
| | ColorSpaceTag::Lch => self.map(|l, c1, c2, a| [f(l), c1, c2, a]), |
There was a problem hiding this comment.
This is a bad merge, right?
color/src/missing.rs
Outdated
| self.0 |= 1 << ix; | ||
| } | ||
|
|
||
| pub fn single(ix: usize) -> Self { | ||
| /// The set containing a single component index. | ||
| pub fn singleton(ix: usize) -> Self { |
There was a problem hiding this comment.
singleton seems like a confusing name for this. This usually implies what in Rust would be a static instance. I would prefer the old name of single. Or perhaps an idiomatic rusty name would be with_single_component.
There was a problem hiding this comment.
I've gone back to single, at your suggestion.
| /// A simple bitset, for representing missing components. | ||
| /// A simple bitset for representing missing components. | ||
| #[derive(Default, Clone, Copy, PartialEq, Eq, Debug)] | ||
| pub struct Bitset(u8); | ||
| pub struct Missing(u8); |
There was a problem hiding this comment.
Would it be clearer if this represented present (non-missing) components? 0 to represent "present" and 1 to represent "missing" seems somehow inverted. Presumably this is equivalent on a technical level?
There was a problem hiding this comment.
Perhaps "missing" semantics make sense because the common case is "no missing components"? If so then perhaps the code could stay the same, but the method could be documented in terms of "which components are present (non-missing)" rather than (or in addition to) "which components are in the set".
e.g. for is_empty:
Returns `true` if the set contains no indices (and therefore all color components are present).
There was a problem hiding this comment.
Would also appreciate an/some example(s) of when components can be missing.
There was a problem hiding this comment.
I'll add some docs about missing components.
Inverting the boolean sense is equivalent at a technical level. I think the strongest case for this way ("missing") is that the default value of 0 makes sense.
Incidentally, I considered and rejected some other representations for this. In particular, color.js uses NaN. That works in pure Rust, but I have at least two problems with it: it complicates GPU interop, because NaN is almost always a bad thing), and it makes the test for "any missing components" more expensive, because you have to do a finiteness check on all the components, as opposed to comparison with 0 here. The current approach also has no memory cost, DynamicColor is 20 bytes either way.
Rename `Missing::singleton` back to `single`, and add explanation of missing components.
As per discussion, the new name is
DynamicColor, following the precedent of theimagecrate.Also do some renames and add some docstrings.
Closes #21. The rename of Bitset closes #17.