[ASCornerRounding] Introduce .cornerRoundingType: CALayer, Precomposited, or Clip Corners. #465

appleguy · 2017-07-23T06:59:01Z

For at least two years, we've gotten requests from the community to support alternatives to CALayer's .cornerRadius. Even on today's devices, this property still has a performance cost (including on early 64-bit SoCs, like the A7 and A8). In the cases where many views need to be rounded and exist on screens with movement, the overhead can become very significant.

This new ASCornerRounding property supports all of the modes described on the Texture website, with a minimal change in our API. This is achieved by using the .backgroundColor property to influence the mode where appropriate. In the future, I'll improve the logic to be able to detect certain undesired conditions, like non-opaque background with ClipCorner type.

http://texturegroup.org/docs/corner-rounding.html

This has already been tested with ASDKgram, including code that automatically advances through each of the corner rounding modes to ensure the transitions between them are smooth.

It would be great to get a review and work towards an accept soon, but before landing this, I'll add snapshot tests and look for opportunities to improve documentation. Suggestions welcome!

It is known that there is a subtle difference in rounding between the arc-based clip corners, and the iOS 7-style UIBezierPath precomposited corners. This is described here: https://www.paintcodeapp.com/blogpost/code-for-ios-7-rounded-rectangles . The difference is not noticeable for most use cases (and of these modes, Precomposited will be used the most, + already uses the newer style). Thus I feel confident revisiting this later for ClipCorner type.

Very brief search for some folks requesting guidance / samples / support for this:
facebookarchive/AsyncDisplayKit#1446
facebookarchive/AsyncDisplayKit#979

…ing features.

…ited corners.

…ding features.

Adlai-Holler

Wow this is one of my favorite diffs of all time! Great work, we'll be putting this to use in Pinterest in short order.

Adlai-Holler · 2017-07-24T18:11:52Z

Source/Private/ASDisplayNode+AsyncDisplay.mm

+}
+- (void)__didDisplayNodeContentWithRenderingContext:(CGContextRef)context image:(UIImage **)image drawParameters:(id _Nullable)drawParameters backgroundColor:(UIColor *)backgroundColor borderWidth:(CGFloat)borderWidth borderColor:(CGColorRef)borderColor
+{
+  if (context == NULL && *image == NULL) {


Not sure whether you're looking for && image == NULL or && *image == nil here.

Adlai-Holler · 2017-07-24T18:15:25Z

Source/Private/ASDisplayNode+AsyncDisplay.mm

+    if (*image) {
+      *image = UIGraphicsGetImageFromCurrentImageContext();
+      UIGraphicsEndImageContext();
+    }


Should this be if (image)? And should we ensure that we only end the image context if we created one above, and not if we were passed one? The context create-readimage-destroy behavior here seems a little unclear, for instance in the caller we will actually re-generate the image and overwrite it if the caller passed us a context.

Adlai-Holler · 2017-07-24T18:16:36Z

Source/Private/ASDisplayNodeInternal.h

@@ -319,4 +326,10 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo

 @end

+@interface ASDisplayNode (InternalPropertyBridge)
+
+@property (nonatomic, assign) CGFloat layerCornerRadius;


May be worth dropping a comment on this property explaining its distinction from .cornerRadius? Seems obvious to us today but who knows tomorrow

appleguy · 2017-08-20T08:42:41Z

@Adlai-Holler Thanks for the comments! I'll try to update this soon, although under a bit of a squeeze for time (not uncommon, just sayin' it like it is! :) ).

I'll certainly address your comments before landing. Anything else you'd be looking for prior to an accept?

Adlai-Holler · 2017-08-21T17:54:26Z

@appleguy Nope! I've spoken now, so I'll forever hold my peace 💞

muukii · 2017-09-06T17:36:34Z

Source/ASDisplayNode.mm

+      for (int idx = 0; idx < 4; idx++) {
+        if (_clipCornerLayers[idx] == nil) {
+          _clipCornerLayers[idx] = [[CALayer alloc] init];
+          _clipCornerLayers[idx].zPosition = 99999;


I think following code will be better performance.
It reduce accessing to array.

CALayer *layer = [[CALayer alloc] init]; layer.zPosition = 99999; layer.delegate = self; _clipCornerLayers[idx] = layer;

@appleguy
Thanks! I understand.
Sorry, I may be misunderstanding about retain and release.
Does returning by subscript of NSArray not create a retain?

appleguy · 2017-09-06T17:41:01Z

Thanks for the suggestion! I think this will create a retain and release call. If so, this is actually much more expensive than the single memory indirection / pointer lookup. Note that this is a C array for performance, not an NSArray.

…

On Wed, Sep 6, 2017 at 10:36 AM Hiroshi Kimura ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Source/ASDisplayNode.mm <#465 (comment)>: > + + UIGraphicsEndImageContext(); + } + [self _layoutClipCornersIfNeeded]; + }); +} + +- (void)_setClipCornerLayersVisible:(BOOL)visible +{ + ASPerformBlockOnMainThread(^{ + ASDisplayNodeAssertMainThread(); + if (visible) { + for (int idx = 0; idx < 4; idx++) { + if (_clipCornerLayers[idx] == nil) { + _clipCornerLayers[idx] = [[CALayer alloc] init]; + _clipCornerLayers[idx].zPosition = 99999; I think following code will be better performance. It reduce accessing to array. CALayer *layer = [[CALayer alloc] init]; layer.zPosition = 99999; layer.delegate = self; _clipCornerLayers[idx] = layer; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#465 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAigA7Fo3cPGrmAWJ31SeoBjcaA63zNlks5sftgkgaJpZM4OgYHw> .

appleguy · 2017-09-09T02:57:52Z

I don’t have the code in front of me, but is the array an NSArray? In an earlier version when I first wrote this, I was pretty sure it was a C array, but maybe I changed it before landing. If it is an NSArray, it will be retained and autoreleased; if a C array, it won’t be retained at all. The additional overhead comes from the variable reference. It will add another retain, and release call in addition to the retain + autorelease from the array access. Although this isn’t a severe issue, I think the code is pretty acceptable as it is. However, we welcome any other suggestions or PRs, even just as cleanup / refactoring!!

…

On Sep 6, 2017, at 10:48 AM, Hiroshi Kimura ***@***.***> wrote: @muukii commented on this pull request. In Source/ASDisplayNode.mm <#465 (comment)>: > + + UIGraphicsEndImageContext(); + } + [self _layoutClipCornersIfNeeded]; + }); +} + +- (void)_setClipCornerLayersVisible:(BOOL)visible +{ + ASPerformBlockOnMainThread(^{ + ASDisplayNodeAssertMainThread(); + if (visible) { + for (int idx = 0; idx < 4; idx++) { + if (_clipCornerLayers[idx] == nil) { + _clipCornerLayers[idx] = [[CALayer alloc] init]; + _clipCornerLayers[idx].zPosition = 99999; @appleguy <https://github.com/appleguy> Thanks! I understand. Sorry, I may be misunderstanding about retain and release. Does returning by subscript of NSArray not create a retain? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#465 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAigA3t5ss25Tc_53yGrBKDUl8Z4tJoeks5sftrdgaJpZM4OgYHw>.

appleguy · 2017-09-18T05:47:11Z

@nguyenhuy @maicki @Adlai-Holler could I get an accept? Otherwise it won't let me land :(. Feel free to merge this at the same time, too!

There are some good further improvements that could be made to this, but it is useful in its current state, and I won't have time to invest in an example project or other upgrades in the near future.

nguyenhuy

Accepting per @Adlai-Holler's comments.

…ted, or Clip Corners. (TextureGroup#465) * [ASCornerRounding] Initial (untested) implementation of ASCornerRounding features. * [ASCornerRounding] Working version of both clip corners and precomposited corners. * [ASCornerRounding] Improve factoring and documentation of corner rounding features. * [ASCornerRounding] Some final fixups. * Add entry to changelog for .cornerRoundingType

bolsinga · 2019-03-15T22:48:09Z

Source/Private/ASDisplayNodeInternal.h

+
+  CGFloat _cornerRadius;
+  ASCornerRoundingType _cornerRoundingType;
+  CALayer *_clipCornerLayers[4];


Hi @appleguy Scott! How are these ObjC objects retained in this C array? Is there some ARC magic I'm clearly not aware of? -- Greg

appleguy added 5 commits June 14, 2017 20:54

[ASCornerRounding] Initial (untested) implementation of ASCornerRound…

0f18715

…ing features.

Merge branch 'master' into ASCornerRounding

86238ed

[ASCornerRounding] Working version of both clip corners and precompos…

704c8e6

…ited corners.

Merge branch 'master' into ASCornerRounding

646ff7d

[ASCornerRounding] Improve factoring and documentation of corner roun…

94c8a42

…ding features.

appleguy self-assigned this Jul 23, 2017

appleguy requested review from maicki, garrettmoon, nguyenhuy and Adlai-Holler July 23, 2017 06:59

[ASCornerRounding] Some final fixups.

273025d

appleguy force-pushed the ASCornerRounding branch from ca1f7e4 to 273025d Compare July 23, 2017 07:07

Add entry to changelog for .cornerRoundingType

82eaa1c

Adlai-Holler reviewed Jul 24, 2017

View reviewed changes

Merge branch 'master' into ASCornerRounding

c102dd2

muukii reviewed Sep 6, 2017

View reviewed changes

Merge branch 'master' into ASCornerRounding

cd62d4f

nguyenhuy approved these changes Sep 18, 2017

View reviewed changes

nguyenhuy merged commit e330e57 into master Sep 18, 2017

nguyenhuy mentioned this pull request Jan 16, 2018

[ASDisplayNode] Always return the thread-safe cornerRadius property, even in slow CALayer rounding mode #749

Merged

bolsinga reviewed Mar 15, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCornerRounding] Introduce .cornerRoundingType: CALayer, Precomposited, or Clip Corners. #465

[ASCornerRounding] Introduce .cornerRoundingType: CALayer, Precomposited, or Clip Corners. #465

appleguy commented Jul 23, 2017 •

edited

Loading

Adlai-Holler left a comment

Adlai-Holler Jul 24, 2017

Adlai-Holler Jul 24, 2017

Adlai-Holler Jul 24, 2017

appleguy commented Aug 20, 2017

Adlai-Holler commented Aug 21, 2017

muukii Sep 6, 2017

muukii Sep 6, 2017

appleguy commented Sep 6, 2017 via email

appleguy commented Sep 9, 2017 via email

appleguy commented Sep 18, 2017 •

edited

Loading

nguyenhuy left a comment

bolsinga Mar 15, 2019 •

edited

Loading

[ASCornerRounding] Introduce .cornerRoundingType: CALayer, Precomposited, or Clip Corners. #465

[ASCornerRounding] Introduce .cornerRoundingType: CALayer, Precomposited, or Clip Corners. #465

Conversation

appleguy commented Jul 23, 2017 • edited Loading

Adlai-Holler left a comment

Choose a reason for hiding this comment

Adlai-Holler Jul 24, 2017

Choose a reason for hiding this comment

Adlai-Holler Jul 24, 2017

Choose a reason for hiding this comment

Adlai-Holler Jul 24, 2017

Choose a reason for hiding this comment

appleguy commented Aug 20, 2017

Adlai-Holler commented Aug 21, 2017

muukii Sep 6, 2017

Choose a reason for hiding this comment

muukii Sep 6, 2017

Choose a reason for hiding this comment

appleguy commented Sep 6, 2017 via email

appleguy commented Sep 9, 2017 via email

appleguy commented Sep 18, 2017 • edited Loading

nguyenhuy left a comment

Choose a reason for hiding this comment

bolsinga Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

appleguy commented Jul 23, 2017 •

edited

Loading

appleguy commented Sep 18, 2017 •

edited

Loading

bolsinga Mar 15, 2019 •

edited

Loading