[DisplayList] Create DrawDashedLine for paragraph code #53411
Conversation
|
It occurs to me that if the dashed lines are overlapping then that might be visible if the paint is not opaque. We could either:
I haven't checked with how Skia deals with that behavior, but it's important to note that this has only ever been used from the paragraph code which tends to not hand us overlapping dashes so this issue has never come up in practice. Now that I've exposed it as a DL primitive (to side-step having to support obscure path effect attributes) we can establish our own behavior, though I don't see a lot of internal uses lining up to use this feature. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
|
There are 2 sets of changes in the goldens (which I will leave untriaged for review purposes). The first set show more visible dashes against the black blob background of the "Ahem" render font. I'm not sure why they are more visible, but the old code used a bunch of moveTo/lineTo segments in a path whereas the new code uses individual drawLines. This is not a case of overlapping dashes/dots, though. The second set show a very specific difference which is a bug fix. The old path code in the paragraph renderer would always draw full dashes even when they went past the end of the |
|
I have one minor fix to the acceptance tests at the top of I'm also looking to write a more specific Playground golden test rather than rely on the results of the Text goldens that are seen in the existing golden failures. |
|
I have updated the logic as mentioned above and added a dash-specific Impeller golden test. This should be ready for review... |
|
If we keep the dash as a single path with multiple contours, then we get overlap detection for free. There is also a cost per draw that will make individual draw lines much more expensive |
Another consideration is consumption of depth values for N separate ops rather than a single one. The current use of multiple DrawLine commands overflows the depth accounting that was done in dl_builder. The regular Canvas implementation of Impeller ignores that information and so doesn't show a problem there. The new ExperimentalCanvas does use this depth information, but it wasn't straightforward to run a test case with it to see the clip failures so I just switched to Path anyway for this and other reasons. I also updated the dl_golden test case to include a clipped section to make sure this never is a problem. |
|
Golden file changes are available for triage from new commit, Click here to view. |
…150399) flutter/engine@a4f266f...1c4e5e2 2024-06-17 flar@google.com [DisplayList] Create DrawDashedLine for paragraph code (flutter/engine#53411) 2024-06-17 chinmaygarde@google.com [Impeller] Update Android CPU profiling instructions. (flutter/engine#53437) 2024-06-17 jonahwilliams@google.com [Impeller] move draw vertices to dl unittests. (flutter/engine#53400) 2024-06-17 chinmaygarde@google.com [Impeller] Link CPU profiling docs from the main README. (flutter/engine#53435) 2024-06-17 skia-flutter-autoroll@skia.org Roll Dart SDK from 1ce6b4d54247 to 51bbba0da7d3 (1 revision) (flutter/engine#53432) 2024-06-17 jonahwilliams@google.com [Impeller] Move drawAtlas golden tests to display list. (flutter/engine#53398) 2024-06-17 fmil@google.com [flatland] Handle fence overflow in flatland_connection.cc (flutter/engine#53366) 2024-06-17 skia-flutter-autoroll@skia.org Roll Skia from 0dda1054f543 to 2b9bc16df969 (1 revision) (flutter/engine#53431) 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 chinmaygarde@google.com,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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
As of #53411 there are no more (non-test) references to path effects anywhere in the engine. Deleting the dead code.
With this minor addition to the DlCanvas/DisplayList API the code in the paragraph builder no longer needs to worry about PathEffect objects and their varying support on the backends.
At this point all PathEffect code in the engine is obsolete and can be deleted, but I'll leave that for a follow-on PR.
The only PathEffect related thing I did delete was support for rendering primitives with a PathEffect in the DL Rendering tests, both because it is a vestigial attribute and also because it would interfere with the new DrawDashedLine rendering test (a PathEffect on top of a PathEffect...).