Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Make Entity move only, simplify construction of geometry/filter contents. #48596

Merged
merged 12 commits into from
Dec 5, 2023

Conversation

jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Dec 2, 2023

Redo of #48585

Removes the Copy constructor of Entity, adds a clone method. Allows us to ensure that we move entity into the entity pass without using rvalue reference parameters

Fixes flutter/flutter#139569

@chinmaygarde chinmaygarde changed the title [Impeller] simplify and speed up entity construction. [Impeller] Simplify and speed up entity construction. Dec 4, 2023
@jonahwilliams jonahwilliams changed the title [Impeller] Simplify and speed up entity construction. [Impeller] Make Entity move only, simplify construction of geometry/filter contents. Dec 5, 2023
@@ -23,6 +25,80 @@

namespace impeller {

namespace {

static std::shared_ptr<Contents> CreateContentsForGeometryWithFilters(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also fixes the double application of color filters.

Copy link
Member

Choose a reason for hiding this comment

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

Can you link an issue please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filled here: flutter/flutter#139569

@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 #48596 at sha 307b46a

@flutter-dashboard
Copy link

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

Changes reported for pull request #48596 at sha cbc636a

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Nice, looks good, there are just a couple of cleanup suggestions that are worth doing.

Comment on lines 77 to 79
static std::shared_ptr<Contents> CreateContentsWithFilters(const Paint& paint,
Path path = {},
bool cover = false) {
Copy link
Member

Choose a reason for hiding this comment

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

This function does 2 different things. If cover == true, path is completely ignored. Can you split this into 2 different functions.

CreateContentsWithFilters(paint, path) and CreateCoverContentsWithFilters(paint)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 178 to 179
Entity copy = *this;
return copy;
Copy link
Member

Choose a reason for hiding this comment

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

return Entity(*this);? I want to make to make the decision to elide the copy as easy as possible for the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

elements_.insert(elements_.end(),
std::make_move_iterator(pass->elements_.begin()),
std::make_move_iterator(pass->elements_.end()));
auto& elements = pass->elements_;
Copy link
Member

Choose a reason for hiding this comment

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

Remove auto please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM!

@flutter-dashboard
Copy link

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

Changes reported for pull request #48596 at sha bb638b5

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 5, 2023
@auto-submit auto-submit bot merged commit 0924225 into flutter:main Dec 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 5, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 5, 2023
…139583)

flutter/engine@7133378...cfdaecc

2023-12-05 [email protected] A11y enabled state (flutter/engine#48653)
2023-12-05 [email protected] Roll Skia from bf1db1c75704 to c0ad3e9bdec0 (1 revision) (flutter/engine#48685)
2023-12-05 [email protected] [Impeller] started taking into account integer gaps in blur sigma, turned on linear filter (flutter/engine#48651)
2023-12-05 [email protected] Roll Dart SDK from 21574eae3a69 to 8bfec2c7ed43 (2 revisions) (flutter/engine#48683)
2023-12-05 [email protected] Update labeler to 5.0.0, fix yaml format for labeler 5.0.0 (flutter/engine#48682)
2023-12-05 [email protected] [Impeller] Make Entity move only, simplify construction of geometry/filter contents. (flutter/engine#48596)
2023-12-05 [email protected] [Impeller] Delete tessellation control/eval shader support. (flutter/engine#48649)

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 [email protected],[email protected],[email protected] 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Impeller] CPU applied color filters can run twice.
2 participants