Closed dhardy closed 3 years ago
Sounds good. I will add from_face
method.
Whoops, I forgot to mention in the readme that unlike hb, rb doesn't support font size. You should scale the produced offsets and advances manually.
Your GlyphBuffer reports codepoint and cluster.
Same as harfbuzz. There are no changes here.
Or does the optimiser avoid this issue anyway?
UnicodeBuffer
and GlyphBuffer
are just safe wrappers around internal Buffer
. There is not copying involved. You can store/cache any of them.
You should scale the produced offsets and advances manually.
So it's not necessary to set font size in the Face
object at all, if not using variation axes?
Same as harfbuzz.
Ach ja. Those names are confusing IMO. From KAS-text's HarfBuzz support:
let index = idx_offset + info.cluster;
assert!(info.codepoint <= u16::MAX as u32, "failed to map glyph id");
let id = GlyphId(info.codepoint as u16);
The Buffer
object is rather large. Put the below in godbolt.org (sorry, share feature isn't working!), and its clear there is a copy (compare different sizes of T
):
type T = [u32; 32];
pub struct A(T);
pub struct B(T);
pub fn f(a: A) -> B {
B(a.0)
}
So it's not necessary to set font size in the Face object at all, if not using variation axes?
Yes. And even with variation it's not necessary, because variation doesn't affect size, technically speaking.
Those names are confusing IMO.
Yes. codepoint
== glyph
after shaping.
The Buffer object is rather large.
It is, but it never copied, or rather cloned, but moved.
You know that CPUs don't have a concept of moves, right? This is a Rust concept (avoiding the need to destroy one object and construct another, especially so that String
and Vec
don't need to reallocate). When you move a String
, the 24 bytes of the String
object (capacity+length+pointer) are copied, though the allocated text buffer isn't.
Sure, and? This is how Rust works. If you what, you can box buffers, but it would not affect the performance in any way.
One would have to box the buffers inside GlyphBuffer
/ UnicodeBuffer
. But whatever, it's not that important.
Other:
Face::from_slice
returns an Option
; it would be preferable IMO if it would forward the ttf-parser
errorYes. This are some leftovers from the old version. But the are no interesting error in ttf-parser either, since it simply skips malformed tables.
Well, implementing this in KAS-text wasn't hard (mostly copy+paste from HarfBuzz code): https://github.com/RazrFalcon/rustybuzz/issues/23
Nice!
Sounds good. I will add
from_face
method.
For text renderers, it would be useful if rather than this method taking an owned ttf_parser::Face
, it took a reference to a ttf_parser::Face
. Otherwise, to do text rendering, you would still need to load the font twice because you wouldn't have access to ttf_parser::Face
after calling from_face
.
If that was already the plan, I think from
implies movement of ownership and would suggest naming the method with_face()
or simply new()
instead. An alternative solution would be an additional face()
method to get a reference to the ttf_parser::Face
out from the rustybuzz::Face
struct.
Awesome library, by the way!
@AldaronLau Yes, this is a common request for ttf-parser, there is even owned_ttf_parser. But the problem is that ttf-parser designed to work on a borrowed data. It doesn't do heap allocations at all.
On the other hand, ttf-parser::Face creation is basically free compared to the shaping process itself. So it shouldn't be a bottleneck.
@RazrFalcon Thanks for the clarification. I didn't know it was basically free to create one.
@AldaronLau Judging by ttf-parser benchmarks, a Face
creation is roughly the same as outlining of a single glyph.
It should be noted that in contrast to ttf-parser's Face
, rustybuzz's Face
is a bit heaver because the parsed GSUB
and GPOS
tables contain heap-allocated vectors. So it probably makes sense to parse these only once. I think it's reasonable to have both rustybuzz::Face::from_face(face: ttf_parser::Face) -> Self
and rustybuzz::Face::face(&self) -> &ttf_parser::Face
.
@laurmaedje Oh, I forgot about it. I thought it allocated only before the shaping itself and not on font creation.
In any case it sounds like it's not a problem to have both a ttf_parser::Face
and a rustybuzz::Face
.
It should be noted that in contrast to ttf-parser's Face, rustybuzz's Face is a bit heaver because the parsed GSUB and GPOS tables contain heap-allocated vectors. So it probably makes sense to parse these only once. I think it's reasonable to have both rustybuzz::Face::from_face(face: ttf_parser::Face) -> Self and rustybuzz::Face::face(&self) -> &ttf_parser::Face.
@laurmaedje Either way, the only Face
creation that's happening twice is in ttf-parser, not in rustybuzz. The API you suggest would get rid of the extra ttf-parser Face
creation, which @RazrFalcon has described as a cheap operation. I'm not sure if there are any downsides to this, but if you're already going to add both of those APIs are there any reasons why not do fn rustybuzz::Face::with_face(face: &'a ttf_parser::Face<'a>) -> rustybuzz::Face<'a>
instead?
@AldaronLau Yes, I guess from_slice
should be replace with with_face
or from_face
. There is not need for duplication.
Either way, the only Face creation that's happening twice is in ttf-parser
@AldaronLau That's true, but in any case I wanted to point out that rustybuzz's Face
is a bit heavier.
The with_face
design could maybe also work. The problem I see is that you couldn't borrow the ttf_parser Face
mutably anymore (which is needed for setting font variations and also currently used in rustybuzz::Face::set_variations
). In contrast, the from_face
design could also have a face_mut
accessor. Then we wouldn't need the set_variations
function anymore in rustybuzz because you can just configure the underlying ttf-parser Face
.
Any thoughts on implementing this through the traits From<ttf::Face>
, AsRef<ttf::Face>
, and AsMut<ttf::Face>
?
I haven't looked into this yet.
Hey @RazrFalcon, just looking over your API for use in KAS-text and I have a few comments.
I notice that
Face
is a wrapper aroundttf-parser::Face
, which KAS-text already has an instance of. I can supply the data pointer and face index no problem, but I could also directly supply a&'a Face
(or possibly even just store therustybuzz::Face
the embeddedttf-parser::Face
could still be accessed). But this isn't really important.More significant, KAS-text stores text-runs with a
FontId
(index in internal list) plus a size (DPU
aka pixels per font unit). I see yourFace
object embeds the size. For my purposes would storing a singleFace
and adjusting the size before each shaping run be the best option?Your
GlyphBuffer
reportscodepoint
andcluster
. KAS-text needs the text-index of each glyph (required for editing support), and trying to reconstruct this from codepoints is not ideal.I see you use the same
UnicodeBuffer ←→ GlyphBuffer
model as HarfBuzz. I'm not sure how best to cache this type since it is consumed by value in theshape
method. Maybe it would be better ifUnicodeBuffer
andGlyphBuffer
were wrappers aroundBox<Buffer>
to avoid large copies? Or does the optimiser avoid this issue anyway?