Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

[ASNetworkImageNode] cornerRadius property doesn't seem to work. #490

Closed
kusmi opened this issue Jun 19, 2015 · 21 comments
Closed

[ASNetworkImageNode] cornerRadius property doesn't seem to work. #490

kusmi opened this issue Jun 19, 2015 · 21 comments
Assignees
Milestone

Comments

@kusmi
Copy link

kusmi commented Jun 19, 2015

When setting the cornerRadius property, it is only applied to backgroundColor etc. but not for the actual image, e.g.

self.brandImageNode = [[ASNetworkImageNode alloc] init];
self.brandImageNode.backgroundColor = ASDisplayNodeDefaultPlaceholderColor();
self.brandImageNode.URL = [NSURL URLWithString:[NSString stringWithFormat:@"http://lorempixel.com/%zd/%zd/abstract/%zd",30,30,1]];
self.brandImageNode.cornerRadius = 15.0f;

will first show the background-color nicely "cornered", but when image loads, it will be square again.

workaround: also use an imageModificationBlock to add corners to image as well.

@appleguy
Copy link
Contributor

Thanks for the report!

Very important to note: the .cornerRadius property is one of the least efficient, most render-intensive properties on CALayer (alongside shadowPath, masking, etc). It performs the clipping operation on every frame — 60FPS during scrolling! — even if the content in that area isn't changing.

I will investigate this because it's quite surprising to me that it wouldn't be working (I suspect a property bridge issue, perhaps). It's definitely a bug and will be fixed, but I recommend you keep your workaround implementation & use similar implementations whenever it is possible to avoid cornerRadius.

In the case that the content both within and behind the rounded view can move / is dynamic, you must use cornerRadius, but accepting a design that requires this will usually mean performance is visibly degraded on iPhone 4, 4S, and 5 (along with comparable iPads / iPods).

@kusmi
Copy link
Author

kusmi commented Jun 24, 2015

And thank you for your suggestions. I was not aware of performance implications of cornerRadius ;-)

I will now use the defaultImage property and then the rounding is done with the imageModificationBlock property.

@appleguy
Copy link
Contributor

appleguy commented Jul 5, 2015

@kusmi Wow, I'm basically mystified, but you're indeed quite right about this bug. Not that I doubted you specifically, just that it is surprising to me that there is any way in which the CA cornerRadius property could be overridden...particularly because I tried directly setting it on the layer too, eliminating a problem with the property bridge.

What I said earlier is still true — cornerRadius is a disastrously expensive property that should only be used when there is no alternative. Fully redrawing the image to clip the corners is actually much faster because it happens once, rather than on every frame composite. So, I think you are unblocked / have a great solution (in that way, not really a workaround, because you wouldn't want to go back to this way regardless). That said, this is an issue I will definitely fix in an upcoming release when I have a little more time to dig in. For today, focusing on launching 1.2.2 and getting the Box Model layout patch out to the world in Beta form :).

@appleguy appleguy self-assigned this Jul 5, 2015
@appleguy appleguy added this to the 1.3 milestone Jul 5, 2015
@appleguy appleguy added the Bug label Jul 5, 2015
@markmark1
Copy link

Whats the right way to do rounded corners if border radius is bad
On Sun, Jul 5, 2015 at 12:27 PM appleguy notifications@github.com wrote:

@kusmi https://github.com/kusmi Wow, I'm basically mystified, but
you're indeed quite right about this bug. Not that I doubted you
specifically, just that it is surprising to me that there is any way in
which the CA cornerRadius property could be overridden...particularly
because I tried directly setting it on the layer too, eliminating a problem
with the property bridge.

What I said earlier is still true — cornerRadius is a disastrously
expensive property that should only be used when there is no alternative.
Fully redrawing the image to clip the corners is actually much faster
because it happens once, rather than on every frame composite. So, I think
you are unblocked / have a great solution (in that way, not really a
workaround, because you wouldn't want to go back to this way regardless).
That said, this is an issue I will definitely fix in an upcoming release
when I have a little more time to dig in. For today, focusing on launching
1.2.2 and getting the Box Model layout patch out to the world in Beta form
:).


Reply to this email directly or view it on GitHub
#490 (comment)
.

@markmark1
Copy link

So if border radius is bad how do we get rounded corners without it thanks
a lot

On Friday, June 19, 2015, Michael Kussmaul notifications@github.com wrote:

When setting the cornerRadius property, it is only applied to
backgroundColor etc. but not for the actual image, e.g.

self.brandImageNode = [[ASNetworkImageNode alloc] init];
self.brandImageNode.backgroundColor =
ASDisplayNodeDefaultPlaceholderColor();
self.brandImageNode.URL = [NSURL URLWithString:[NSString stringWithFormat:@
"http://lorempixel.com/%zd/%zd/abstract/%zd",30,30,1]];
self.brandImageNode.cornerRadius = 15.0f;

will first show the background-color nicely "cornered", but when image
loads, it will bi square again.

workaround: also use an imageModificationBlock to add corners to image as
well.


Reply to this email directly or view it on GitHub
#490.

@appleguy
Copy link
Contributor

appleguy commented Jul 7, 2015

@markmark1 there are different techniques, but performing some cropping of the image (and creating a new image that is cropped) is always preferred when possible. The only case you have to use .cornerRadius is when you have a complex hierarchy of elements that can have content moving both inside and outside the corner-clipped region, but if you have a UI like this, performance is going to be bad on many devices. At Facebook / Instagram we would often redesign UI specifically to avoid this, and the build system produced huge warnings in diff review for any usage of .cornerRadius, .shadowPath, etc.

For AsyncDisplayKit, there is something called the .imageModificationBlock. I think there may even be an included block type that does rounded corners (it would be reasonable to add one if there isn't yet). You can pretty easily create a UIBezierPath instance and call "clip" on it, then call "draw" on the UIImage in that graphics context. Let me know if you have any trouble!

@nickvelloff
Copy link
Contributor

This is an example fwiw. If you find a better way please let me know.

cardImageView?.imageModificationBlock = { [weak cardImageView] image in

            if image == nil {
                return image
            }


            var modifiedImage: UIImage?
            var rect = CGRect(origin: CGPointZero, size: image.size)

            UIGraphicsBeginImageContextWithOptions(image.size, false, UIScreen.mainScreen().scale)
            let maskPath = UIBezierPath(roundedRect: rect, byRoundingCorners: UIRectCorner.AllCorners, cornerRadii: CGSizeMake(10, 10))
            maskPath.addClip()
            image.drawInRect(rect)
            modifiedImage = UIGraphicsGetImageFromCurrentImageContext()
            UIGraphicsEndImageContext()
            return modifiedImage
        }

@markmark1
Copy link

thanks what about uibuttons and uiviews will this work to make rounded
corners

On Tuesday, July 7, 2015, Nick Velloff notifications@github.com wrote:

This is an example fwiw. If you find a better way please let me know.

cardImageView?.imageModificationBlock = { [weak cardImageView] image in

        if image == nil {
            return image
        }


        var modifiedImage: UIImage?
        var rect = CGRect(origin: CGPointZero, size: image.size)

        UIGraphicsBeginImageContextWithOptions(image.size, false, UIScreen.mainScreen().scale)
        let maskPath = UIBezierPath(roundedRect: rect, byRoundingCorners: UIRectCorner.AllCorners, cornerRadii: CGSizeMake(10, 10))
        maskPath.addClip()
        image.drawInRect(rect)
        modifiedImage = UIGraphicsGetImageFromCurrentImageContext()
        UIGraphicsEndImageContext()
        return modifiedImage
    }


Reply to this email directly or view it on GitHub
#490 (comment)
.

@markmark1
Copy link

Is there an objective c example

On Tue, Jul 7, 2015 at 6:26 PM, Nick Velloff notifications@github.com
wrote:

This is an example fwiw. If you find a better way please let me know.

cardImageView?.imageModificationBlock = { [weak cardImageView] image in

            if image == nil {
                return image
            }


            var modifiedImage: UIImage?
            var rect = CGRect(origin: CGPointZero, size: image.size)

            UIGraphicsBeginImageContextWithOptions(image.size, false, UIScreen.mainScreen().scale)
            let maskPath = UIBezierPath(roundedRect: rect, byRoundingCorners: UIRectCorner.AllCorners, cornerRadii: CGSizeMake(10, 10))
            maskPath.addClip()
            image.drawInRect(rect)
            modifiedImage = UIGraphicsGetImageFromCurrentImageContext()
            UIGraphicsEndImageContext()
            return modifiedImage
        }

Reply to this email directly or view it on GitHub:
#490 (comment)

@markmark1
Copy link

Is this better http://stackoverflow.com/questions/11613321/rounded-corners-on-a-view

On Tue, Jul 7, 2015 at 6:26 PM, Nick Velloff notifications@github.com
wrote:

This is an example fwiw. If you find a better way please let me know.

cardImageView?.imageModificationBlock = { [weak cardImageView] image in

            if image == nil {
                return image
            }


            var modifiedImage: UIImage?
            var rect = CGRect(origin: CGPointZero, size: image.size)

            UIGraphicsBeginImageContextWithOptions(image.size, false, UIScreen.mainScreen().scale)
            let maskPath = UIBezierPath(roundedRect: rect, byRoundingCorners: UIRectCorner.AllCorners, cornerRadii: CGSizeMake(10, 10))
            maskPath.addClip()
            image.drawInRect(rect)
            modifiedImage = UIGraphicsGetImageFromCurrentImageContext()
            UIGraphicsEndImageContext()
            return modifiedImage
        }

Reply to this email directly or view it on GitHub:
#490 (comment)

@nickvelloff
Copy link
Contributor

That is essentially the same solution without the off thread performance boost.

On Jul 7, 2015, at 9:14 PM, markmark1 notifications@github.com wrote:

Is this better http://stackoverflow.com/questions/11613321/rounded-corners-on-a-view

On Tue, Jul 7, 2015 at 6:26 PM, Nick Velloff notifications@github.com
wrote:

This is an example fwiw. If you find a better way please let me know.

cardImageView?.imageModificationBlock = { [weak cardImageView] image in

if image == nil {
return image
}


var modifiedImage: UIImage?
var rect = CGRect(origin: CGPointZero, size: image.size)

UIGraphicsBeginImageContextWithOptions(image.size, false, UIScreen.mainScreen().scale)
let maskPath = UIBezierPath(roundedRect: rect, byRoundingCorners: UIRectCorner.AllCorners, cornerRadii: CGSizeMake(10, 10))
maskPath.addClip()
image.drawInRect(rect)
modifiedImage = UIGraphicsGetImageFromCurrentImageContext()
UIGraphicsEndImageContext()
return modifiedImage
}

Reply to this email directly or view it on GitHub:
#490 (comment)

Reply to this email directly or view it on GitHub #490 (comment).

@markmark1
Copy link

what is the objective-c example with the off-the-thread perf boost

On Tue, Jul 7, 2015 at 9:53 PM, Nick Velloff notifications@github.com
wrote:

That is essentially the same solution without the off thread performance boost.

On Jul 7, 2015, at 9:14 PM, markmark1 notifications@github.com wrote:

Is this better http://stackoverflow.com/questions/11613321/rounded-corners-on-a-view

On Tue, Jul 7, 2015 at 6:26 PM, Nick Velloff notifications@github.com
wrote:

This is an example fwiw. If you find a better way please let me know.

cardImageView?.imageModificationBlock = { [weak cardImageView] image in

if image == nil {
return image
}


var modifiedImage: UIImage?
var rect = CGRect(origin: CGPointZero, size: image.size)

UIGraphicsBeginImageContextWithOptions(image.size, false, UIScreen.mainScreen().scale)
let maskPath = UIBezierPath(roundedRect: rect, byRoundingCorners: UIRectCorner.AllCorners, cornerRadii: CGSizeMake(10, 10))
maskPath.addClip()
image.drawInRect(rect)
modifiedImage = UIGraphicsGetImageFromCurrentImageContext()
UIGraphicsEndImageContext()
return modifiedImage
}

Reply to this email directly or view it on GitHub:
#490 (comment)

Reply to this email directly or view it on GitHub #490 (comment).


Reply to this email directly or view it on GitHub:
#490 (comment)

@nickvelloff
Copy link
Contributor

I do not have one as my project is Swift. I'm not part of the AsyncDisplayKit development team.

@markmark1
Copy link

Thanks

@appleguy appleguy modified the milestones: 2.1, 1.3 Dec 18, 2015
@appleguy appleguy changed the title cornerRadius property for ASNetworkImageNode does not work [ASNetworkImageNode] cornerRadius property doesn't seem to work. Dec 18, 2015
@macistador
Copy link

@markmark1 the example you linked is not the same as what is recommended here as it creates a mask layer to produce the rounded corners. This is also a time consuming operation as is therefore not recommended. It would be a better choice from a performance perspective to redraw your view using something similar to what @nickvelloff suggested. Note that it doesn't work well if your view should update in any way as it will be statically rendered.

@nickvelloff
Copy link
Contributor

@macistador I'm running into the statically rendered issue you noted when transitioning to a new custom UICollectionViewLayout. Is there another way to get around this issue?

@macistador
Copy link

@nickvelloff If you have a non-transparent & unicolor background behind your photos, a dirty trick would be to have some kind of transparent resizable image used as a mask; of course this is depend on your different layouts and may not work. For even more flexibility you can imagine a sort of 9-patch system

@nickvelloff
Copy link
Contributor

@macistador thanks. I was considering the 9 slice approach but had the issue of a gradient background. It may be that just going with a unicolor background or making another design tweak would make this possible.

@fatuhoku
Copy link

@appleguy Any updates on this one?

cornerRadius is sort of important — especially when you want a round border to go around the image as well. This cannot be accomplished via imageModificationBlock, can it?

@fatuhoku
Copy link

I found that setting _networkImageNode.clipsToBounds = YES; did crop the image! Not sure why I didn't try this before...

@hannahmbanana hannahmbanana removed this from the 2.1 milestone Jan 31, 2017
@hannahmbanana hannahmbanana modified the milestones: Temporary, 2.1 Jan 31, 2017
@nguyenhuy
Copy link
Contributor

Tried cornerRadius in ASDKgram, master branch. It works after setting clipsToBounds = YES. This is expected behavior of UIKit. Closing...

fruitcoder pushed a commit to digitalegarage/AsyncDisplayKit that referenced this issue Sep 6, 2017
…t transition is nil (facebookarchive#490)

* Avoid calling didComplete method if pending layout transition is nil

* Update CHANGELOG
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants