Skip to content
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

Optionally wrap Animate with RepaintBoundary #46

Open
S-ecki opened this issue Jan 27, 2023 · 7 comments · May be fixed by #47
Open

Optionally wrap Animate with RepaintBoundary #46

S-ecki opened this issue Jan 27, 2023 · 7 comments · May be fixed by #47
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@S-ecki
Copy link

S-ecki commented Jan 27, 2023

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.

@peter-gy
Copy link

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.

@gskinner gskinner added enhancement New feature or request good first issue Good for newcomers labels Jan 28, 2023
@S-ecki S-ecki linked a pull request Jan 28, 2023 that will close this issue
@S-ecki
Copy link
Author

S-ecki commented Jan 28, 2023

@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?

@gskinner
Copy link
Owner

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
Copy link
Owner

gskinner commented Feb 7, 2023

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

@gskinner
Copy link
Owner

gskinner commented Feb 10, 2023

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. :)

@S-ecki
Copy link
Author

S-ecki commented Feb 23, 2023

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

@drown0315
Copy link
Contributor

drown0315 commented Dec 19, 2024

I think the addRepaintBoundary parameter will be used very frequently in animations, similar to how ListView often needs it for its items. I think that's why ListView provides the addRepaintBoundaries option. Having an addRepaintBoundary option, as opposed to directly using RepaintBoundary, reduces an extra layer of nesting and indentation, improving readability.

  ListView.builder(
      itemBuilder: (_, __) => const SizedBox.shrink(),
      itemCount: 1,
      addRepaintBoundaries: false,
    )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants