Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use font-kit to implement XeTeXFontInst #188

Closed
wants to merge 41 commits into from

Conversation

cormacrelf
Copy link
Collaborator

@cormacrelf cormacrelf commented Nov 14, 2019

#186 It's a work in progress. Notable semi-related things included:

  • The Read implementation was converting -1 (error) to u64 and telling
    read_to_end to start reading at the other end of the universe.
  • Many cleanups of the font manager/font info c2rust conversion.
  • fixes Merge xetex_layout_engine.rs into xetex_layout_interface.rs #142
  • FT_MAKE_TAG macro replaced
  • Some name splitting code refactored, to make find_native_font more manageable as font-kit will hopefully replace some of it.
  • Fixed some non-FFI-safe warnings that are new in recent nightly compilers I think?

Some things that need work:

  • The global name_of_file and other tex globals are way too involved in font loading. It's painfully unnecessary. set/getReqEngine also.
  • There's heavy mixing of the slash-code font loading options like "/AAT", with the rest of the font loading. I'm not quite sure how that works or how it could be disentangled. Also, font-kit will find a font by name, but if it has to search by name and by type... ?
  • PlatformFontRef has replaced XeTeXFont, which was just a type alias, but that has made it clear that not every PlatformFontRef is actually a PlatformFontRef -- many are cast willy-nilly to XeTeXFontInst and the platform-specific subclasses. This is terrible.
  • The C++ inheritance needs to go. I haven't come up with a neat way of doing it yet.
  • The harfbuzz interactions could be done with the user_data pointing to a font_kit Font. I'm not sure Font supports all the things harfbuzz is going to need, but you can always keep forcing font-kit to use the freetype loader and use Font.native_font() to access FT_Face.
  • I think putting things in harfbuzz user_data should use a Pin pointer?

Some notes:

  • loadAATFont and loadOTFont return completely different objects, each under a *mut libc::c_void pointer, but both pointers are stored in the font_layout_engine static mut. font_area 's data stored at the same offset is effectively an enum tag of layout engine types.
  • loadAATFont's "layout engine" is actually just a CFDictionaryRef of font attributes. Every thing the "layout engine" is required to do, xetex_ext calls into some aat:: function for, after discovering font_area is 0xffffu32. So you could think of it as a layout engine, since the resulting API looks similar to using functions (not methods) from xetex_layout_engine.
  • loadOTFont is poorly named. It doesn't load the font. It takes a font, matches some options, and essentially wraps createLayoutEngine. No other code uses createLayoutEngine.
  • loadOTFont returns a XeTeXLayoutEngine, which holds a font: XeTeXFontInst from createFont, and fontRef: PlatformFontRef, which is the font loaded with findFontByName.
  • I honestly have no idea why you don't see panics in measure_native_node, which doesn't handle AAT fonts but is called during, among other places, line_break.

Here's the big plan:

  • Make a trait for everything in xetex_ext that checks font_area/0xffffu32/0xfffeu32
  • impl LayoutEngine for a newtype struct AATLayoutEngine { attributes: CFDictionaryRef }
  • impl LayoutEngine for XeTeXLayoutEngine
  • Store Box<dyn LayoutEngine> in font_layout_engine, and delete font_area. (Hooray!)
  • See if you can use Skribo to make another impl LayoutEngine similar to XeTeXLayoutEngine
  • Improve the LayoutEngine API over time


pub unsafe fn get_glyph_bounds(&self, gid: GlyphID) -> GlyphBBox {
if let Some(font) = &self.fk_font {
if let Ok(rect) = font.typographic_bounds(gid as u32) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, this is the only place we're actually querying font information so far. Further down, font-kit is also used to load the FT_Face.

let mut varString = String::new();
let mut slice = &variant_string[..];
while !slice.is_empty() {
if slice.starts_with("AAT") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the slash-options intermingling I was talking about.

ttstub_input_read actually translated into read_exact, which means
that the provided methods of Read were getting messed up by the errors
being returned.

So it's been renamed to what it actually means: ttstub_input_read_exact,
and supplemented with ttstub_input_read which does the normal Read::read.
Importantly, Read::read is implemented with the latter.
e.g. [lmroman12-regular], parsed into something? Not sure. Kept that code.
let mut sl = CStringListBuilder::new();
sl.push_non_null_terminated(&b"ot"[..]);
...
let sl = sl.freeze();

hb_shape_plan_create_cached(..., sl.as_ptr());
Note especially the very clean API for layout_text.
@cormacrelf
Copy link
Collaborator Author

Update:

  • Swapped out the void*-and-font_area-based dynamic typing for enums, using enum_dispatch to allow dropping down to non-trait methods.
  • Had an annoying time initializing a static mut hashmap, so it's in a lazy RefCell, which is suboptimal. Rust really doesn't make this bad pattern easy.
  • There's a bad measurement going on somewhere.

@burrbull
Copy link
Collaborator

Maybe if you split this PR on several and rebase it will be much easier to understand what's going on here.

@burrbull
Copy link
Collaborator

burrbull commented Nov 7, 2020

@cormacrelf
Could you make another try?

burrbull added a commit that referenced this pull request Feb 7, 2021
retry #188 (part 1 Enum Dispatch)
burrbull added a commit that referenced this pull request Feb 8, 2021
@burrbull
Copy link
Collaborator

burrbull commented Feb 8, 2021

Closing this as all main changes are already in oxidize

@burrbull burrbull closed this Feb 8, 2021
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.

Merge xetex_layout_engine.rs into xetex_layout_interface.rs
2 participants