-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Impeller] Use new SkParagraph APIs for stroked text. #41735
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
I see a revert just got queued up for the Skia change so I've moved this back to WIP |
🙃 |
Hey it relanded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the bounds offset, but overall LGTM!
@@ -5,6 +5,7 @@ | |||
#pragma once | |||
|
|||
#include "flutter/fml/macros.h" | |||
#include "impeller/geometry/path.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove unused include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return; | ||
} | ||
canvas_.Save(); | ||
canvas_.Translate({x + bounds->origin.x, y + bounds->origin.y, 0.0}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the frame bounds origin actually the right thing to be adding here? It would seem weird for Skia's API to circularly depend on the text frame bounds origin when coming up with path coords to output. Needing to add in something like the baseline offset would make sense, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I don't really know. This is similar to the approach I used when drawing with different color sources on top of the text: https://github.com/flutter/engine/blob/main/impeller/aiks/canvas.cc#L529-L534
return {}; | ||
} | ||
|
||
return ToPath(skia::textlayout::Paragraph::GetPath(blob.get())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I'll be damned. That's a lot easier than I was expecting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I had to wait 3 months for the API to be added so IMO not that easy 😆
@@ -1104,9 +1104,23 @@ void DlDispatcher::drawDisplayList( | |||
void DlDispatcher::drawTextBlob(const sk_sp<SkTextBlob> blob, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's strictly necessary, but adding a playground test in display_list_unittests.cc
would be nice as a jumping off point for debugging in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…130458) flutter/engine@16e2ab7...1b1ccdd 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617) 2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614) 2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593) 2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363) 2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118) 2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609) 2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606) 2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604) 2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…130496) Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions) flutter/engine@16e2ab7...b408036 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633) 2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631) 2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630) 2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628) 2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626) 2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625) 2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623) 2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621) 2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388) 2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617) 2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614) 2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593) 2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363) 2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118) 2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609) 2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606) 2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604) 2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...
…lutter#130458) flutter/engine@16e2ab7...1b1ccdd 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617) 2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614) 2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593) 2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363) 2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118) 2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609) 2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606) 2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604) 2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#130496) Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions) flutter/engine@16e2ab7...b408036 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633) 2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631) 2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630) 2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628) 2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626) 2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625) 2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623) 2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621) 2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388) 2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617) 2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614) 2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593) 2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363) 2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118) 2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609) 2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606) 2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607) 2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604) 2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...
~~Requires https://skia-review.googlesource.com/c/skia/+/692196~~ Landed Fixes flutter/flutter#126010 Looks correct on iOS, renders incorrectly on Android � . Might be related to all of our text bounds looking off. ### iOS ![flutter_03](https://github.com/flutter/engine/assets/8975114/50897fd0-47fd-4cad-b158-1662aa650092) ### Android ![flutter_04](https://github.com/flutter/engine/assets/8975114/bcefe843-71a0-4b98-956f-039918e9a4cc)
Requires https://skia-review.googlesource.com/c/skia/+/692196LandedFixes flutter/flutter#126010
Looks correct on iOS, renders incorrectly on Android 🤷 . Might be related to all of our text bounds looking off.
iOS
Android