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

Use color for color. #63

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

waywardmonkeys
Copy link
Contributor

This leaves peniko::Color for now, but begins the migration to using the new color crate.

@waywardmonkeys
Copy link
Contributor Author

waywardmonkeys commented Nov 23, 2024

Some of these will be resolved in color and some here in this crate:

@waywardmonkeys
Copy link
Contributor Author

I've done more work here updating most of the gradient code, adding a feature to disable the legacy color code, and adding some more comments.

There are some FIXME(color) comments in the code to help point to things that need resolution in conjunction with the list in a comment above.

src/brush.rs Outdated
@@ -37,7 +48,8 @@ impl From<Image> for Brush {

impl Default for Brush {
fn default() -> Self {
Self::Solid(Color::default())
// FIXME(color): Decide what to do about color and defaults
Self::Solid(color::palette::css::TRANSPARENT)
Copy link
Member

Choose a reason for hiding this comment

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

Current Peniko defaults to transparent. Opaque black can be considered: it is visible, and has a natural representation in the color spaces we include in color. The other special color is white, though the physical color it represents depends on the color space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a previous PR with discussion about it: #31 and that links to more discussion in Xilem.

We'll also have to decide whether to solve it here in the Brush::default() only or also in color or both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, with linebender/color#63 in place, this would be shorter …

@waywardmonkeys
Copy link
Contributor Author

I think I will remove the legacy code instead, and export a type alias Color that is the same as AlphaColor<Srgb> as that would simplify updating Vello.

This leaves `peniko::Color` for now, but begins the migration to
using the new `color` crate.
@tomcur
Copy link
Member

tomcur commented Nov 25, 2024

(Just a note: with color becoming 32-bit per channel, maybe peniko::Image::alpha should become f32.)

peniko/src/image.rs

Lines 37 to 50 in d825bdf

pub struct Image {
/// Blob containing the image data.
pub data: Blob<u8>,
/// Pixel format of the image.
pub format: Format,
/// Width of the image.
pub width: u32,
/// Height of the image.
pub height: u32,
/// Extend mode.
pub extend: Extend,
/// An additional alpha multiplier to use with the image.
pub alpha: u8,
}

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.

2 participants