Set default clipBehavior to Clip.none and update tests #20205
Conversation
There was a problem hiding this comment.
all the constructors where you add clipBehavior should assert that it's not null, and should mention in their documentation that it must not be null.
There was a problem hiding this comment.
we should probably export Clip from painting/basic_types.dart
There was a problem hiding this comment.
why do you define this as a closure instead of a function?
There was a problem hiding this comment.
Defined as a function now. I didn't realize that I can define function inside a function. Thanks!
There was a problem hiding this comment.
i don't think this is accurate. If it is, then we need to find a solution. People don't want or expect bleeding in these cases.
There was a problem hiding this comment.
According to our discussion offline, change rotation back to 1.0 radians, and comment that there should only be bleeding edges on rect edges (but not round corners).
There was a problem hiding this comment.
the root render object is always a RenderView, and in any case none of the classes you list there are render objects, so I'm not sure this documentation is what you mean?
There was a problem hiding this comment.
Changed to [RenderClipXXX].
I copied the comment from the next class which also says
Asserts that a [Finder] locates a single object whose root RenderObject is a [RenderClipRRect]...
I guess that we can find something in the middle using a globalKey?
There was a problem hiding this comment.
Oops, this is a typo. Fixed.
There was a problem hiding this comment.
Defined as a function now. I didn't realize that I can define function inside a function. Thanks!
There was a problem hiding this comment.
Changed to [RenderClipXXX].
I copied the comment from the next class which also says
Asserts that a [Finder] locates a single object whose root RenderObject is a [RenderClipRRect]...
I guess that we can find something in the middle using a globalKey?
There was a problem hiding this comment.
According to our discussion offline, change rotation back to 1.0 radians, and comment that there should only be bleeding edges on rect edges (but not round corners).
liyuqian
force-pushed
the
noclip
branch
2 times, most recently
from
cfd8130 to
8513b5b
Compare
|
@Hixie ping... I just did a performance measurement locally and there should be 5% improvement on average frame time and 10-20% improvement on 99th percentile average frame time for flutter_gallery__transition. |
|
|
|
@liyuqian this is too old to revert and it is blocking the flutter roll - is the fix easy or are you willing to do the rollback yourself? |
|
@jonahwilliams You mean Google3 roll? Can please you send me the link to the failed Google3 roll so I can check how to fix it? |
|
This seems to have caused a regression in the tiles_scroll_perf__timeline_summary benchmark (average_frame_rasterizer_time_millis). See https://flutter-dashboard.appspot.com/benchmarks.html |
|
I guess this change has been reverted, the perf regression is now gone. |
|
@a-siva : the regression is caused by our over-simplistic raster cache heuristics: the raster cache thought that the scroll item is too simple to cache because we removed a clip… We should probably improve our raster cache strategy (meanwhile, there's also discussion that we should abandon raster cache completely; it will hurt our average frame time and battery usage, but probably not bad enough to cause a jank in this case @chinmaygarde @Hixie ) |
5 participants
No description provided.