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

Conversation

@hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented Mar 26, 2024

The previous PR flutter/flutter#143420 rounds out the layers and rounds in the platform views. This results in missing pixel on the edge of the intersection when there's fractional coordinate (as shown in the screenshot below), because platform view is below the layers.

It turns out that we have to round out both platform view and layers, because:

  • rounding in platform view rects will result in missing pixels on the edge of the intersection.
  • rounding in layer rects will result in missing pixels on the edge of the layer that's on top of the platform view.

This PR simply skips the single (or partial) pixel on the edge, which is a special case, while still preserve the roundOut behavior for general non-edge cases.

Before the fix, notice a very thin gray line cutting through the purple box:

Then after the fix, the gray line is gone:

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#143420

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@hellohuanlin hellohuanlin force-pushed the platform_view_intersection_ignore_single_edge_px branch 5 times, most recently from 33188cb to 1100700 Compare March 26, 2024 23:05
@hellohuanlin hellohuanlin marked this pull request as ready for review March 26, 2024 23:46
@hellohuanlin hellohuanlin force-pushed the platform_view_intersection_ignore_single_edge_px branch from 1100700 to f31b39c Compare March 26, 2024 23:46
@hellohuanlin hellohuanlin force-pushed the platform_view_intersection_ignore_single_edge_px branch from f31b39c to 55566db Compare March 26, 2024 23:48
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Could we solve this problem a bit differently? Correct me if I'm wrong, but the "overlay" flutter view is sized exactly to the platform view right? Could we let it overhang the platform view by 1px to completely remove the border?

@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented Mar 27, 2024

Could we solve this problem a bit differently? Correct me if I'm wrong, but the "overlay" flutter view is sized exactly to the platform view right?

The overlay platform view is the size of the intersection (after rounding).

Could we let it overhang the platform view by 1px to completely remove the border?

That is already the previous behavior that this PR is trying to revert back to. For example, if the platform view's edge is at pixel 100.2, then the overlay overhangs the platform view by 0.8 pixel. That's why reverting the rounding in this PR completely removes the border here.

@jonahwilliams
Copy link
Contributor

Oh wait, is the screenshot you posted Before or After?

@flar
Copy link
Contributor

flar commented Mar 27, 2024

Ah, I see. The problem is that when doing the query, the region code clips the intersecting rectangles to the query rect.

We sort of want to query against roundIn, but intersect them against roundOut, but region doesn't provide that capability. So, the proposed fix is to just query against roundOut and then filter them after the fact?

This also points out that we have redundant intersections - first region intersects against the query rectangle, and then later the PV embedder code intersects them against the PV rectangle (which is also the query rectangle).

@flar
Copy link
Contributor

flar commented Mar 27, 2024

Looking through the code, there is so much waste in it all.

The act of computing the region is repeated for every single platform view. We literally create a region, grab a list of rectangles from it, then drop it on the floor, then on the next pass the same region is recreated only to be intersected with some other rectangle, then dropped on the floor. UPDATE: It looks like the DlRTree object caches the return value of the region() method.

On each of these repeated steps we make a full DlRegion of the entire DL, then we intersect it, then we re-band it, even if the intersection only touches a tiny portion of the entire region of the whole DL. If we reused the region, that wouldn't be so bad, but since we don't reuse it, we might as well query the rtree with the rect and only make a temp region from that subset of rectangles that are in the area of interest which would save a lot of region debanding operations and each region created would be much smaller. UPDATE: The intersection and rebanding may be a waste if there is only one PV since we will perform the Region setup code on many more rectangles than we need just to pare them down with the intersection. Even with a few PVs, if they don't have a lot of overlap then intersecting before creating the regions will reduce the overall work of creating any of the sub-regions.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

I feel like we are fighting the region/rtree system here and we need to rethink all of these APIs. Also, see my comment on the efficiency of the whole process - much wasted work going on here at a bunch of levels.

@hellohuanlin
Copy link
Contributor Author

Oh wait, is the screenshot you posted Before or After?

The screenshot was before the fix (with the border). This PR removes the border. I added the screenshot after the fix as well.

@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented Mar 27, 2024

@flar I created a separate issue to track the potential performance improvement around the double-for-loop: flutter/flutter#145802

I'd strongly prefer to keep that untouched for now, and have a separate PR for that improvement, so that we can measure it in the benchmark.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@hellohuanlin
Copy link
Contributor Author

i'm gonna land it before the cut to fix the issue, but feel free to leave more comment.

@hellohuanlin hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2024
@auto-submit auto-submit bot merged commit 01d42ad into flutter:main Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 27, 2024
…145841)

flutter/engine@00dab0d...b7dddee

2024-03-27 [email protected] Fail pre-submit if a negative image is encountered as part of `goldctl imgtest add`. (flutter/engine#51685)
2024-03-27 [email protected] [ios]ignore single edge pixel instead of rounding (flutter/engine#51687)

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 platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants