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

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Sep 9, 2022

Description

Adding clamping to RRect radii when deflating, and asserting that supplied radii aren't negative, since having negative radii was causing some rendering artifacts.

This is the right place to do it, rather than higher in the stack, (e.g. as was necessary in flutter/flutter#106849).

Also, added a reference to math.dart in the dart_ui.gni file that was missed in #36106

Related PRs:

Related Issues

Tests

  • Added tests for inflate/deflate and the constructor assertions.

@gspencergoog gspencergoog force-pushed the clamp_rrect_radii branch 3 times, most recently from 8c3c2db to fe04609 Compare September 12, 2022 16:29
@gspencergoog gspencergoog changed the title Clamp RRect radii when deflating Clamp RRect radii when deflating, assert on negative radii Sep 12, 2022
@gspencergoog gspencergoog force-pushed the clamp_rrect_radii branch 2 times, most recently from fe09c32 to cce13a5 Compare September 12, 2022 23:15
@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 8.
View them at https://flutter-engine-gold.skia.org/cl/github/36062

@gspencergoog gspencergoog marked this pull request as ready for review September 13, 2022 21:29
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Sep 13, 2022
"//flutter/lib/ui/isolate_name_server.dart",
"//flutter/lib/ui/key.dart",
"//flutter/lib/ui/lerp.dart",
"//flutter/lib/ui/math.dart",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was added in #36106, but I forgot to add it here. To be honest, I have no idea why it didn't blow up the framework build, but it didn't. Anyhow, adding it in now so that we can finish moving clampDouble into dart:ui.

@gspencergoog gspencergoog requested review from goderbauer and removed request for yjbanov September 14, 2022 20:31
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

assert(blRadiusX != null),
assert(blRadiusY != null);
assert(blRadiusY != null),
assert(tlRadiusX >= 0),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention this in the docs for the public constructors that feed into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 15, 2022
@auto-submit auto-submit bot merged commit 0dceaab into flutter:main Sep 15, 2022
@gspencergoog gspencergoog deleted the clamp_rrect_radii branch September 15, 2022 01:03
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 15, 2022
itsjustkevin pushed a commit to itsjustkevin/engine that referenced this pull request Sep 21, 2022
itsjustkevin added a commit that referenced this pull request Sep 21, 2022
* Add `Radius.clamp` and `Radius.clampValues` (#36106)

* Build CanvasKit in the Flutter Engine (#32510)

* Clamp `RRect` radii when deflating, assert on negative radii (#36062)

Co-authored-by: Greg Spencer <[email protected]>
Co-authored-by: Harry Terkelsen <[email protected]>
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
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-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants