vello_cpu: Add ImageResolver for resolving opaque IDs at rasterization time#1451
vello_cpu: Add ImageResolver for resolving opaque IDs at rasterization time#1451
ImageResolver for resolving opaque IDs at rasterization time#1451Conversation
grebmeg
force-pushed
the
gemberg/glyph-cache-image-resolver
branch
from
0c1cefb to
c4775c9
Compare
grebmeg
force-pushed
the
gemberg/glyph-cache-image-resolver
branch
from
c4775c9 to
b10f8b3
Compare
|
What's the story for the This seems like it is sitting on the edge of the discussions about resource management. (That said, this looks like a decent direction for |
| // Note: Multi-threaded dispatcher does not support ImageSource::OpaqueId. | ||
| // Images with OpaqueId will panic at rasterization time. | ||
| let noop_resolver = NoOpImageResolver; |
There was a problem hiding this comment.
If we use the NoOpImageResolver anyway, why does it panic? Or does it already panic in its current state (i.e. before this PR)?
| /// Image registry for resolving `ImageSource::OpaqueId` to pixmap data. | ||
| /// | ||
| /// This allows decoupling render commands from image data, enabling | ||
| /// patterns like spritesheet rendering. | ||
| image_registry: HashMap<u32, Arc<Pixmap>>, | ||
| /// Counter for generating unique image IDs. | ||
| next_image_id: u32, |
There was a problem hiding this comment.
nit, but it might be worth encapsulating the two into a custom struct?
| /// If the paint is an image with `ImageSource::OpaqueId`, it will be | ||
| /// resolved to the corresponding pixmap at rasterization time. | ||
| /// Make sure to register images with [`register_image`](Self::register_image) first. |
There was a problem hiding this comment.
This needs a warning that this doesn't work with multi-threading enabled.
| /// Resolve an `ImageId` to its pixmap data. | ||
| /// | ||
| /// Returns `None` if the image ID is not found in the registry. | ||
| fn resolve(&self, id: ImageId) -> Option<Arc<Pixmap>>; |
There was a problem hiding this comment.
Is my intuition correct that, at least in vello_cpu, this is called once at rasterization time and then cached, instead of being called individually in each wide tile? If so, might be worth documenting this (i.e. that this will be called once at rasterization time).
| } | ||
|
|
||
| /// Update an existing image in the registry with new pixmap data. | ||
| pub fn update_image(&mut self, id: ImageId, pixmap: Arc<Pixmap>) { |
There was a problem hiding this comment.
What's the use case for this? Wouldn't it be better to just create a new image?
| /// Saved state for recording operations. | ||
| #[derive(Debug)] | ||
| struct RenderState { | ||
| pub struct RenderState { |
There was a problem hiding this comment.
Why does this need to be public now?
| pub fn take_current_state(&mut self) -> RenderState { | ||
| RenderState { | ||
| paint: self.paint.clone(), | ||
| paint_transform: self.paint_transform, | ||
| transform: self.transform, | ||
| fill_rule: self.fill_rule, | ||
| stroke: core::mem::take(&mut self.stroke), | ||
| } | ||
| } | ||
|
|
||
| /// Restore the saved rendering state. | ||
| pub fn restore_state(&mut self, state: RenderState) { | ||
| self.transform = state.transform; | ||
| self.fill_rule = state.fill_rule; | ||
| self.stroke = state.stroke; | ||
| self.paint = state.paint; | ||
| self.paint_transform = state.paint_transform; | ||
| } |
There was a problem hiding this comment.
Same here, do those need to be public? If I'm not mistaken they are only used inside vello_cpu.
3 participants
This PR introduces
ImageResolvertrait and an image registry onRenderContext, enabling deferred resolution ofImageSource::OpaqueIdimages duringvello_cpurasterization. This decouples render command encoding from image data ownership, supporting patterns like dynamic sprite atlases and glyph caches where the backing pixmap may be updated between encoding and rendering.