Skip to content

Variable shaper - #41

Open
simoncozens wants to merge 7 commits into
linebender:masterfrom
simoncozens:variable-shaper
Open

Variable shaper#41
simoncozens wants to merge 7 commits into
linebender:masterfrom
simoncozens:variable-shaper

Conversation

@simoncozens

Copy link
Copy Markdown

So a lot of the changes here are just rustfmt doing its thing, but this PR also:

  • Adds a location field to the FontRef struct, to allow for shaping variable fonts.
  • Passes that location to Harfbuzz when glyphs are being shaped.
  • Makes the direction/script/language optional in layout_fragment, allowing the user to set explicit values if they want them but using Harfbuzz's guess_segment_properties for a decent default fallback.
  • Publicizes various structs so that they can be used directly by higher-level layout engines.

@jdm jdm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! These changes look useful.

Comment thread src/hb_layout.rs
if !font.location.is_empty() {
hb_font_set_variations(
hb_font,
get_variation_data(font).as_ptr(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Even though this is technically safe according to my experiments on the rust playground, I get very nervous passing a pointer to a temporary value as an argument here. Can we add a let variations = get_variation_data(font); before calling hb_font_set_variations to make the lifetime more clear?

@khaledhosny

Copy link
Copy Markdown
  • Makes the direction/script/language optional in layout_fragment, allowing the user to set explicit values if they want them but using Harfbuzz's guess_segment_properties for a decent default fallback.

This is really bad and should not be happen in production code. hb_buffer_guess_segment_properties() can easily be wrong and the user will be none the wiser. For example it guesses the direction from the script, so an sequence of digits from an RTL script will be set to RTL even though it should almost always be LTR.

@khaledhosny

Copy link
Copy Markdown

(I wish HarfBuzz never made hb_buffer_guess_segment_properties() public and kept it only for the command line test utilities).

Comment thread src/hb_layout.rs
Comment on lines +132 to +134
direction: Option<Direction>,
script: Option<hb_script_t>,
language: Option<String>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sounds like these should all be non-Optional.

Comment thread src/hb_layout.rs
b.set_direction(Direction::LTR);
b.set_script(script);
b.set_language(Language::from_string("en_US"));
b.guess_segment_properties();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's remove this guessing.

@simoncozens

Copy link
Copy Markdown
Author

Thanks, I'll work on these. I think an architectural question to resolve is whether we see the variation location as an attribute of the FontRef or of the TextStyle. I can see arguments for each. I put them in the FontRef so that they would be available to access when you have a Vec<LayoutFragment> (i.e. when you're breaking the text into lines after shaping). But there's also an argument that a LayoutFragment should know its TextStyle, and if we add that property, it would be much neater to put the variation location into TextStyle too.

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.

3 participants