[Impeller] geometry changes to support line/point style. by jonahwilliams · Pull Request #56340 · flutter/engine

jonahwilliams · 2024-11-02T19:41:01Z

Split off from #55230 . Adds getter that tracks if a Path is a single contour.

################################

gaaclarke · 2024-11-07T18:04:08Z

impeller/geometry/path_component.h

  explicit GLESVertexWriter(std::vector<Point>& points,
-                            std::vector<uint16_t>& indices);
+                            std::vector<uint16_t>& indices,
+                            bool line_strip = false);


Can you add a docstring for what line_strip is? Maybe just link to tessellator's documentation.

gaaclarke · 2024-11-07T18:07:26Z

impeller/tessellator/tessellator.h

-  /// @return A point vector containing the vertices in triangle strip format.
+  ///                        Matrix::GetMaxBasisLengthXY of the CTM applied to
+  ///                        the path for rendering.
+  /// @param[in]  line_strip if true, generates line strip geometry instead of a


What is a "line strip"? A triangle strip that makes a line?

A line strip is a GL_LINE_STRIP geometry. How about a link to the primitive type?

I don't understand why a tessellator would be generating a line strip, tessellation means converting something to packed polygons (triangles in our case). I see, this is doing stuff like converting a bezier path to a line strip?

Yea, mentioning GL_LINE_STRIP would help to distinguish it from a line make with a triangle strip. Especially since a generating a GL_LINE_STRIP isn't technically tessellation.

The brief for this method also explicitly says it creates a triangle fan structure, that should be updated.

Now that I think of it. Don't you think this would be better off as it's own function instead of a bool parameter? Generally it's advised to make procedures do 1 thing. We can share the code in a helper function.

Yeah I agree with you, a separate method would be better in this case. I suppose since its a line primitive its not exactly tessellation but primitive generation :)

gaaclarke · 2024-11-07T18:09:43Z

impeller/tessellator/tessellator.cc

+      point_buffer.GetBuffer()->Flush(point_buffer.GetRange());
+      index_buffer.GetBuffer()->Flush(index_buffer.GetRange());


Is there a test for these changes?

I don't have a test case for this, except for the fact that it may or may not work on swiftshader. I don't want to add a mock test for these, instead I think switching to the point arena like we did for strokes is probably better?

So, you're saying if this wasn't here the goldens would look wrong? Sounds good to me.

It would be racy :(

gaaclarke

lgtm! thanks

auto-submit · 2024-11-07T23:52:46Z

auto label is removed for flutter/engine/56340, due to - The status or check suite Mac mac_ios_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

flar · 2024-11-08T01:41:48Z

impeller/geometry/path_component.h

+
+  std::pair<size_t, size_t> GetVertexCount() const;
+
+  const std::vector<Point>& GetOveriszedBuffer() const;


typo Overiszed

Fixed, thanks!

…tter/engine#56340)

flutter/engine@7b3eacd...35041f1 2024-11-11 skia-flutter-autoroll@skia.org Roll Dart SDK from a393f3ed040a to 69170fac244c (1 revision) (flutter/engine#56513) 2024-11-11 chris@bracken.jp iOS,macOS: Refactor TestMetalContext for ARC (flutter/engine#56510) 2024-11-11 jonahwilliams@google.com [Impeller] geometry changes to support line/point style. (flutter/engine#56340) 2024-11-11 skia-flutter-autoroll@skia.org Roll Skia from af6a4f9a85ee to 11046fd10394 (9 revisions) (flutter/engine#56508) 2024-11-11 jonahwilliams@google.com [Impeller] dont unnecessarily copy point data out of display list. (flutter/engine#56492) 2024-11-11 jonahwilliams@google.com [Impeller] fix line/polygon depth and GLES scissor state. (flutter/engine#56494) 2024-11-11 jason-simmons@users.noreply.github.com Do not stop flutter_tester if microtasks are still pending (flutter/engine#56432) 2024-11-11 skia-flutter-autoroll@skia.org Roll Skia from 261316c10484 to af6a4f9a85ee (5 revisions) (flutter/engine#56505) 2024-11-11 skia-flutter-autoroll@skia.org Roll Dart SDK from 4ea43aa234a4 to a393f3ed040a (1 revision) (flutter/engine#56506) 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 jsimmons@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

…ine#56340) Split off from flutter/engine#55230 . Adds getter that tracks if a Path is a single contour. ################################ flutter#152702

jonahwilliams added 2 commits November 2, 2024 12:37

[Impeller] geometry changes to support line/point style.

031fc72

add missing flushes to tessellator.

f37cfbf

github-actions bot added the e: impeller label Nov 2, 2024

jonahwilliams requested review from flar and gaaclarke November 4, 2024 17:02

gaaclarke reviewed Nov 7, 2024

View reviewed changes

jonahwilliams added 3 commits November 7, 2024 11:05

line strip tessellator.

34ba6ac

Merge branch 'main' of github.com:flutter/engine into split_geom_1

91c8860

line strip primitive generator.

397f7ad

jonahwilliams requested a review from gaaclarke November 7, 2024 19:48

gaaclarke approved these changes Nov 7, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2024

flar reviewed Nov 8, 2024

View reviewed changes

typo

879f9d0

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 11, 2024

auto-submit bot merged commit 980e4d2 into flutter:main Nov 11, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 11, 2024

engine-flutter-autoroll mentioned this pull request Nov 11, 2024

Roll Flutter Engine from 7b3eacd20eb6 to 980e4d20dfd8 (7 revisions) flutter/flutter#158478

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 11, 2024

980e4d2 [Impeller] geometry changes to support line/point style. (flu…

c309e27

…tter/engine#56340)

engine-flutter-autoroll mentioned this pull request Nov 11, 2024

Roll Flutter Engine from 7b3eacd20eb6 to 73ccb5b55f05 (8 revisions) flutter/flutter#158482

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 11, 2024

980e4d2 [Impeller] geometry changes to support line/point style. (flu…

3d2308b

…tter/engine#56340)

engine-flutter-autoroll mentioned this pull request Nov 12, 2024

Roll Flutter Engine from 7b3eacd20eb6 to 35041f118744 (9 revisions) flutter/flutter#158485

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2024

980e4d2 [Impeller] geometry changes to support line/point style. (flu…

eb5d96a

…tter/engine#56340)

		point_buffer.GetBuffer()->Flush(point_buffer.GetRange());
		index_buffer.GetBuffer()->Flush(index_buffer.GetRange());


		std::pair<size_t, size_t> GetVertexCount() const;

		const std::vector<Point>& GetOveriszedBuffer() const;

Conversation

jonahwilliams commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Nov 7, 2024

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!

3 participants

Comments

jonahwilliams commented Nov 2, 2024 •

edited

Loading