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

Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas #12229

Merged
merged 4 commits into from
Sep 17, 2019
Merged

Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas #12229

merged 4 commits into from
Sep 17, 2019

Conversation

ditman
Copy link
Member

@ditman ditman commented Sep 11, 2019

And use it to renderDRRect in web.

Fixes flutter/flutter#37772 (and others)

@ditman ditman changed the title Add check to determine if a rrect is fully contained within another. Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas Sep 13, 2019
yjbanov
yjbanov reviewed Sep 16, 2019
View reviewed changes
lib/web_ui/lib/src/engine/recording_canvas.dart Outdated
&& outer.contains(ui.Offset(scaled.right, scaled.bottom - scaled.brRadiusY))
&& outer.contains(ui.Offset(scaled.right - scaled.brRadiusX, scaled.bottom)) // Right-Bottom
&& outer.contains(ui.Offset(scaled.left + scaled.blRadiusX, scaled.bottom))
&& outer.contains(ui.Offset(scaled.left, scaled.bottom - scaled.blRadiusY)))) { // Left-Bottom
Copy link
Contributor

@yjbanov yjbanov Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In the happy case when the drrect is properly specified, this check will allocate 8 ui.Offset objects. Is it possible to optimize that away?

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @rakudrama Am I being too paranoid here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try with a different check, but I'm not sure it'll be geometrically precise, let me give that a quick shot, I can always come back to this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean rewrite the check. I mean do exactly the same math without allocation. Or is there something that prevents you from doing that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The API of contains is what it is, and once the radius information is used to construct the RRect, everything is stored as doubles, so I don't see any way of computing the offsets of each of the points of the corner without allocating an Offset. Also Offsets seem to be immutable, so even if I try to move it around, it'll return a new instance all the time, right?

Anyway, I wrote a slightly different revision that does the same with a little bit less allocation (but in a slightly less compact code).

@ditman ditman closed this Sep 17, 2019
@ditman
Copy link
Member Author

ditman commented Sep 17, 2019

Closing and reopening to see if Cirrus tests restart.

@ditman ditman reopened this Sep 17, 2019
@ditman ditman merged commit 851f460 into flutter:master Sep 17, 2019
@ditman ditman deleted the issues/37772 branch September 17, 2019 20:27
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 17, 2019
@engine-flutter-autoroll
851f460 Improve check to render (or not) a DRRect when inner falls ou…
c89ef37 
…tside of outer on RecordingCanvas (flutter/engine#12229)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 17, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 18, 2019
@engine-flutter-autoroll
Roll engine 63873d9..d1692d4 (22 commits) (#40717)
00d50be 
[email protected]:flutter/engine.git/compare/63873d9f421f...d1692d4

git log 63873d9..d1692d4 --no-merges --oneline
2019-09-17 [email protected] Update canvaskit backend (flutter/engine#12318)
2019-09-17 [email protected] README for the felt tool (flutter/engine#12323)
2019-09-17 [email protected] Fix continuous event polling in the GLFW event loop (flutter/engine#12320)
2019-09-17 [email protected] Tests for #11283 (flutter/engine#12322)
2019-09-17 [email protected] Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229)
2019-09-17 [email protected] Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits)
2019-09-17 [email protected] Channel buffers (flutter/engine#12167)
2019-09-17 [email protected] Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128)
2019-09-17 [email protected] Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287)
2019-09-17 [email protected] Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319)
2019-09-17 [email protected] Add a build command to felt (flutter/engine#12303)
2019-09-17 [email protected] Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316)
2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315)
2019-09-17 [email protected] Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313)
2019-09-17 [email protected] Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312)
2019-09-16 [email protected] Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306)
2019-09-16 [email protected] Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277)
2019-09-16 [email protected] Cleanup in web_ui (flutter/engine#12307)
2019-09-16 [email protected] Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182)
2019-09-16 [email protected] Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304)
2019-09-16 [email protected] Include firefox in check to quote font families (flutter/engine#12288)
2019-09-16 [email protected] Roll src/third_party/dart 7799f424f4..dd1969a43a (2 commits)


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] on the revert to ensure that a human
is aware of the problem.

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/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 63873d9..d1692d4 (22 commits) (flutter#40717)
85706af 
[email protected]:flutter/engine.git/compare/63873d9f421f...d1692d4

git log 63873d9..d1692d4 --no-merges --oneline
2019-09-17 [email protected] Update canvaskit backend (flutter/engine#12318)
2019-09-17 [email protected] README for the felt tool (flutter/engine#12323)
2019-09-17 [email protected] Fix continuous event polling in the GLFW event loop (flutter/engine#12320)
2019-09-17 [email protected] Tests for flutter#11283 (flutter/engine#12322)
2019-09-17 [email protected] Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229)
2019-09-17 [email protected] Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits)
2019-09-17 [email protected] Channel buffers (flutter/engine#12167)
2019-09-17 [email protected] Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128)
2019-09-17 [email protected] Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287)
2019-09-17 [email protected] Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319)
2019-09-17 [email protected] Add a build command to felt (flutter/engine#12303)
2019-09-17 [email protected] Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316)
2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315)
2019-09-17 [email protected] Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313)
2019-09-17 [email protected] Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312)
2019-09-16 [email protected] Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306)
2019-09-16 [email protected] Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277)
2019-09-16 [email protected] Cleanup in web_ui (flutter/engine#12307)
2019-09-16 [email protected] Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182)
2019-09-16 [email protected] Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304)
2019-09-16 [email protected] Include firefox in check to quote font families (flutter/engine#12288)
2019-09-16 [email protected] Roll src/third_party/dart 7799f424f4..dd1969a43a (2 commits)


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] on the revert to ensure that a human
is aware of the problem.

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/+/master/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonahwilliams jonahwilliams jonahwilliams left review comments

@yjbanov yjbanov yjbanov approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[web]: BoxDecoration border doesn't render if borderRadius is set
4 participants
@ditman @yjbanov @jonahwilliams @googlebot