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

Ensure GlyphRunImpl has consistent bounds #12765

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

MrJul
Copy link
Member

@MrJul MrJul commented Sep 2, 2023

What does the pull request do?

This PR makes sure that the bounds of a GlyphRunImpl are always computed with the same edging.

What is the current behavior?

Currently, the font's edging might be changed between two GlyphRunImpl creations, resulting in different bounds with the same parameters with some specific fonts and sizes.

What is the updated/expected behavior with this PR?

The same parameters result in the same GlyphRun bounds. A test has been added.

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

Avoiding SKFont sharing

The SKFont property has been removed from GlyphTypefaceImpl to avoid mutating the same font instance depending on usages. The SKFont was initially created with default parameters, but those parameters were changed by various methods (GlyphRunImpl.GetTextBlob(), PlatformRenderInterface.BuildGlyphRunGeometry()).

Fixing those methods to restore the old font property values isn't sufficient: the SKFont is currently implicitly used (through the GlyphRun) by both the UI and the render thread, and it's theoretically possible to have some text being rendered with a given size, while the font is mutated by the UI thread to another size: it's not thread safe.

Since the SkFont itself is pretty lightweight (it's basically a typeface pointer + some primitive fields), it's created when needed instead. Performance wise, It's two very short-lived (Gen0) allocations per GlyphRun, which is basically noise in text processing.

Using a default edging

GlyphRunImpl now always use a default font edging of SubpixelAntialias while computing the bounds. Ideally the requested edging should be passed to the glyph run, but we can't since the actual edging is computed dynamically by the drawing context from the RenderOptions. The bounds are needed earlier, in the UI thread, for measuring purposes.

This can result in a 1px difference between the computed bounds and the rendered text in some cases. While the solution isn't perfect, it's still better than using the latest globally used edging: at least it's now consistent.

Fixed issues

Future

In the long term, I think the UI layer should be responsible for computing the actual render options so they can be used in the layout pass, and passed directly to the drawing context implementation. A possible implementation would be an inherited property (as a bonus, it would allow render options to be settable in styles).

It would also avoid repeating the same render options logic in every graphics platform (not that we have that many).

Pinging @Gillibald the text master :)

Copy link
Contributor

@Gillibald Gillibald left a comment

Choose a reason for hiding this comment

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

I didn't realize the edging mode could affect the resulting bounds. Good catch. It should be fine to to use a specific edging mode to calculate the bounds. Ideally bounds would be independent of the edging mode.

@MrJul MrJul force-pushed the fix/glyphrunimpl-edging-bounds branch from e2c84fa to 0df3023 Compare September 4, 2023 10:00
@Gillibald Gillibald enabled auto-merge September 5, 2023 06:55
@Gillibald Gillibald added this pull request to the merge queue Sep 6, 2023
@avaloniaui-team
Copy link
Contributor

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

Merged via the queue into AvaloniaUI:master with commit b9c8e71 Sep 6, 2023
HermanKirshin pushed a commit to JetBrains/Avalonia that referenced this pull request Sep 7, 2023
* Add GlyphRun InkBounds failing test

* Ensure GlyphRunImpl has consistent bounds

(cherry picked from commit b9c8e71)
@grokys grokys mentioned this pull request Sep 8, 2023
@MrJul MrJul deleted the fix/glyphrunimpl-edging-bounds branch September 15, 2023 10:20
@MrJul MrJul added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Sep 30, 2023
grokys pushed a commit that referenced this pull request Oct 2, 2023
* Add GlyphRun InkBounds failing test

* Ensure GlyphRunImpl has consistent bounds
@grokys grokys added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird 1 pixel layout bug when a control is loaded. Repro and video included.
4 participants