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

Make sure GlyphRunImpl.InkBounds are always at a positive location #16601

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

Gillibald
Copy link
Contributor

@Gillibald Gillibald commented Aug 5, 2024

What does the pull request do?

For an unknown reason, Skia reports glyph bounds at a negative x-position which causes some alignment issues for monospace fonts. This PR makes sure glyph bounds are always at a positive position.

What is the current behavior?

What is the updated/expected behavior with this PR?

Screenshot 2024-08-05 163407

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes: #16555

@Gillibald Gillibald added area-textprocessing backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Aug 5, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050990-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added the bug label Aug 5, 2024
@MrJul
Copy link
Member

MrJul commented Aug 5, 2024

Just a though, aren't the negative bounds returned because of the edging?

// Ideally the requested edging should be passed to the glyph run.
// Currently the edging is computed dynamically inside the drawing context, so we can't know it in advance.
// But the bounds depends on the edging: for now, always use SubpixelAntialias so we have consistent values.
// The resulting bounds may be shifted by 1px on some fonts:
// "F" text with Inter size 14 has a 0px left bound with SubpixelAntialias but 1px with Antialias.

This comment says that the result could be shifted by 1px, but it could also probably be -1px depending on the font.


Since that negative bound might apply to several other characters in the run and not only the first, won't shifting the bounds of the whole run potentially cause other problems, for example with hit testing?

Also, should we add a render test here to avoid future regressions?

@Gillibald
Copy link
Contributor Author

Y sounds like we need a render test

@Gillibald Gillibald changed the title Make sure GlyphRunImpl.InkBounds are always at a positive location [WIP] Make sure GlyphRunImpl.InkBounds are always at a positive location Aug 5, 2024
@Gillibald
Copy link
Contributor Author

@Gillibald
Copy link
Contributor Author

So ideally glyph bounds should never affect layout. So each glyphs anchor point should be adjusted to the expected glyph advances. In the end this should only matter for the first glyph's bounds because these define the left edge of a run.

@Gillibald Gillibald changed the title [WIP] Make sure GlyphRunImpl.InkBounds are always at a positive location Make sure GlyphRunImpl.InkBounds are always at a positive location Sep 12, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0051781-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 enabled auto-merge September 12, 2024 05:00
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0051805-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 disabled auto-merge September 12, 2024 23:10
@maxkatz6 maxkatz6 merged commit d72ed10 into AvaloniaUI:master Sep 12, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-textprocessing backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some monospace fonts are misaligned
4 participants