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

[Impeller] Add rects to accumulator rather than bounds #37435

Merged
auto-submit merged 1 commit into from
Nov 8, 2022
Merged

[Impeller] Add rects to accumulator rather than bounds #37435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Add rects to accumulator rather than bounds 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
  • Loading branch information
@iskakaushik
iskakaushik committed Nov 8, 2022
commit 868f8a278ad8790db7da1b4c6752947ed7231bd4
7 changes: 6 additions & 1 deletion display_list/display_list_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,12 @@ void DisplayListBoundsCalculator::drawPicture(const sk_sp<SkPicture> picture,
}
void DisplayListBoundsCalculator::drawDisplayList(
const sk_sp<DisplayList> display_list) {
AccumulateOpBounds(display_list->bounds(), kDrawDisplayListFlags);
const SkRect bounds = display_list->bounds();
std::list<SkRect> rects =
Copy link
Contributor

@flar flar Nov 8, 2022

Choose a reason for hiding this comment

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

This is an expensive operation that must now be done on any bounds calculation whether or not it is attempting to accumulate an RTree. This option must be conditional.

display_list->rtree()->searchNonOverlappingDrawnRects(bounds);
for (const SkRect& rect : rects) {
AccumulateOpBounds(rect, kDrawDisplayListFlags);
Copy link
Contributor

@flar flar Nov 8, 2022

Choose a reason for hiding this comment

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

The use of kDrawDisplayListFlags here is problematic as it isn't clear if the attributes assigned to an entire display list apply to each individual rectangle.

For now, we don't apply any attributes, but in the future we might apply attributes and they can't necessarily be distributed like that. So, this works presently, but is a potential logic time bomb. Minimally a comment should be added to mention this discrepancy.

}
}
void DisplayListBoundsCalculator::drawTextBlob(const sk_sp<SkTextBlob> blob,
SkScalar x,
Expand Down
2 changes: 0 additions & 2 deletions testing/scenario_app/run_ios_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,10 @@ echo "Running simulator tests with Impeller"
echo ""

# Skip testFontRenderingWhenSuppliedWithBogusFont: https://github.com/flutter/flutter/issues/113250
# Skip testOneOverlayAndTwoIntersectingOverlays: https://github.com/flutter/flutter/issues/113251
set -o pipefail && xcodebuild -sdk iphonesimulator \
-scheme Scenarios \
-destination 'platform=iOS Simulator,OS=13.0,name=iPhone 8' \
clean test \
FLUTTER_ENGINE="$FLUTTER_ENGINE" \
-skip-testing "ScenariosUITests/BogusFontTextTest/testFontRenderingWhenSuppliedWithBogusFont" \
-skip-testing "ScenariosUITests/UnobstructedPlatformViewTests/testOneOverlayAndTwoIntersectingOverlays" \
INFOPLIST_FILE="Scenarios/Info_Impeller.plist" # Plist with FLTEnableImpeller=YES