[Impeller] Fill/StrokePathGeometry use geom objects as path sources #168125
Conversation
github-actions
bot
added
engine
|
The spiritual successor to #167863, now with polymorphism to simplify the geometry object ownership. |
|
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. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The goldens look fine - similar edge coverage pixels on rounded corners that we've seen before. Most likely caused by the switch from cubics to conics to approximate the corners of round rects. Conics are more accurate and are what Skia uses, but |
|
Spot checking some of the Google failures, the first couple of failures were basically someone else's change to Dart code that this PR couldn't have possibly been responsible for. |
…stroke-path-geometry-with-geom-objects
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
This approach looks good to me. I just have a couple of notes.
|
|
||
| namespace impeller { | ||
|
|
||
| /// @brief A geometry that is created from a filled path object. |
There was a problem hiding this comment.
Does this need some refinement after this change? Probably worth mentioning it's an abstract base class.
There was a problem hiding this comment.
I've completely revamped the doc comments for these base classes and all of the sub-classes and also moved their implementations into their type-specific source files.
There was a problem hiding this comment.
Leaving this open so you can look at all of the class doc comments when I update them.
There was a problem hiding this comment.
The new stuff is pushed now, and I added an implementation of CoversArea for the new generalized round rect class which wasn't feasible before when it was turned into a path.
engine/src/flutter/impeller/entity/geometry/fill_path_geometry.h
Outdated
Show resolved
Hide resolved
engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.h
Outdated
Show resolved
Hide resolved
engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.h
Outdated
Show resolved
Hide resolved
…stroke-path-geometry-with-geom-objects
|
A follow-on task would be to have StrokeParameters defined in a general include file and shared by both Paint and StrokePathGeometry... |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, I think the stroke params really cleaned things up.
flutter-dashboard
bot
removed
the
autosubmit
…lutter#168125) When basic rendering operations end up in a "general case" situation in Impeller, it converts the basic shape into a path and calls `DrawPath`. But, the creation of the path is expensive and all the mechanisms behind drawing a path now only need a `PathSource` object which can replay the path to them. We now have explicit lightweight path source generators which can feed the path drawing operations directly from the source data without having to create a full `impeller::Path` object.
…#168276) flutter#168125 introduced StrokeParameters to hold all of the stroke-related values, but was only used in the rendering code. We now use it across Impeller to describe stroke decorations.
…lutter#168125) When basic rendering operations end up in a "general case" situation in Impeller, it converts the basic shape into a path and calls `DrawPath`. But, the creation of the path is expensive and all the mechanisms behind drawing a path now only need a `PathSource` object which can replay the path to them. We now have explicit lightweight path source generators which can feed the path drawing operations directly from the source data without having to create a full `impeller::Path` object.
…#168276) flutter#168125 introduced StrokeParameters to hold all of the stroke-related values, but was only used in the rendering code. We now use it across Impeller to describe stroke decorations.
2 participants
When basic rendering operations end up in a "general case" situation in Impeller, it converts the basic shape into a path and calls
DrawPath. But, the creation of the path is expensive and all the mechanisms behind drawing a path now only need aPathSourceobject which can replay the path to them.We now have explicit lightweight path source generators which can feed the path drawing operations directly from the source data without having to create a full
impeller::Pathobject.