Add support for reusable ShapePlans. #88
Conversation
| buffer.script.unwrap_or(script::UNKNOWN), | ||
| plan.script.unwrap_or(script::UNKNOWN) |
There was a problem hiding this comment.
The need for .unwrap_or(script::UNKNOWN) here is due to the fact that the UnicodeBuffer::script() function returns a Script (instead of an Option<Script>), and performs the same unwrapping logic.
The shape plan caching I implemented in cosmic-text to test this invoked UnicodeBuffer::script() to get the script value to pass to the ShapePlan constructor, so the assertion would fail if buffer.0.script is None, because the ShapePlan would have been constructed with Some(script::UNKNOWN).
src/plan.rs
Outdated
| @@ -67,6 +73,7 @@ pub struct ShapePlanner<'a> { | |||
| pub script_zero_marks: bool, | |||
| pub script_fallback_mark_positioning: bool, | |||
| pub shaper: &'static ComplexShaper, | |||
| pub user_features: Vec<Feature>, | |||
There was a problem hiding this comment.
Why do we have to allocate it again?
There was a problem hiding this comment.
I think we can pass user_features directly to compile.
There was a problem hiding this comment.
Both collect_features() and compile() require the list of features. We could change the signature for compile() to also take the list of features, though that opens the (unlikely) possibility that different lists of features are passed.
That said, this doesn't allocate again - constructing an empty Vec performs no heap allocations, so there's no allocation in new(). We only allocate once, in collect_features(), when we call user_features.to_vec().
Given the fact that there isn't actually any performance cost here, and the fact that collect_features() is a more natural place to set self.user_features, this seems better to me.
There was a problem hiding this comment.
But compile is a private method. It cannot be misused.
As for allocation, yes, my bad. There is none. But the code is still confusing. It confused me. I would prefer to avoid storing user_features in the ShapePlanner.
There was a problem hiding this comment.
Sure, done. I'm used to working on codebases with many more contributors (and PR approvers), where there's more value in protecting even private functions from misuse within the codebase. :)
|
|
||
| /// Shapes the buffer content using provided font and features. | ||
| /// | ||
| /// Consumes the buffer. You can then run `GlyphBuffer::clear` to get the `UnicodeBuffer` back | ||
| /// without allocating a new one. | ||
| pub fn shape(face: &Face, features: &[Feature], buffer: UnicodeBuffer) -> GlyphBuffer { | ||
| pub fn shape(face: &Face, features: &[Feature], mut buffer: UnicodeBuffer) -> GlyphBuffer { |
There was a problem hiding this comment.
I would add something along the lines to the doc here:
If you plan to shape multiple strings using the same [`Face`] prefer [`shape_with_plan`].
This is because [`ShapePlan`] initialization is pretty slow and should preferably be called once for each [`Face`].
|
Thanks! I will publish a new version soon. |
This exposes
ShapePlanoutside of therustybuzzcrate, and provides a newshape_with_planfunction that takes in aShapePlaninstead of a list of user features.Callers are expected to properly cache
ShapePlans to avoid incorrect rendering due to mismatches between theShapePlanand the provided buffer. To help avoid bugs due to mismatches, I added twodebug_assert!()lines to verify that the plan and buffer use the same direction and script.Given that
ShapePlanis now public outside of the crate, I changed a number ofpubspecifiers topub(crate)to maintain the same effective visibility.