[Impeller] Refactor SolidFill to use Path/Cover Geometry classes by jonahwilliams · Pull Request #36661 · flutter/engine

jonahwilliams · 2022-10-07T18:05:56Z

Work towards go/impeller-geometry.

Notes:

Added ISize to the GeometryPosition instead of UVs to match how existing solid fill shaders work.
Always returning a huge rect from coverage for CoverGeometry, not sure if that is the correct approach.
Deleted solid_fill_utils since all usages of this code are just inlined into geometry.

jonahwilliams · 2022-10-07T19:09:22Z

impeller/entity/geometry.cc

+}
+
+std::optional<Rect> CoverGeometry::GetCoverage(Matrix transform) {
+  return Rect::MakeLTRB(-INFINITY, -INFINITY, INFINITY, INFINITY);


crowdsourcing some alternative ideas here 😄

Handling infinity rects has been a long time coming. I believe infinity coverage shouldn't cause a problem for EntityPass in its current state given the frustum/stencil clip cropping. We've already got a few Aiks tests using DrawPaint -- including ColorWheel. If none of these tests crash when flipping through them then we're probably good to go.

Ah, I just noticed this doesn't replace DrawPaint yet. I'd say this is good. If we find problems down the road when landing it they should be easy fixes. It also occurs to me that we still need move comprehensive filter cropping support.
Although eventually we should just ignore most image filters and execute color filters on the CPU for the DrawPaint case anyway.

jonahwilliams · 2022-10-07T19:40:40Z

impeller/entity/contents/solid_color_contents.cc

    return std::nullopt;
  }
-  return path_.GetTransformedBoundingBox(entity.GetTransformation());
+  if (geometry_ == nullptr) {


This worked before because we'd end up with an empty path here if nothing was set, and we have a test that verifies this works for SolidColorContents, but nothing else.

Is there a valid use-case for drawing a contents without a path or cover, or should this assert instead?

bdero · 2022-10-07T21:26:59Z

impeller/geometry/rect.h

 using IRect = TRect<int64_t>;

+constexpr Rect kLargestCover =
+    Rect::MakeLTRB(-INFINITY, -INFINITY, INFINITY, INFINITY);


What about adding a Rect::MakeMaximum constructor?

Also, std::numeric_limits<Scalar>::infinity?

Done, I think :)

jonahwilliams · 2022-10-07T22:22:34Z

impeller/entity/geometry.cc

+    HostBuffer& host_buffer,
+    std::shared_ptr<Tessellator> tessellator,
+    ISize render_target_size) {
+  auto rect = Rect(Size(render_target_size));


Updated this to skip tessellation

bdero

LGTM! Just some minor comments.

It's nice to see those cover branches moved out of the Contents::Render impls. They've been making me feel uneasy for a while now.

bdero · 2022-10-08T00:51:29Z

impeller/entity/contents/clip_contents.cc

-void ClipContents::SetPath(Path path) {
-  path_ = std::move(path);
+void ClipContents::SetGeometry(std::unique_ptr<Geometry> geometry) {
+  geometry_ = std::move(geometry);


Ah yes, clip paths... Even though clips can only be specified using paths right now, using Geometry SGTM because we'll want clip paths to benefit from all the special optimizations we're gonna land over time. Heck, we might even produce polylines from DL paths directly at some point.

bdero · 2022-10-08T01:03:09Z

impeller/entity/geometry.cc

+  auto rect = Rect(Size(render_target_size));
+  constexpr uint16_t kRectIndicies[6] = {0, 1, 2, 1, 2, 3};
+  return GeometryResult{
+      .type = PrimitiveType::kTriangle,


Nit: This could also be a triangle strip with 4 indices! ;)

bdero · 2022-10-08T01:08:19Z

impeller/geometry/rect.h

+    auto wx = origin.x + size.width;
+    auto wy = origin.y + size.height;
+    return {origin.x, origin.y, wx, origin.y, origin.x, wy, wx, wy};
+  }


This data layout is identical to GetPoints. Can we use that instead?

bdero · 2022-10-08T01:14:00Z

impeller/entity/contents/solid_color_contents.cc

@@ -62,12 +59,13 @@ bool SolidColorContents::Render(const ContentContext& renderer,
      renderer.GetSolidFillPipeline(OptionsFromPassAndEntity(pass, entity));


Note to self: We should clean up these these pipelines later on by using the new vertex shaders/deleting the old ones.

…ses (flutter/engine#36661)

jonahwilliams added 11 commits October 6, 2022 14:54

[Impeller] geometry + vertices

ad01c70

++

8f28379

++

c129201

++

2d911ec

++

5569ecf

++

cce1ee9

Merge branch 'master' of github.com:flutter/engine into geometry_start

35d9814

[Impeller] use Geometry for solid cover

a784d21

Merge branch 'master' of github.com:flutter/engine into geometry_start

883c70a

++

42a3b74

++

8d4c416

jonahwilliams self-assigned this Oct 7, 2022

jonahwilliams added the e: impeller label Oct 7, 2022

jonahwilliams requested review from bdero and dnfield October 7, 2022 18:06

jonahwilliams commented Oct 7, 2022

View reviewed changes

++

91ebedb

jonahwilliams commented Oct 7, 2022

View reviewed changes

bdero reviewed Oct 7, 2022

View reviewed changes

bdero review

0f4f829

jonahwilliams commented Oct 7, 2022

View reviewed changes

bdero approved these changes Oct 8, 2022

View reviewed changes

jonahwilliams added 2 commits October 7, 2022 18:25

++

9573ab2

++

5eb03d2

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 8, 2022

auto-submit bot merged commit feb8405 into flutter:main Oct 8, 2022

engine-flutter-autoroll mentioned this pull request Oct 8, 2022

Roll Flutter Engine from b75d9bd05c17 to feb84056107f (1 revision) flutter/flutter#113138

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 8, 2022

feb8405 [Impeller] Refactor SolidFill to use Path/Cover Geometry clas…

4022031

…ses (flutter/engine#36661)

		@@ -62,12 +59,13 @@ bool SolidColorContents::Render(const ContentContext& renderer,
		renderer.GetSolidFillPipeline(OptionsFromPassAndEntity(pass, entity));

Conversation

jonahwilliams commented Oct 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bdero Oct 7, 2022 •

edited

Loading

bdero Oct 7, 2022 •

edited

Loading