[Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. #45393

jonahwilliams · 2023-09-01T19:15:40Z

This is a design problem in the Flutter API, that in general you can't assume a Path hasn't changed from the last time you used it. We're so close, so just move the mutation methods to PathBuilder :)

Fixes flutter/flutter#133880

chinmaygarde · 2023-09-01T19:19:19Z

impeller/geometry/path.h

 ///
-///             Creating paths that describe complex shapes is usually done by a
-///             path builder.
+///             Paths are externally immutable once created, Creating paths must


jonahwilliams · 2023-09-01T21:38:43Z

So we might have a bug in path builder, or I'm misunderstanding the point of the test. We had a test case "PathCreatePolyLineDoesNotDuplicatePoints"

engine/impeller/geometry/geometry_unittests.cc

Lines 2009 to 2026 in f151289

    
           TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) { 
        
             Path path; 
        
             path.AddContourComponent({10, 10}); 
        
             path.AddLinearComponent({10, 10}, {20, 20}); 
        
             path.AddLinearComponent({20, 20}, {30, 30}); 
        
             path.AddContourComponent({40, 40}); 
        
             path.AddLinearComponent({40, 40}, {50, 50}); 
        
             auto polyline = path.CreatePolyline(1.0f); 
        
             ASSERT_EQ(polyline.contours.size(), 2u); 
        
             ASSERT_EQ(polyline.points.size(), 5u); 
        
             ASSERT_EQ(polyline.points[0].x, 10); 
        
             ASSERT_EQ(polyline.points[1].x, 20); 
        
             ASSERT_EQ(polyline.points[2].x, 30); 
        
             ASSERT_EQ(polyline.points[3].x, 40); 
        
             ASSERT_EQ(polyline.points[4].x, 50); 
        
           }

I updated it to use the PathBuilder API in a way that I think is correct. However, now it seems like the polyline does in fact have duplicated points.

jonahwilliams · 2023-09-01T21:42:00Z

The points I get from the tests are now:

[ERROR:flutter/impeller/geometry/geometry_unittests.cc(2035)] (10, 10)
[ERROR:flutter/impeller/geometry/geometry_unittests.cc(2035)] (20, 20)
[ERROR:flutter/impeller/geometry/geometry_unittests.cc(2035)] (20, 20)
[ERROR:flutter/impeller/geometry/geometry_unittests.cc(2035)] (30, 30)
[ERROR:flutter/impeller/geometry/geometry_unittests.cc(2035)] (40, 40)
[ERROR:flutter/impeller/geometry/geometry_unittests.cc(2035)] (50, 50)

WHich I think is because PathBuilder is inserting Moves. However, we're only ever building paths with PathBuilder today, so I'm not sure if the expectation is overly specific to the original bug.

FYI @bdero

bdero · 2023-09-01T22:57:14Z

impeller/geometry/geometry_unittests.cc

+  PathBuilder builder;
+  builder.MoveTo({10, 10});
+  builder.AddLine({10, 10}, {20, 20});
+  builder.AddLine({20, 20}, {30, 30});


Add* inserts a new contour -- this actually needs to be LineTo to be equivalent.

builder.MoveTo({10, 10}); builder.LineTo({20, 20}); builder.LineTo({30, 30});

And no, you're not crazy. Yes, the linear component has redundant data.

Aghh, good catch. Thank you

flutter-dashboard · 2023-09-02T00:45:06Z

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 #45393 at sha 923f7b7

…to use PathBuilder to create Path. (flutter/engine#45393)

…33924) Manual roll requested by zra@google.com flutter/engine@489c399...e496eec 2023-09-02 skia-flutter-autoroll@skia.org Roll Skia from 2d8849f9f0cc to 15f77147a3ec (1 revision) (flutter/engine#45414) 2023-09-02 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from OF4TS05qlWCjukWw6... to MesZPNdj-uw8VdCyV... (flutter/engine#45413) 2023-09-02 dkwingsmt@users.noreply.github.com Remove --disable-service-auth-codes (flutter/engine#45356) 2023-09-02 bdero@google.com [Impeller] Import cstring for memcpy. (flutter/engine#45408) 2023-09-02 skia-flutter-autoroll@skia.org Roll Dart SDK from cdf1ce0c6d7e to a5c7102af509 (1 revision) (flutter/engine#45412) 2023-09-02 skia-flutter-autoroll@skia.org Roll ANGLE from 179bd7762ffa to ebf1e7163216 (1 revision) (flutter/engine#45411) 2023-09-02 dkwingsmt@users.noreply.github.com Remove deprecated MOCK_METHODx calls (flutter/engine#45307) 2023-09-02 jonahwilliams@google.com [Impeller] Better demonstrate blur and draw picture? (flutter/engine#45388) 2023-09-02 jonahwilliams@google.com [Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. (flutter/engine#45393) 2023-09-02 skia-flutter-autoroll@skia.org Roll ANGLE from 962fdf7b7882 to 179bd7762ffa (1 revision) (flutter/engine#45409) 2023-09-02 flar@google.com Cull the RTree bounds when they are forwarded in DrawDisplayList (flutter/engine#45358) 2023-09-02 skia-flutter-autoroll@skia.org Roll Skia from fedff79a6afc to 2d8849f9f0cc (3 revisions) (flutter/engine#45407) 2023-09-02 jonahwilliams@google.com [impeller] premultiply vertices colors. (flutter/engine#45406) 2023-09-01 skia-flutter-autoroll@skia.org Roll ANGLE from 6a09e41ce6ea to 962fdf7b7882 (224 revisions) (flutter/engine#45400) 2023-09-01 skia-flutter-autoroll@skia.org Roll Skia from 22ae23891e8e to fedff79a6afc (1 revision) (flutter/engine#45405) 2023-09-01 30870216+gaaclarke@users.noreply.github.com [Impeller] turned on validations for all debug builds (flutter/engine#45350) 2023-09-01 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from sk7JBGzW1Jw10Wy-T... to OF4TS05qlWCjukWw6... (flutter/engine#45403) 2023-09-01 skia-flutter-autoroll@skia.org Roll Skia from 2c0405489966 to 22ae23891e8e (1 revision) (flutter/engine#45402) 2023-09-01 737941+loic-sharma@users.noreply.github.com [Windows] Update vsync on raster thread (flutter/engine#45310) 2023-09-01 skia-flutter-autoroll@skia.org Roll Dart SDK from a2ea759c16cc to cdf1ce0c6d7e (1 revision) (flutter/engine#45397) 2023-09-01 skia-flutter-autoroll@skia.org Roll Skia from f3f6c733c7e6 to 2c0405489966 (1 revision) (flutter/engine#45396) 2023-09-01 skia-flutter-autoroll@skia.org Roll Skia from 02fa14799c6c to f3f6c733c7e6 (1 revision) (flutter/engine#45394) 2023-09-01 skia-flutter-autoroll@skia.org Roll Skia from d5d3b0d4ee77 to 02fa14799c6c (2 revisions) (flutter/engine#45392) 2023-09-01 41930132+hellohuanlin@users.noreply.github.com [ios][ios17][text_input]fix text input system highlight in iOS 17 Beta 7 with firstRectForRange (flutter/engine#45303) 2023-09-01 skia-flutter-autoroll@skia.org Roll Skia from d6266ef14a7e to d5d3b0d4ee77 (2 revisions) (flutter/engine#45389) 2023-09-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 0c121a6431cc to a2ea759c16cc (1 revision) (flutter/engine#45384) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from sk7JBGzW1Jw1 to MesZPNdj-uw8 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

jonahwilliams added 2 commits September 1, 2023 12:12

[Impeller] make Paths externally immutable, always use PathBuilder.

db62ff2

[Impeller] make Paths externally immutable, always use PathBuilder.

34a5161

github-actions bot added the e: impeller label Sep 1, 2023

chinmaygarde approved these changes Sep 1, 2023

View reviewed changes

chinmaygarde assigned jonahwilliams Sep 1, 2023

jonahwilliams added 2 commits September 1, 2023 12:22

++

092414e

++

47a55fd

bdero reviewed Sep 1, 2023

View reviewed changes

jonahwilliams added 2 commits September 1, 2023 17:09

add some docs and fix test.

49af6b9

++

923f7b7

flutter-dashboard bot added the will affect goldens label Sep 2, 2023

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 2, 2023

auto-submit bot merged commit eabe8fb into flutter:main Sep 2, 2023

engine-flutter-autoroll mentioned this pull request Sep 2, 2023

Roll Flutter Engine from 489c399e3dc6 to e496eec40e21 (26 revisions) flutter/flutter#133909

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 2, 2023

eabe8fb [Impeller] Make paths externally immutable, update all tests …

b2b7a7e

…to use PathBuilder to create Path. (flutter/engine#45393)

engine-flutter-autoroll mentioned this pull request Sep 2, 2023

Roll Flutter Engine from 489c399e3dc6 to e496eec40e21 (26 revisions) flutter/flutter#133916

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 2, 2023

eabe8fb [Impeller] Make paths externally immutable, update all tests …

f7929d4

…to use PathBuilder to create Path. (flutter/engine#45393)

jonahwilliams deleted the make_paths_imm branch September 2, 2023 19:41

engine-flutter-autoroll mentioned this pull request Sep 2, 2023

Manual roll Flutter Engine from 489c399e3dc6 to e496eec40e21 (26 revisions) flutter/flutter#133924

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 2, 2023

eabe8fb [Impeller] Make paths externally immutable, update all tests …

40adb3e

…to use PathBuilder to create Path. (flutter/engine#45393)

chinmaygarde mentioned this pull request Sep 8, 2023

[Impeller] Improve Path API usability flutter/flutter#100262

Closed

gnprice mentioned this pull request Sep 11, 2023

Window resizing is broken on windows desktop. flutter/flutter#134262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. #45393

[Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. #45393

Uh oh!

jonahwilliams commented Sep 1, 2023

Uh oh!

chinmaygarde Sep 1, 2023

Uh oh!

jonahwilliams commented Sep 1, 2023

Uh oh!

jonahwilliams commented Sep 1, 2023

Uh oh!

bdero Sep 1, 2023 •

edited

Loading

Uh oh!

jonahwilliams Sep 1, 2023

Uh oh!

flutter-dashboard bot commented Sep 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. #45393

[Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. #45393

Uh oh!

Conversation

jonahwilliams commented Sep 1, 2023

Uh oh!

chinmaygarde Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Sep 1, 2023

Uh oh!

jonahwilliams commented Sep 1, 2023

Uh oh!

bdero Sep 1, 2023 • 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!

jonahwilliams Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Sep 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bdero Sep 1, 2023 •

edited

Loading