Add reuseable context to new expand_* functions - #582
Conversation
| /// Take the output path by value, leaving an empty `BezPath` in its place. | ||
| #[inline(always)] | ||
| pub fn take_output(&mut self) -> BezPath { | ||
| core::mem::take(&mut self.output) | ||
| } |
There was a problem hiding this comment.
I dropped this. Consumers can simply replace_output(BezPath::default()) for the same behaviour. But happy to add back if requested 🙏
There was a problem hiding this comment.
I'd be in favour of adding this method. Seems like a common case, and there's little cost to having it.
(importance level: low)
raphlinus
left a comment
There was a problem hiding this comment.
I think this is a good idea and am ok with the PR landing as-is. One question I have is whether we need the API surface duplication and just have everything on ExpandCtx, get rid of the free functions.
|
|
||
| /// The same as [`expand_path`], but using a caller-provided [`ExpandCtx`] to enable | ||
| /// allocation re-use. | ||
| pub fn expand_path_with( |
There was a problem hiding this comment.
It feels appropriate to have this (and the signed version) as a &mut self method on ExpandCtx, as opposed to taking a context argument.
| self.tolerance = tolerance; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Thinking about the ergonomics in the simple case, I'm wondering if it might be nice to have an into BezPath (ie From<ExpandCtx> for BezPath). Then the simple case looks like:
let mut ctx = ExpandCtx::default();
ctx.expand(...);
let bezpath: BezPath = ctx.into();Not quite as short as the free function, but I'm thinking this could be a reminder that there's an allocation and a nudge to maybe get rid of it.
There was a problem hiding this comment.
I don't really like using Into for this. I feel like if we're going to do this it ought to just be a regular method (fn into_bez_path(self) or similar). A bonus of this is that you won't run into type inference issues.
There was a problem hiding this comment.
Fair enough, the type inference concern is legit.
waywardmonkeys
left a comment
There was a problem hiding this comment.
I know it is obvious to us, but we should be a bit more clear in the API docs. They should explicitly say _with functions return output through ExpandCtx::output(), that the output is cleared/reused on the next call, and that callers must copy/consume it before reusing the context.
While reviewing linebender/vello#1628 (comment), I noticed that the new expand methods allocate a new
BezPathper call.The proposed variant (exposing a
ExpandCtx) parallels the existingStrokeCtx.kurbo/kurbo/src/stroke.rs
Lines 200 to 220 in 6836244
Further, the proposed
ExpandCtxprovides the ability to better integrate with aBezPathfree list. For example, by providing methods intoExpandCtxthat allow a caller to "take" ownership of the underlyingBezPathwe can more easily move that allocation into a glyph cache (without cloning).Happy to change any and all naming - not attached to anything here.
cc @jrmoulton