-
Notifications
You must be signed in to change notification settings - Fork 29.9k
[cupertino] improve cupertino picker performance by using at most one opacity layer #124719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
02692b6
25711db
06eb67a
47a840b
3db1b0d
721fe24
dd5d163
697a4db
e8ae26b
3215f96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -828,22 +828,42 @@ class RenderListWheelViewport | |
|
|
||
| /// Paints all children visible in the current viewport. | ||
| void _paintVisibleChildren(PaintingContext context, Offset offset) { | ||
| // Note that the magnifier cannot be turned off if the opacity is less than | ||
| // 1.0. | ||
| if (overAndUnderCenterOpacity >= 1) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also go down this code path if useMagnifier is false? Seems like in that case the other code path would paint some children twice since _paintTransformedChild ignores the value of center in that case?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see line 907
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the magnifier cannot be disabled if there is opacity.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that enforced anywhere in the API that This is confusing, but I guess not a problem of this PR...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the value of use magnifier is just ignored |
||
| _paintAllChildren(context, offset); | ||
| return; | ||
| } | ||
|
|
||
| // In order to reduce the number of opacity layers, we first paint all | ||
| // partially opaque children, then finally paint the fully opaque children. | ||
| context.pushOpacity(offset, (overAndUnderCenterOpacity * 255).round(), (PaintingContext context, Offset offset) { | ||
| _paintAllChildren(context, offset, center: false); | ||
| }); | ||
| _paintAllChildren(context, offset, center: true); | ||
| } | ||
|
|
||
| void _paintAllChildren(PaintingContext context, Offset offset, { bool? center }) { | ||
| RenderBox? childToPaint = firstChild; | ||
| while (childToPaint != null) { | ||
| final ListWheelParentData childParentData = childToPaint.parentData! as ListWheelParentData; | ||
| _paintTransformedChild(childToPaint, context, offset, childParentData.offset); | ||
| _paintTransformedChild(childToPaint, context, offset, childParentData.offset, center: center); | ||
| childToPaint = childAfter(childToPaint); | ||
| } | ||
| } | ||
|
|
||
| /// Takes in a child with a **scrollable layout offset** and paints it in the | ||
| /// **transformed cylindrical space viewport painting coordinates**. | ||
| // Takes in a child with a **scrollable layout offset** and paints it in the | ||
| // **transformed cylindrical space viewport painting coordinates**. | ||
| // | ||
| // The value of `center` is passed through to _paintChildWithMagnifier only | ||
| // if the magnifier is enabled and/or opacity is < 1.0. | ||
| void _paintTransformedChild( | ||
| RenderBox child, | ||
| PaintingContext context, | ||
| Offset offset, | ||
| Offset layoutOffset, | ||
| ) { | ||
| Offset layoutOffset, { | ||
| required bool? center, | ||
| }) { | ||
| final Offset untransformedPaintingCoordinates = offset | ||
| + Offset( | ||
| layoutOffset.dx, | ||
|
|
@@ -876,22 +896,34 @@ class RenderListWheelViewport | |
|
|
||
| final bool shouldApplyOffCenterDim = overAndUnderCenterOpacity < 1; | ||
| if (useMagnifier || shouldApplyOffCenterDim) { | ||
| _paintChildWithMagnifier(context, offset, child, transform, offsetToCenter, untransformedPaintingCoordinates); | ||
| _paintChildWithMagnifier(context, offset, child, transform, offsetToCenter, untransformedPaintingCoordinates, center: center); | ||
| } else { | ||
| _paintChildCylindrically(context, offset, child, transform, offsetToCenter); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we assert that center is null if we go down the else branch? Actually, the fact that this branch ignores center silently makes the code kinda hard to follow (and I think it is also the source of a bug in the current implementation, see my comment about useMagnifier above). Maybe we need to refactor this somehow to make it easier to follow.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What about this one? Since this is already pretty confusing it may be worthwhile to put a guardrail in place for when this gets refactored to make the caller instantly aware of the fact that their specification of center is not doing what they may think it is doing?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the assert. I'm not in love with this code but I don't see a nice way to re-arrange it all... |
||
| } | ||
| } | ||
|
|
||
| /// Paint child with the magnifier active - the child will be rendered | ||
| /// differently if it intersects with the magnifier. | ||
| // Paint child with the magnifier active - the child will be rendered | ||
| // differently if it intersects with the magnifier. | ||
| // | ||
| // `center` controls how items that partially intersect the center magnifier | ||
| // are rendered. If `center` is false, items are only painted cynlindrically. | ||
| // If `center` is true, only the clipped magnifier items are painted. | ||
| // If `center` is null, partially intersecting items are painted both as the | ||
| // magnifier and cynlidrical item, while non-intersecting items are painted | ||
| // only cylindrically. | ||
| // | ||
| // This property is used to lift the opacity that would be applied to each | ||
| // cylindrical item into a single layer, reducing the rendering cost of the | ||
| // pickers which use this viewport. | ||
| void _paintChildWithMagnifier( | ||
| PaintingContext context, | ||
| Offset offset, | ||
| RenderBox child, | ||
| Matrix4 cylindricalTransform, | ||
| Offset offsetToCenter, | ||
| Offset untransformedPaintingCoordinates, | ||
| ) { | ||
| Offset untransformedPaintingCoordinates, { | ||
| required bool? center, | ||
| }) { | ||
| final double magnifierTopLinePosition = | ||
| size.height / 2 - _itemExtent * _magnification / 2; | ||
| final double magnifierBottomLinePosition = | ||
|
|
@@ -902,27 +934,28 @@ class RenderListWheelViewport | |
| final bool isBeforeMagnifierBottomLine = untransformedPaintingCoordinates.dy | ||
| <= magnifierBottomLinePosition; | ||
|
|
||
| final Rect centerRect = Rect.fromLTWH( | ||
| 0.0, | ||
| magnifierTopLinePosition, | ||
| size.width, | ||
| _itemExtent * _magnification, | ||
| ); | ||
| final Rect topHalfRect = Rect.fromLTWH( | ||
| 0.0, | ||
| 0.0, | ||
| size.width, | ||
| magnifierTopLinePosition, | ||
| ); | ||
| final Rect bottomHalfRect = Rect.fromLTWH( | ||
| 0.0, | ||
| magnifierBottomLinePosition, | ||
| size.width, | ||
| magnifierTopLinePosition, | ||
| ); | ||
| // Some part of the child is in the center magnifier. | ||
| if (isAfterMagnifierTopLine && isBeforeMagnifierBottomLine) { | ||
| final Rect centerRect = Rect.fromLTWH( | ||
| 0.0, | ||
| magnifierTopLinePosition, | ||
| size.width, | ||
| _itemExtent * _magnification, | ||
| ); | ||
| final Rect topHalfRect = Rect.fromLTWH( | ||
| 0.0, | ||
| 0.0, | ||
| size.width, | ||
| magnifierTopLinePosition, | ||
| ); | ||
| final Rect bottomHalfRect = Rect.fromLTWH( | ||
| 0.0, | ||
| magnifierBottomLinePosition, | ||
| size.width, | ||
| magnifierTopLinePosition, | ||
| ); | ||
| final bool inCenter = isAfterMagnifierTopLine && isBeforeMagnifierBottomLine; | ||
|
|
||
| if ((center == null || center) && inCenter) { | ||
| // Clipping the part in the center. | ||
| context.pushClipRect( | ||
| needsCompositing, | ||
|
|
@@ -939,25 +972,29 @@ class RenderListWheelViewport | |
| ); | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| // Clipping the part in either the top-half or bottom-half of the wheel. | ||
| // Clipping the part in either the top-half or bottom-half of the wheel. | ||
| if ((center == null || !center) && inCenter) { | ||
| context.pushClipRect( | ||
| needsCompositing, | ||
| offset, | ||
| untransformedPaintingCoordinates.dy <= magnifierTopLinePosition | ||
| ? topHalfRect | ||
| : bottomHalfRect, | ||
| (PaintingContext context, Offset offset) { | ||
| _paintChildCylindrically( | ||
| context, | ||
| offset, | ||
| child, | ||
| cylindricalTransform, | ||
| offsetToCenter, | ||
| ); | ||
| _paintChildCylindrically( | ||
| context, | ||
| offset, | ||
| child, | ||
| cylindricalTransform, | ||
| offsetToCenter, | ||
| ); | ||
| }, | ||
| ); | ||
| } else { | ||
| } | ||
|
|
||
| if ((center == null || !center) && !inCenter) { | ||
| _paintChildCylindrically( | ||
| context, | ||
| offset, | ||
|
|
@@ -987,17 +1024,12 @@ class RenderListWheelViewport | |
| ); | ||
| } | ||
|
|
||
| // Paint child cylindrically, with [overAndUnderCenterOpacity]. | ||
| void opacityPainter(PaintingContext context, Offset offset) { | ||
| context.pushOpacity(offset, (overAndUnderCenterOpacity * 255).round(), painter); | ||
| } | ||
|
|
||
| context.pushTransform( | ||
| needsCompositing, | ||
| offset, | ||
| _centerOriginTransform(cylindricalTransform), | ||
| // Pre-transform painting function. | ||
| overAndUnderCenterOpacity == 1 ? painter : opacityPainter, | ||
| painter, | ||
| ); | ||
|
|
||
| final ListWheelParentData childParentData = child.parentData! as ListWheelParentData; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: per style guide just remove the "note that" at the beginning here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done