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

Migrate to using raw_resource_handle #68

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

Conversation

waywardmonkeys
Copy link
Collaborator

No description provided.

@waywardmonkeys
Copy link
Collaborator Author

Before this can land, I think 3 things need to happen, at least:

  • The crate should be published and we don't add another git dependency.
  • We decide whether or not that crate should add a serde feature or we remove the serde support from Image in this crate, which I guess would propagate outward and include removing it from Brush as well.
  • We think about adding a different type bound on the contstructor Image::new and maybe some additional From impls on Blob so that creating an image doesn't necessarily have to know about Blob (but can create one automatically).

This needs feedback from @dfrg at a minimum.

@waywardmonkeys waywardmonkeys force-pushed the migrate-to-raw_resource_handle branch from d53fef9 to c212f6e Compare November 29, 2024 14:01
@waywardmonkeys
Copy link
Collaborator Author

Some previous discussion of serde in #30.

@ratmice
Copy link
Contributor

ratmice commented Nov 29, 2024

As far as point 2, I've definitely been using serialize on peniko::Brush, none of my programs ever construct the Brush::Image variant though it is exposed from the library, selvage::PaintOp

That library code however mostly provides (the trait) for a thin layer around vello::Scene, and the Scene::Stroke and Fill, in vello, both take BrushRef and thus potentially images.

As we discussed on zulip though, I think I should be able to use serialize_with for that enum in selvage::PaintOp
should we remove it from peniko. I only recently got it working all together with all the crate versions coming into alignment, but it isn't the worst thing to go back to git versions, and [patch] sections if necessary too.

In that library though, my preference is definitely to keep things relatively similar to the Scene API but that does like in this case bring with it these variants of brush I'm not actually using.

@dfrg
Copy link
Contributor

dfrg commented Nov 30, 2024

My general thought on this is that we should carefully consider what raw_resource_handle should look like rather than just extracting the current design and adding a dependency immediately.

Just a few potential questions:

  1. Should we disconnect blob and identifier? I feel like shared unique ids are the most important concept. We may want to support resources that are not represented as raw bytes.
  2. Maybe separate the id space by type rather than having one shared by all resources?
  3. Should blob also support &'static [T] to handle embedded data?
  4. Should blob have an associated range to support suballocated resources (potentially from mmaped files?)

And probably others that I haven’t considered.

@waywardmonkeys
Copy link
Collaborator Author

@dfrg That's exactly the conversations that I want to kick off. There kept being requests to start down this path, but we needed to take the first step.

I don't really think this is something that will happen for the December set of releases. (I have very little time for it myself and my focus is elsewhere in the stack.)

@nicoburns
Copy link

Would it be crazy to make the identity aspect a trait instead of (or as well as) as type? Then something like Font could be usize smaller by packing the index and id into 64 bits...

@ratmice
Copy link
Contributor

ratmice commented Jan 3, 2025

We decide whether or not that crate should add a serde feature or we remove the serde support from Image in this crate, which I guess would propagate outward and include removing it from Brush as well.

@waywardmonkeys it occurred to me that since Serialize returns a Result we could just stop supporting serialization for the Image variant of Brush. It looks like we can just use the skip_serializing + skip_deserializing variant attributes from the serde_derive macros for that...

something like:

#[derive(Clone, Debug, PartialEq)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum Brush {
    /// Solid color brush.
    Solid(AlphaColor<Srgb>),
    /// Gradient brush.
    Gradient(Gradient),
    /// Image brush.
   #[serde(skip_serializing, skip_deserializing)]
    Image(Image),
}

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.

4 participants