Skip to content

Conversation

@DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Nov 21, 2025

This is an initial implementation of Vello API, our abstraction between Vello CPU and Vello Hybrid (and also maybe Vello Classic).

A few notes:

  • This is designed to be combined with something like a "Parley Draw", i.e. it's designed to allow external glyph rendering.
  • The design is fully no_std + alloc compatible.
  • Vello API does not try to abstract away the unabstractable, e.g. handling external textures.
  • The multi-threading design is something which will need more thought. I think we're likely to end up with an explicit CommandEncoder like API, similarly to that seen in wgpu.

Importantly, this PR is intentionally not making internal changes to the extant crates, and instead is only adding new API surface for the abstraction. The plan would be to make these changes only once this has landed, to limit merge conflicts as much as possible.

I think it's valuable to get something landed here, whether or not it ends up being the final state. The main advantage there is that it allows porting tests in a piecemeal fashion, as well as making changes to the APIs of Vellos CPU and Hybrid (without creating a massive merge conflict headache). This PR is currently scoped small enough to be easily revertible if we want to go a different direction.

A previous version of this PR used the renderer abstraction for the tests. That code is now gone as it wasn't landable, but can be seen in b200d5e and earlier.

@DJMcNab DJMcNab force-pushed the api-design branch 2 times, most recently from b200d5e to a48f6c5 Compare November 26, 2025 18:45
@DJMcNab DJMcNab changed the title Implement an initial version of Vello API, and implement most of the sparse strip tests using it Implement an initial version of Vello API Nov 26, 2025
Comment on lines +11 to +49
// TODO: Maybe a better type is `atomicow::CowArc<'static, str>`
pub label: Option<&'static str>,
Copy link
Contributor

Choose a reason for hiding this comment

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

https://lib.rs/crates/smol_str is another option here (Winit depends on it)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering how much effort it would be to coordinate a UI-ecosystem-wide change to one of the other mini-string crates. IIRC some of them have more features.

Maybe at next RustWeek.

@DJMcNab DJMcNab marked this pull request as ready for review December 19, 2025 14:59
@taj-p taj-p self-requested a review December 21, 2025 20:51
@grebmeg grebmeg self-requested a review December 22, 2025 02:47
Copy link
Collaborator

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

Overall, not sure how to think about this PR. It has a lot of TODO comments and hedges in places where we'll need to make judgment calls sooner or later.

Comment on lines 92 to 93
// This needing to be turbofished shows a real footgun with the proped "push_layer" API here.
None::<BezPath>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we'll solve this in the short-term, but this is definitely worth creating an issue about.

Comment on lines +101 to +102
// TODO: Change module and give a better name.
pub type OurBrush = peniko::Brush<peniko::ImageBrush<TextureHandle>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest "VelloBrush" for now.

Comment on lines 73 to 81
fn push_layer(
&mut self,
clip_transform: Affine,
clip_path: Option<impl Shape>,
blend_mode: Option<BlendMode>,
opacity: Option<f32>,
// mask: Option<Mask>,
// filter: Option<Filter>
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason why the API doesn't let you add transform layers?

Copy link
Member Author

@DJMcNab DJMcNab Dec 30, 2025

Choose a reason for hiding this comment

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

It isn't an existing feature in any of the Vellos {Classic, Hybrid, CPU}.

It also doesn't provide new functionality over the user manually keeping track of a transform externally, but it makes reasoning about a number of things more difficult (primarily drawing hinted glyphs, but also in the future how filter effects and masks will be applied).

Implementation wise, it's also a bit confused because it doesn't behave in the same way as any other layer type; i.e. it would be a layer type which doesn't implement a new isolated context, unless you set any of the other fields.

Comment on lines +170 to +190
self.commands
.extend(other_commands.iter().map(|command| match command {
RenderCommand::DrawPath(transform, path) => {
RenderCommand::DrawPath(correct_transform(*transform), correct_path(*path))
}
RenderCommand::PushLayer(command) => RenderCommand::PushLayer(PushLayerCommand {
clip_transform: correct_transform(command.clip_transform),
clip_path: command.clip_path.map(correct_path),
blend_mode: command.blend_mode,
opacity: command.opacity,
}),
RenderCommand::PopLayer => RenderCommand::PopLayer,
RenderCommand::SetPaint(affine, brush) => {
// Don't update the paint_transform, as it's already path local.
RenderCommand::SetPaint(*affine, brush.clone())
}
RenderCommand::BlurredRoundedRectPaint(brush) => {
// Don't update the paint_transform, as it's (currently) already path local.
RenderCommand::BlurredRoundedRectPaint(brush.clone())
}
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

For example, seems like this block of code could be replaced with a transform layer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that's true; this code is equivalent to the implementation of Scene::append in Vello Classic. That code would still need to exist in a version of this proposal with transform layers; the path references still need to be updated.

The only part of this code which would change is the two calls to correct_transform; instead those would be handled by the transform layer.

Comment on lines 53 to 54
// TODO: Format? Premultiplication? Hdr?
// TODO: Explicit atlasing?
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're not sure what fields you'll add, this struct should be #[non_exhaustive].

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you clarify how you expect this to work, if users need to create this struct?

This type mirrors the descriptors from wgpu, which aren't non_exhaustive.
Hopefully we get default_field_values soon enough, ideally with a nice non_exhaustive story.

Comment on lines 63 to 77
#[derive(Debug)]
pub struct Scene {
pub paths: PathSet,
pub commands: Vec<RenderCommand>,
// TODO: Make this an `Option`? That is, allow explicitly renderer-agnostic scenes
// which also don't allow bringing in textures?
pub renderer: Arc<dyn Renderer>,
pub hinted: bool,
/// Handles for each texture stored in this scene.
///
/// We store these to ensure that the textures won't be freed until this scene
/// is fully rendered (and dropped).
// TODO: HashSet? We'd need to bring in hashbrown, but that's probably fine
pub textures: Vec<TextureHandle>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Scene type needs way more documentation. As it is, I don't know how it's supposed to be used, how it fields relate to each other, what assumptions it makes that mustn't be broken, etc.

It's especially not clear to me how Scene and PathSet relate. Does each scene store its own set of paths? Doesn't that kind of defeat the point of path caching?

Copy link
Member Author

@DJMcNab DJMcNab Dec 30, 2025

Choose a reason for hiding this comment

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

It's especially not clear to me how Scene and PathSet relate. Does each scene store its own set of paths? Doesn't that kind of defeat the point of path caching?

Sure, this is definitely under-documented. I'm planning to spend the time to add some more docs as my near-term priority here.

Essentially, each Scene accumulates the paths which it's drawing. These can either be copied into any painters which the scene is used in, to be rasterised in a just-in-time manner, or moved into the renderer (i.e. cached).
This accumulation pattern is designed to avoid an allocation for each path, whilst still allowing reference-count based garbage collection for cached paths (they are reference counted as a group of "related" paths).

But whilst PathSet currently exists for that design, the caching is not yet implemented (as it doesn't actually exist in the underlying renderers, for one). But I feel it still makes sense as an abstraction boundary to have, albeit with better docs...

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