Use Color to represent colors #48
Conversation
|
A discussion question: should I make solid brush creation non-failable while I'm breaking the API, or is that a separate change? (I'd also want to make text format creation significantly less result-oriented) |
I think you should. |
piet/src/color.rs
Outdated
| } | ||
|
|
||
| /// Opaque white. | ||
| pub const fn white() -> Color { |
There was a problem hiding this comment.
glad that these are const fns. I might go a bit further, though, and add them as associated constants on the struct itself; Color::WHITE, Color::BLACK, etc.
There was a problem hiding this comment.
Cool, that's been stable since 1.20, it's hard to keep track :)
| @@ -87,13 +89,15 @@ pub trait RenderContext { | |||
| /// responsiblity? We could have a cache that is flushed when the Direct2D | |||
| /// render target is rebuilt. Solid brushes are super lightweight, but | |||
| /// other potentially retained objects will be heavier. | |||
| fn solid_brush(&mut self, rgba: u32) -> Result<Self::Brush, Error>; | |||
| fn solid_brush(&mut self, color: Color) -> Self::Brush; | |||
There was a problem hiding this comment.
What if we removed this API, and just had Impl Into<Brush> for Color? I could imagine needing to expose something for gradients (although possibly also not) but I'm not sure that this API is pulling its weight. Is brush creation expensive? Even in that case, we're rarely reusing brushes between draw calls or anything...
There was a problem hiding this comment.
I have a branch where I'm trying to do this, but it turns out to be not anywhere nearly so easy; the SolidColorBrush::create method requires a RenderTarget (and there's some funny stuff about the brush being bound to this render target, but I think I can ease that by using a DeviceContext everywhere - I tested and this does work on Windows 7 SP1).
I did try making an IntoBrush associated trait that would take either an &Brush or a Color, but the type tetris got a bit much for me (the result should be a Cow to handle both cases, and needs to encode the lifetime).
Brush creation in D2D is a COM call that does an allocation, so it's not free. It does feel clunky though; a completely reasonable alternative is for the Rust type to be cheap and then have a cache to D2D brush objects. This goes a bit beyond my original intent of being a thin wrapper over D2D, but maybe in this case the simplification for users is worth it.
.travis.yml
Outdated
| # See https://github.com/gtk-rs/cairo/issues/263 | ||
| env: | ||
| - PKG_CONFIG_PATH=/usr/local/opt/libffi/lib/pkgconfig | ||
|
|
There was a problem hiding this comment.
Probably should have done a rebase instead of a merge, but then it needs a force-push. In any case, it should turn out the same. Ah, git.
Fixes #40