Pipe text color information to rasterize_glyphs #748
Conversation
|
@changm Looks like there are compile errors on travis / appveyor. |
|
I think you should be able to implement |
changm
force-pushed
the
coloru
branch
3 times, most recently
from
972114e to
81d7bf5
Compare
|
☔ The latest upstream changes (presumably #756) made this pull request unmergeable. Please resolve the merge conflicts. |
kvark
left a comment
kvark
left a comment
There was a problem hiding this comment.
Looks good, just have one concern.
webrender_traits/src/types.rs
Outdated
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| pub fn from_colorF(color: ColorF) -> ColorU { |
There was a problem hiding this comment.
Typically, in Rust this is done by implementing the From and Into traits:
impl From<ColorF> for ColorU {}
impl Into<ColorF> for ColorU {}| known_heap_size!(0, ColorF); | ||
|
|
||
| #[derive(Clone, Copy, Hash, Eq, Debug, Deserialize, PartialEq, PartialOrd, Ord, Serialize)] | ||
| pub struct ColorU { |
There was a problem hiding this comment.
It feels slightly wrong that both ColorU and ColorF are exposed in webrender_traits. Shouldn't one of them be just an implementation detail (hidden inside webrender)?
There was a problem hiding this comment.
We need it for GlyphKey :(. Otherwise yes, I would've just passed ColorF around until we were inside webrender.
webrender_traits/src/types.rs
Outdated
|
|
||
| impl ColorU { | ||
| fn round_to_int(x: f32) -> u8 { | ||
| assert!((0.0 <= x) && (x <= 1.0)); |
There was a problem hiding this comment.
You may want to make these debug_assert! instead (assertions run in release by default in rust).
|
Thanks @changm ! |
|
📌 Commit 2e8f719 has been approved by |
|
⚡ Test exempted - status |
Pipe text color information to rasterize_glyphs @kvark r? Required to do the gamma look up table stuff. The unfortunate side effect of this is that the Glyph keys require ColorU as u8 rgb channels instead of floats since f32 doesn't implement std::eq :/. Also, all the other API calls expects color information as ColorF. Instead of making users of the api give text as ColorU, we just translate it into/out of ColorU when we create the glyph key. I'd be happy if you become picky again and find a better way :) <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/748) <!-- Reviewable:end -->
5 participants
@kvark r? Required to do the gamma look up table stuff. The unfortunate side effect of this is that the Glyph keys require ColorU as u8 rgb channels instead of floats since f32 doesn't implement std::eq :/. Also, all the other API calls expects color information as ColorF. Instead of making users of the api give text as ColorU, we just translate it into/out of ColorU when we create the glyph key. I'd be happy if you become picky again and find a better way :)
This change is