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
Conversation
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
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, |
There was a problem hiding this comment.
Should this part also have a unit test?
gaaclarke
reviewed
Oct 6, 2023
| @@ -252,6 +252,7 @@ void Canvas::DrawRRect(Rect rect, Scalar corner_radius, const Paint& paint) { | |||
| auto path = PathBuilder{} | |||
| .SetConvexity(Convexity::kConvex) | |||
| .AddRoundedRect(rect, corner_radius) | |||
| .SetBounds(rect) | |||
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?
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?
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
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.
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
…136094) flutter/engine@1bb228d...eb5d5c6 2023-10-06 jonahwilliams@google.com [Impeller] Ensure known geometry has simple bounds computation. (flutter/engine#46623) 2023-10-06 jonahwilliams@google.com [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 chinmaygarde@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
Mairramer
pushed a commit
to Mairramer/flutter
that referenced
this pull request
Oct 10, 2023
…lutter#136094) flutter/engine@1bb228d...eb5d5c6 2023-10-06 jonahwilliams@google.com [Impeller] Ensure known geometry has simple bounds computation. (flutter/engine#46623) 2023-10-06 jonahwilliams@google.com [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 chinmaygarde@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
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.