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

[Impeller] track path convexity and use it to simplify fill tessellation. #41834

Merged
merged 10 commits into from
May 9, 2023

Conversation

jonahwilliams
Copy link
Member

We can do this on the GPU, but we'll need a fallback when we don't have polyline compute.

From experimentation, Skia has already computed this value when Impeller gets that path. Additionally, this will allow us to speed up tessellation of known convex shapes like ovals that we don't want to add special cases for in the API.

@jonahwilliams jonahwilliams requested a review from dnfield May 8, 2023 23:41
@jonahwilliams jonahwilliams marked this pull request as ready for review May 8, 2023 23:41
@jonahwilliams
Copy link
Member Author

Note that once we add the GPU case, we could probably simplify things a bit and delete the specialized RRect geometry.

@flutter-dashboard
Copy link

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.

Changes reported for pull request #41834 at sha f578b09

std::vector<Point> output;
output.reserve(polyline.points.size() * 3);

for (auto j = 0u; j < polyline.contours.size(); j++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs an assertion that there's only a single contour. Otherwise, please add some tests that show it works with multiple contours.

output.emplace_back(point_b);
output.emplace_back(Point(center_x, center_y));
}
// Some shapes don't seem to self close?
Copy link
Contributor

Choose a reason for hiding this comment

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

If you fill a path that is missing an explicit close, you need to add one.

We should just document this behavior in the header doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that when we convert to the polyline we should add this close if it isn't present. Anyway, this is handled below. i'll update the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do it in polyline conversion, we need to make sure to not add a close if it's for a stroke (or if a close is already present).

const auto& point_b = polyline.points[i];
output.emplace_back(point_a);
output.emplace_back(point_b);
output.emplace_back(Point(center_x, center_y));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not define this as a fan, and use kTriangeFan below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Triangle fan is not universally supported, and isn't included in the set of PrimitiveTypes

Comment on lines 52 to 55
std::vector<uint16_t> index(output.size());
for (auto i = 0u; i < output.size(); i++) {
index[i] = i;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should drop this and just use kTriangleFan. We need to teach the rest of the pipeline to not require an index buffer in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above, not supported

Copy link
Contributor

Choose a reason for hiding this comment

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

(Talked offline, I guess Metal removed this, so ok - instead just copy the vertices to the vertex buffer and make the index buffer form them into a strip).

Comment on lines 38 to 41
kUnknown,
kConvex,
kConcave,
kComplex,
Copy link
Contributor

Choose a reason for hiding this comment

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

Document these - I'm not sure what you mean by complex (holes? multiple non-overlapping polygons with different convexity? self-intersection?)

Copy link
Member Author

Choose a reason for hiding this comment

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

complex polygons have self intersection. I think for now since I'm not using these other cases, I'll drop it down to just unknown and convex

@@ -94,6 +101,10 @@ class Path {

FillType GetFillType() const;

void SetConvexity(Convexity value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd feel a little bit better about this if it lived on PathBuilder and only PathBuilder set it on Path. Right now the API is easy to misuse. I'd like to ideally get to a world where impeller::Path is immutable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, can do that

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Mostly looks good

@dnfield
Copy link
Contributor

dnfield commented May 9, 2023

Although the golden change looks a little sad -
image

@jonahwilliams
Copy link
Member Author

I'll remove the center point and see if that improves the golden. Technically we could just use one of the vertices as the "center" point, maybe that gives a better result.

{10, 0}, {10, 10}, {4, 4}, //
{10, 10}, {0, 10}, {4, 4}, //
{0, 10}, {0, 0}, {4, 4}, //
{0, 0}, {10, 0}, {10, 10}, {0, 10}, //
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

void SetConvexity(Convexity value);

Convexity GetConvexity() const;
bool GetIsConvex() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: IsConvex()

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dnfield
Copy link
Contributor

dnfield commented May 9, 2023

Looks like the golden change is still there but it's not quite as bad as I initially saw - there's some shift in how the line across the circle is stepped, but it's stepped a little unevenly in both cases.

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #41834 at sha dd1a1a5

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 9, 2023
@dnfield
Copy link
Contributor

dnfield commented May 9, 2023

Actually I'm confused. The original diff looked like it had no large steps. The second diff I looked at both sides had steps. Now when I click the latest one I don't see any changes

@jonahwilliams
Copy link
Member Author

sorry I just clicked approve...

@auto-submit auto-submit bot merged commit 8ca16cb into flutter:main May 9, 2023
@jonahwilliams jonahwilliams deleted the track_convexity branch May 9, 2023 18:46
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 9, 2023
jonahwilliams pushed a commit to flutter/flutter that referenced this pull request May 9, 2023
…126360)

flutter/engine@824cd09...8ca16cb

2023-05-09 jonahwilliams@google.com [Impeller] track path convexity and
use it to simplify fill tessellation. (flutter/engine#41834)
2023-05-09 skia-flutter-autoroll@skia.org Roll Skia from 9ba374e0b0f0 to
8c936fb9ba8e (7 revisions) (flutter/engine#41854)
2023-05-09 skia-flutter-autoroll@skia.org Roll Skia from 485cd3d0f9ca to
9ba374e0b0f0 (3 revisions) (flutter/engine#41849)
2023-05-09 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
oGF4h14qc40l5ANL3... to c4JvQUEOBHgtRdNPc... (flutter/engine#41848)
2023-05-09 skia-flutter-autoroll@skia.org Roll Dart SDK from
498cfa57165b to 4b4c0325fd5b (2 revisions) (flutter/engine#41847)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from oGF4h14qc40l to c4JvQUEOBHgt

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 aaclarke@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://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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants