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

Lifetimes are extremely difficult (impossible?) to navigate #52

Open
LoganDark opened this issue Jan 8, 2022 · 10 comments
Open

Lifetimes are extremely difficult (impossible?) to navigate #52

LoganDark opened this issue Jan 8, 2022 · 10 comments

Comments

@LoganDark
Copy link
Contributor

Rasterization and such is very complex in this library, so I wanted to abstract parts of it (i.e. getting an OutlineBuilder) to make it easier. But that's not possible, because of how this library uses lifetimes.

Basic stuff like this does not work:

	pub fn from_bytes(bytes: &'a [u8]) -> Result<Self, FromBytesError> {
		let font_data = ReadScope::new(bytes.as_ref()).read::<FontData<'a>>()?;
		let provider = font_data.table_provider(0)?;
		let font = match allsorts::Font::new(provider)? {
			Some(font) => font,
			None => return Err(FromBytesError::NoFont)
		};

		let head_table = match font.head_table()? {
			Some(head_table) => head_table,
			None => return Err(FromBytesError::NoHeadTable)
		};

		Ok(Self { font_data, font, head_table })
	}

due to the ownership rules required by allsorts. For example, font (well actually provider) depends on font_data's lifetime instead of bytes.

(I managed to get that function working, I think, using boxes, but the below case I haven't been able to solve at all)

it only goes downhill from here. Let's say I wanted to outsource the process of finding an OutlineBuilder, to a function. I want to have that function do all the stupid parsing of tables for me so I can just focus on rasterizing the outlines

enum GlyphOutlineProvider<'a> {
	CFF(CFF<'a>),
	GLYF(GlyfTable<'a>)
}

#[derive(thiserror::Error, Eq, PartialEq, Debug)]
enum GlyphOutlineProviderOutlineBuilderError {
	#[error("{0:?}")]
	CFF(#[from] CFFError),

	#[error("{0:?}")]
	GLYF(#[from] ParseError)
}

impl<'a> OutlineBuilder for GlyphOutlineProvider<'a> {
	type Error = GlyphOutlineProviderOutlineBuilderError;

	fn visit<S: OutlineSink>(&mut self, glyph_index: u16, sink: &mut S) -> Result<(), Self::Error> {
		match self {
			Self::CFF(cff) => cff.visit(glyph_index, sink).map_err(Self::Error::CFF),
			Self::GLYF(glyf) => glyf.visit(glyph_index, sink).map_err(Self::Error::GLYF)
		}
	}
}

impl<'a> Font<'a> {
	fn glyph_outline_provider_for(font: &mut allsorts::Font<DynamicFontTableProvider>, head_table: &HeadTable) -> Result<GlyphOutlineProvider<'a>, ParseError> {
		if font.glyph_table_flags.contains(GlyphTableFlags::CFF) && font.font_table_provider.sfnt_version() == OTTO {
			let cff_data = font.font_table_provider.read_table_data(CFF)?;
			let cff = ReadScope::new(&*cff_data).read::<CFF<'_>>()?;

			Ok(GlyphOutlineProvider::CFF(cff))
		} else if font.glyph_table_flags.contains(GlyphTableFlags::GLYF) {
			let loca_data = font.font_table_provider.read_table_data(LOCA)?;

			let loca = ReadScope::new(&*loca_data).read_dep::<LocaTable<'_>>((
				font.maxp_table.num_glyphs as usize,
				head_table.index_to_loc_format
			))?;

			let glyf_data = font.font_table_provider.read_table_data(GLYF)?;
			let glyf = ReadScope::new(&*glyf_data).read_dep::<GlyfTable<'_>>(&loca)?;

			Ok(GlyphOutlineProvider::GLYF(glyf))
		} else {
			panic!("no CFF or GLYF tables in the font?");
		}
	}
}

Whoops, Rust complains. Looks like cff_data depends on the lifetime of font instead of the data that it points to. And cff then depends on cff_data instead of the data that font points to. You can probably see where I'm going with this. This feels like a mess and it's impossible for me to navigate.

I can't store the buffer as a box in GlyphOutlineProvider because that would break rust's ownership rules (the box's contents are immutably borrowed, but someone else would be able to mutate the box if it were returned, so rust complains that the box is borrowed when I try to put it into the enum). And that's the end of that, there's really no other way to do this. Storing it under the struct is a bust because that would borrow the struct indefinitely. Unsafe code is a bust because it's way too hard to come up with something that is sound and doesn't cause UB. So on so on...

I would really love for everything in this library to just use the same lifetime, the lifetime of the slice that Font loads its data from. Because right now, the situation is kind of unmanageable for me.

I talked with @wezm over Discord and they recommended I create this issue

@LoganDark
Copy link
Contributor Author

It appears that this is also a sort-of-solved problem with crates like ouroboros or moveit. However, it's not obvious that those crates exist or where to find them, so I didn't know they existed when writing this issue. :/

@wezm
Copy link
Contributor

wezm commented Jan 18, 2022

Yes they can be useful. We already use ouroboros in Allsorts for holding table data along with the parsed table, sorry I didn't mention it earlier.

pub struct CBLC {

@LoganDark
Copy link
Contributor Author

LoganDark commented Jan 18, 2022

Yes they can be useful. We already use ouroboros in Allsorts for holding table data along with the parsed table, sorry I didn't mention it earlier. https://github.com/yeslogic/prince/blob/35b30c568d2e32000013570b353f71e94ac64420/src/fonts/allsorts/src/font.rs#L99

Doesn't look like that repository is public. Don't worry, I get it :)

@wezm
Copy link
Contributor

wezm commented Jan 18, 2022

Oh sorry, it is public, I copied from the wrong repo. I've updated the link.

@LoganDark
Copy link
Contributor Author

I've updated the link.

In my comment xD. But it works!

@DemiMarie
Copy link

Is this a problem?

@wezm
Copy link
Contributor

wezm commented Jan 2, 2023

I can't say for sure whether it's a problem still—it will depend on your use-case. However, this change from last year may have made things better: 3d42e51

@DemiMarie
Copy link

I can't say for sure whether it's a problem still—it will depend on your use-case. However, this change from last year may have made things better: 3d42e51

What about for toolkits like Slint?

@wezm
Copy link
Contributor

wezm commented Jan 3, 2023

I expect you might run into other limitations even if you didn't have this particular problem. E.g. to implement a text field I think #31 would need to be implemented.

@fogzot
Copy link

fogzot commented Sep 5, 2023

I have the same problem with the references. I'd like to cache the Font instance and be able to pass to different functions the cache (a simple Hash<String,Box<CachedFont>> where CachedFont keeps track of the initial [u8] buffer and everything else needed up to Font) but I am fighting with the fact that every intermediate object (the reader, the table provider and so on) depends on the initial &[u8]. Is there any kind of "official" way to do this? Some example code maybe?

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

No branches or pull requests

4 participants