This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Impeller] Ensure known geometry has simple bounds computation.#46623
Merged
auto-submit[bot] merged 3 commits intoflutter:mainfrom Oct 6, 2023
Merged
[Impeller] Ensure known geometry has simple bounds computation.#46623auto-submit[bot] merged 3 commits intoflutter:mainfrom
auto-submit[bot] merged 3 commits intoflutter:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
zanderso
reviewed
Oct 6, 2023
| PathBuilder{} | ||
| .AddCircle(center, radius) | ||
| .SetConvexity(Convexity::kConvex) | ||
| .SetBounds(Rect::MakeLTRB(center.x - radius, center.y - radius, |
Member
There was a problem hiding this comment.
Should this part also have a unit test?
gaaclarke
reviewed
Oct 6, 2023
| auto path = PathBuilder{} | ||
| .SetConvexity(Convexity::kConvex) | ||
| .AddRoundedRect(rect, corner_radius) | ||
| .SetBounds(rect) |
Member
There was a problem hiding this comment.
So, the gist is that if you don't call SetBounds, it is getting calculated instead? What about rounded rects that have extreme corner radii? Does that become a shape whose bounds are different than the one specified in the draw call?
Contributor
Author
There was a problem hiding this comment.
Yes, normally we use the Skia computed bounds - but the Aiks API doesn't accomidated that. What are you thinking of for extreme corner radii?
Contributor
Author
There was a problem hiding this comment.
Running some tests:
for (auto i = - 100; i < 100; i += 10) {
SkRRect rrect = SkRRect::MakeRectXY(SkRect::MakeLTRB(0, 0, 100, 100), i, i);
SkPath path = SkPath().addRRect(rrect);
auto bds = path.getBounds();
FML_LOG(ERROR) << "Bounds for RRect with radius " << i << " -> " << bds.x() << "," << bds.y() << "m" << bds.width() << "," << bds.height();
}
Result:
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -100 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -90 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -80 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -70 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -60 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -50 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -40 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -30 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -20 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -10 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 0 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 10 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 20 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 30 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 40 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 50 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 60 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 70 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 80 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 90 -> 0,0m100,100
Member
There was a problem hiding this comment.
I was wondering if when the corner radius was high would it start looking more like a circle that was inset in the rect. LGTM.
Contributor
There was a problem hiding this comment.
Did you try this with reversed rects? RRects is one of the areas where Skia was doing "auto-sorting" of the corners of the rects...
jonahwilliams
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 6, 2023
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Oct 6, 2023
flutter/engine@1bb228d...eb5d5c6 2023-10-06 [email protected] [Impeller] Ensure known geometry has simple bounds computation. (flutter/engine#46623) 2023-10-06 [email protected] [Impeller] Refactor CapabilitiesGLES into a Capabilties. (flutter/engine#46621) 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://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
Mairramer
pushed a commit
to Mairramer/flutter
that referenced
this pull request
Oct 10, 2023
…6094) flutter/engine@1bb228d...eb5d5c6 2023-10-06 [email protected] [Impeller] Ensure known geometry has simple bounds computation. (flutter/engine#46623) 2023-10-06 [email protected] [Impeller] Refactor CapabilitiesGLES into a Capabilties. (flutter/engine#46621) 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://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
harryterkelsen
pushed a commit
that referenced
this pull request
Oct 23, 2023
Computing the bounds of an RRect can be really slow if you don't actually know that it is an RRect, and shows up in the traces for flutter gallery on wembly. This change should be semantically equivalent and is only an optimization to avoid recomputing the same value we already know.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Computing the bounds of an RRect can be really slow if you don't actually know that it is an RRect, and shows up in the traces for flutter gallery on wembly. This change should be semantically equivalent and is only an optimization to avoid recomputing the same value we already know.