Skip to content

SKShaper: Support SKPaint.TextAlign property - #1910

Merged
mattleibow merged 10 commits into
mono:mainfrom
koolkdev:skshaper_align
Jun 3, 2022
Merged

SKShaper: Support SKPaint.TextAlign property#1910
mattleibow merged 10 commits into
mono:mainfrom
koolkdev:skshaper_align

Conversation

@koolkdev

Copy link
Copy Markdown
Contributor

Description of Change

Added handling of the SKPaint.TextAlign property in SKShaper.Shape.
The width is calculated by the sum of XAdvance

Befpre;
DrawText with TextAlign = Center:

DrawShapedText with TextAlign = Center

After this PR:
DrawShapedText with TextAlign = Center

Bugs Fixed

Behavioral Changes

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Updated documentation


// adjust alignment
if (paint.TextAlign != SKTextAlign.Left) {
var width = pos.Sum(p => p.XAdvance) * textSizeX;

@Gillibald Gillibald Dec 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might be a huge performance hit. I think text alignment should be evaluated when the text blob is being drawn onto a canvas. Better implement this in the DrawShapedText extension method. SKTextBlob.Bounds should give you the needed Size for your calculations.

@koolkdev koolkdev Dec 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for your suggestion. I tried to implement it, but Bounds return much larger box than there is actually rendered.

For example:
SKTextBlob.Create("test", font).Bounds.Width
returns 157.3125
while
paint.MeasureText("test")
returns 67.125

According to the documentation of bounds:

    /** Returns conservative bounding box. Uses SkPaint associated with each glyph to
        determine glyph bounds, and unions all bounds. Returned bounds may be
        larger than the bounds of all glyphs in runs.

        @return  conservative bounding box
    */

When diving deeper into the code of SkTextBlobBuilder, there are two functions to calculate the bounds, one is called TightRunBounds and the other ConservativeRunBounds, where ConservativeRunBounds is used for sk_textblob_builder_alloc_run_pos and sk_textblob_builder_alloc_run_pos_h, and TightRunBounds is only used for sk_textblob_builder_alloc_run (and there is a FIXME comment to not use tight run in this case too).
So it seems that bounds is pretty useless for this use case.

Another option is to use paint.MeasureText but it would not be accurate with things like Arabic text (which is the point of SKShaper)

Another option to that I can think of is to return it as part of the Result of SKShaper.Shape (last xOffset minus the starting xOffset) and than use it in DrawShapedText. What do you think?
Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DrawPositionedText should use

public SKPositionedRunBuffer AllocatePositionedRun (SKPaint font, int count, int textByteCount, SKRect? bounds)
if that doesn't work there is something wrong with the binding

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, I think bounds is misused here and should be an out parameter. Only when a nullptr is used Skia calculates the bounds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems that bounds is an input parameter, according to documentation:

        bounds defines an optional bounding box, used to suppress drawing when SkTextBlob
        bounds does not intersect SkSurface bounds. If bounds is nullptr, SkTextBlob bounds
        is computed from RunBuffer::pos, and RunBuffer::glyphs metrics.

Skia calculate the bounds anyway, but that parameter is just the initial value which it does SkRect.join to
https://github.com/mono/skia/blob/916a1e9721de74a05c75391f52393972ed7f736c/src/core/SkTextBlob.cpp#L514
https://github.com/mono/skia/blob/916a1e9721de74a05c75391f52393972ed7f736c/src/core/SkTextBlob.cpp#L383
At least if I am understanding the code correctly....

Anyway, the issue is the that Skia uses conservative way to calculate the bounds (probably for performance) which result in a much larger bounds than the actual bounds. (like twice as big or more in some cases), so it doesn't really help in this case.

What do you say about the current solution?

@mattleibow mattleibow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this PR. It has been so long since I reviewed a PR on this repo 😢 Thanks for this, but I just got 1 suggestion about API/ABI compatibility.

Comment thread source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz.Shared/SKShaper.cs
@mattleibow mattleibow added this to the v2.88.1 milestone May 22, 2022
@mattleibow

Copy link
Copy Markdown
Contributor

/azp run SkiaSharp

@azure-pipelines

Copy link
Copy Markdown
No pipelines are associated with this pull request.

@mattleibow mattleibow closed this May 30, 2022
@mattleibow mattleibow reopened this May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants