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

[style] New API, hb_style_get_value #1890

Merged
merged 4 commits into from
Jun 4, 2020
Merged

Conversation

ebraminio
Copy link
Collaborator

@ebraminio ebraminio commented Aug 4, 2019

Searches variation axes of a hb_font_t object for a specific axis first,
if not set, then tries to get default style values from different
tables of the font.

Related patches, master...ebraminio:style master...ebraminio:style2 master...ebraminio:italicangle #1468

@behdad
Copy link
Member

behdad commented Aug 4, 2019

Haven't looked at what you've done, but related:
#1888 (comment)

@behdad
Copy link
Member

behdad commented Aug 4, 2019

At first glance I like this, but see my comment linked above. This needs to hook up to font's currently-set variation values as well. And of course, what you have is the fallback path. The (missing) main path should be reading STAT table. And the API to expose that table is a lot more involved. So we shouldn't rush this, but keep evolving it till it's complete.

@ebraminio
Copy link
Collaborator Author

ebraminio commented Aug 4, 2019

This needs to hook up to font's currently-set variation values as well.

The values here don't scale and are like hb_ot_var_find_axis_info.default_value which works with hb_face_t. Say in a font management system we want to know about different aspects of a face, why one should create a hb_font_t just to learn about a face.

And of course, what you have is the fallback path. The (missing) main path should be reading STAT table.

Was working on it as wanted to have it in separate change, done

And the API to expose that table is a lot more involved.

I'd say lets do that later, fallback won't be needed there even, mixing that with this just complicates things for a simple font crawling application.

@ebraminio
Copy link
Collaborator Author

ebraminio commented Aug 4, 2019

If someday we decide to expose one of these flags, OS2.fselection.{UNDERSCORE,OUTLINED,STRIKEOUT} or post.isFixedPitch or head.macStyle.{UNDERLINE,OUTLINE,SHADOW} (which we don't, just theoretically), for example outlined flag which we can check both OS2 and head for better compatibility, don't we go for a naming like hb_bool_t hb_ot_style_is_outlined (hb_face_t *face), I think the API here is also similar to that, working upon a face, giving best effort result.

/* XXX: It should be 900! */
g_assert_cmpfloat_hb (hb_ot_style_get_value (font, HB_OT_STYLE_TAG_WEIGHT), 1);\
/* XXX: It should be 50! */
g_assert_cmpfloat_hb (hb_ot_style_get_value (font, (hb_ot_style_tag_t) HB_TAG ('C','N','T','R')), 0.5);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see why but not sure if we like to do that :/

@@ -128,11 +162,24 @@ struct avar
const SegmentMaps *map = &firstAxisSegmentMaps;
for (unsigned int i = 0; i < count; i++)
{
coords[i] = map->map (coords[i]);
// coords[i] = map->map (coords[i]);
Copy link
Collaborator Author

@ebraminio ebraminio Aug 5, 2019

Choose a reason for hiding this comment

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

guess can't reversed easily, test-ot-style test is working without this so if I could map unmap work I could reversed it correctly.

#define g_assert_cmpfloat_with_epsilon(n1, n2, epsilon) g_assert_cmpfloat ((int) roundf (n1), ==, (int) roundf (n2))
#endif

#define EPSILON 0.01
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 its precision on retrieving original coords which I believe is pretty good.

@ebraminio
Copy link
Collaborator Author

Dropped _ot_ as most of the tags can be considered for non OT fonts also.

How about renaming hb_style_get_value to hb_font_get_style_value? Guess hb_style_tag_t naming should be remained, or, going for hb_font_style_tag_t also?

@ebraminio ebraminio changed the title [style] New API, hb_style_get_value [style] New API, hb_font_get_style_value Feb 8, 2020
@ebraminio
Copy link
Collaborator Author

ebraminio commented Feb 12, 2020

@behdad can you review this also? Specially on hb_style_get_value vs hb_font_get_style_value and hb_style_t part. Thanks!

@behdad
Copy link
Member

behdad commented Feb 13, 2020

@behdad can you review this also? Specially on hb_style_get_value vs hb_font_get_style_value and hb_style_t part. Thanks!

Not now. I'm very conflicted on this still :). Let me recover a bit and be more comfortable with other code that has gone in first. Like to come to a conclusion on the pen stuff first. Thanks.

@ebraminio ebraminio changed the title [style] New API, hb_font_get_style_value [style] New API, hb_style_get_value Mar 13, 2020
@ebraminio ebraminio force-pushed the stat branch 2 times, most recently from d9e5401 to ccb8e20 Compare June 4, 2020 11:31
Searches variation axes of a hb_font_t object for a specific axis first,
if not set, then tries to get default style values from different
tables of the font.
ptem, used for AAT's tracking/`trak` table is equivalent to opsz of variable fonts.

For variable AAT fonts, such as SFNS, ideally variable axis of the hb_font_t
should be set and equivalent to ptem, https://crbug.com/1005969#c37
@ebraminio
Copy link
Collaborator Author

ebraminio commented Jun 4, 2020

as previous off tracker agreement we had on having this as an experimental API, just like to experiment with the API on other projects to show its usefulness and guess there will be some clients for this just like the draw API clients found on github so they can comment on usefulness and correctness of the API also so let's have this as an experimental API for now.

@ebraminio ebraminio merged commit 759df46 into harfbuzz:master Jun 4, 2020
@ebraminio ebraminio deleted the stat branch June 4, 2020 16:03
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.

None yet

2 participants