flutter_animate icon indicating copy to clipboard operation
flutter_animate copied to clipboard

Optionally wrap `Animate` with `RepaintBoundary`

Open S-ecki opened this issue 2 years ago • 6 comments

Reasoning

As discussed in this talk about flutter_animate, the performance of animations might get significantly improved by wrapping them in a RepaintBoundary. I argue that this is not apparent to everyone. Especially newer Flutter developers might not be aware of this performance improvement opportunity.

Proposal

Add an optional bool? useRepaintBoundary to the Animate() widget. If set true, the animation should be automatically wrapped with a RepaintBoundary. The parameter should be false by default, as RepaintBoundaries also come with a cost and the usage should be transparent.

It would be vital to also add extensive explanation in the documentation of the parameter about when to use RepaintBoundary and when not to, optimally with examples. This way, people not knowing about the widget would get educated and situations where RepaintBoundary is applied suboptimally get minimized (although not eliminated!).

If the addition of such a parameter comes with too high of a risk of missusage, I propose to at least add a section to the ReadMe/Docs of this package explaining the potential benefits and drawbacks of using a RepaintBoundary in combination with animations.

Additional Context

This matter was discussed briefly in this tweet with the author of the package.

S-ecki avatar Jan 27 '23 20:01 S-ecki

I would love to see this proposal accepted and implemented. I think this addition would align perfectly with the already existing mindset of Flutter, that is, to make it as easy to write performant code as possible.

peter-gy avatar Jan 28 '23 18:01 peter-gy

@gskinner I created a small draft PR and decided not to include the parameter in AnimateList for now as I think it the RepaintBoundary is more likely to have negative effects there. But do you think about that? Should I add it to have a uniform API across the animate widgets?

S-ecki avatar Jan 28 '23 18:01 S-ecki

Thanks @S-ecki !

I think that's a good call, if you have a list of animated items, it's usually not going to be a good candidate for wrapping each item in RepaintBoundary. I'm coding on some other stuff right now, but will aim to look over the PR in the next day or so.

gskinner avatar Jan 28 '23 18:01 gskinner

just a note that this is still on list to fully review, just working through a few other items first.

gskinner avatar Feb 07 '23 19:02 gskinner

Looked this over, and it looks good, but I want to make sure there's a good answer for one simple question before including this: "why?"

What is the ultimate goal for this feature? It's not any shorter or easier than just wrapping in a widget:

foo.animate(useRepaintBoundary: true)
// vs
RepaintBoundary(child: foo.animate())

I have an idea or two of why it might be justified, but I'd be interested to hear other thoughts on this (versus, for example, just adding a note in the README about RepaintBoundary).

Sorry for not asking this sooner, it just occurred to me while reviewing the PR tonight. :)

gskinner avatar Feb 10 '23 06:02 gskinner

quick note @gskinner, I'm in Australia backpacking rn, will answer as soon as I'm back :)

S-ecki avatar Feb 23 '23 06:02 S-ecki