diegolavalledev / swiftuilib-wrap-stack

MIT License
11 stars 15 forks source link

WHStack too slow for ~60 items #2

Closed ammulder closed 3 years ago

ammulder commented 3 years ago

I tried to add a ScrollView with an WHStack with 60 buttons, and it takes 5-10 seconds to lay out. Is there any way to make this perform well?

                ScrollView {
                    WHStack {
                        arrayOf60Strings.map({ entry in
                            Button(action: {
                                print("Tapped \(entry)")
                            }) {
                                Text(entry).modifier(FloatingButtonText())
                            }
                        })
                    }
                }.border(Color.gray, width: 2)

struct FloatingButtonText: ViewModifier {
    func body(content: Content) -> some View {
        content
            .fixedSize()
            .padding([.horizontal], 6)
            .padding([.vertical], 3)
            .foregroundColor(.black)
            .overlay(RoundedRectangle(cornerRadius: 5).stroke(Color.blue, lineWidth: 2))
            .padding([.horizontal], 3)
    }
}
diegolavalledev commented 3 years ago

Thank you Aaron and sorry it took me this long to reply. You are absolutely right about the poor performance, my guess is that it has something to do with the way we calculate the intrinsic size of each of the items in the stack. Right now I'm using kind of a dirty trick that involves wrapping the element in a UIHostingController like so:

UIHostingController(rootView: item).view.intrinsicContentSize

It might not be the only bottleneck but I'm highly suspicious of it. It might just be possible to achieve the same result using a combination of GeometryReader and PreferenceKey instances. I will explore this alternative in the near future, hoping it doesn't imply a huge refactor.

You're most welcome to send over any potential optimisations as well if you can think of any. Thanks again for the heads up and have a great start of the new year. Cheers!

leobenini commented 3 years ago

I'm interested too! Thank you

marcusglowe commented 3 years ago

@diegolavalledev without refactoring, one simple change that you can make it to compute WrapStack.metrics once rather than as a computed variable. During initialization should be fine.

I believe it's recomputed 3 times per render which is what makes this much slower than necessary.

~Sample code from my local fork (I don't need columns, so hard to PR directly from this):

    var metrics: (Int, [Int])
    init(
        width: CGFloat,
        verticalAlignment: VerticalAlignment,
        spacing: CGFloat,
        content: [Content]
    ) {
        self.width = width
        self.verticalAlignment = verticalAlignment
        self.spacing = spacing
        self.content = content
        let (lanes, limits, _, _) =
            content.reduce((0, [], 0, width)) {
                (accum, item) -> (Int, [Int], Int, CGFloat) in
                var (lanesSoFar, limits, index, laneLength) = accum
                let itemSize = UIHostingController(rootView: item).view.intrinsicContentSize
                let itemLength = itemSize.width
                if laneLength + itemLength > width {
                    lanesSoFar += 1
                    laneLength = itemLength
                    limits.append(index)
                } else {
                    laneLength += itemLength + spacing
                }
                index += 1
                return (lanesSoFar, limits, index, laneLength)
            }
        metrics = (lanes, limits)
    }
diegolavalledev commented 3 years ago

Thanks @marcusglowe I implemented your solution for both horizontal and vertical wrap stacks. It's all on this commit https://github.com/diegolavalledev/swiftuilib-wrap-stack/commit/68ebbc743cf486640761d9689936bea65341c2a4

Basically just calculating everything in the initializer as a constant. I tested it and it's much faster. @ammulder @leobenini give it a try if you'd like. Release is https://github.com/swiftuilib/wrap-stack/releases/tag/1.0.1

Thanks!

graycreate commented 2 years ago

The performance is still too slow, and I have tested, it shows this line let itemSize = UIHostingController(rootView: item).view.intrinsicContentSize of code will cost a lot of time. And it's no need to create the UIHostingController for every item, a little update:

image

This update could improve performance by four times.

But still if you place this wrap stack into a scrollview, and you could see the scroll is not smooth, it seems when you scroll the calculation will persist happening again and again.

It would be great if we could calculate the sizes lazily (only calculate the will show item)

diegolavalledev commented 2 years ago

Thank you that's a great improvement! I will take a look at using GeometryReader for measuring instead of .intrinsicContentSize

ammulder commented 2 years ago

For what it's worth, I ended up rolling my own where I used the intrinsicContentSize but only (lazily) calculated it once for each item and cached it -- in my case it was a filterable list of buttons from a nearly-fixed set of options -- for practical purposes the list could not exceed about a hundred items so it wasn't such a big deal to cache the sizes forever. There were also a couple other things that tied it to my particular use, so it's not really a generic solution.

I would be happy to use a more performant version of WHStack next time. :)