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

WIP [COLR] Add PaintLocation #4384

Closed
wants to merge 1 commit into from
Closed

WIP [COLR] Add PaintLocation #4384

wants to merge 1 commit into from

Conversation

behdad
Copy link
Member

@behdad behdad commented Aug 22, 2023

Doesn't work...

@behdad behdad marked this pull request as draft August 22, 2023 23:00
@behdad
Copy link
Member Author

behdad commented Aug 22, 2023

This is near impossible to make work. Definitely not with our current abstractions and APIs.

The problem is that PaintLocation cannot just set new coordinates on hb_font_t, for two reasons:

  • It would break the threadsafety of hb_font_t, which is a non-starter.
  • Even if we install new coordinates on hb_font_t, then hb_font_t attachments (a cairo scaled font, etc) won't notice that the hb_font_t changed underneath them.

Another approach would be to create a new hb_font_t and install the locations on it. But in general we don't have a hb_font_create_similar(). The hb_font_create_sub_font() is not sufficient, since it won't have proper font-funcs installed on it.

So to do this, we would need to add a hb_font_draw_at_location() which would be very ugly.

Yet another approach, would be to add push_location / pop_location to the hb-paint API, but that would have its own problems I believe.

@behdad behdad closed this Aug 22, 2023
@behdad
Copy link
Member Author

behdad commented Aug 22, 2023

FontTools counterpart. fonttools/fonttools#3262

@behdad
Copy link
Member Author

behdad commented Aug 23, 2023

FontTools counterpart. fonttools/fonttools#3262

The patch in this PR doesn't quite match the fonttools code. In particular AxisList has a Format field in the fonttools impl, that this patch doesn't account for. But as I said in the previous comment, it's near impossible to make it work currently anyway. So I'm abandoning it.

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

1 participant