CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.89k stars 1.38k forks source link

Animation Performance degration post PR #631 #759

Closed hermitdave closed 7 years ago

hermitdave commented 7 years ago

I am noticing significant perf issues.. UI Thread becoming extremely busy.. in a few places I used Fade and Offset extensions

tasks[0] = this.ChannelFlipView.Fade(0).StartAsync();
tasks[1] = this.RelatedArticleView.Fade(0).StartAsync();
tasks[2] = this.ArticleFlipView.Fade(1).StartAsync();
tasks[3] = this.ArticleCommentsGrid.Fade(0).StartAsync();
tasks[4] = this.ReadLaterItems.Fade(0).StartAsync();

await Task.WhenAll(tasks);

in other places the sequence is slightly different.. the code doesn't care which one had opacity 0 / 1 it just blindly asks for it to be set.

b.Offset(offsetY: 0, duration: animationduration).Start();
separator.Fade(0, animationduration).Start();

In this instance I don't wait for animations to finish before kicking off the next set finally the standalone fade calls when changing layout due to size changes

await this.Fade(value: 0, duration: 10).StartAsync();
/* change layout etc */
await this.Fade(value: 1, duration: 100).StartAsync();

This results in a significant overhead on UI thread and it blocks most UI from rendering for a visibly noticeable period. please help

hermitdave commented 7 years ago

This is post 1.2 and using myget builds or directly connecting to source. ping @nmetulev & @shenchauhan

JohnnyWestlake commented 7 years ago

Frankly for your use case using those methods seems like expensive overkill considering the amount of unnecessary tasks and ManualResetEvents that AnimationSet relies on for your use case.

Granted I have no idea the thought process behind animationset being so complicated and maybe it does need to be, but you'd be better off rolling your own simple extensions, for what you're doing as even if the regression is handled it's still wasteful (and ideally wants to be disposed after use which is more work on your part)

hermitdave commented 7 years ago

I agree @JohnnyWestlake I have a copy of original extensions based on storyboard and I used them for 10240 as fade isn't supported. I could always take a copy of original composition extensions.

The diff showed that it was not a small change by any means https://github.com/Microsoft/UWPCommunityToolkit/pull/631/files#diff-7d1d14b0d3ac642b0a209d0075717803

just want to make sure what is now is usable in general usage

shenchauhan commented 7 years ago

I will sit down with @nmetulev once he is back from being OOF. Tasks are used to support effects (like lighting) StartAsync, stop and reset. Does the UI Thread get busy even when the XAML tree is not deep? Do you have a sample that you can share? It would help us diagnose better.

hermitdave commented 7 years ago

I am not really sure.. this is the in progress app. Its designed like SPA. not forward backward navigation. depending upon user selection, views are toggled. There are two flipviews which substantially template selectors for content listview There's 2 additional listview and a grid that contains a listview as well so no its not a light XAML tree.

From a decent rendering steep now the images take forever to load.

If the current extensions are optimised for better usage, I can just take a copy of original composition extensions or just create a custom set since my requirement is very simple

hermitdave commented 7 years ago

I will try and attach a simple repro today

hermitdave commented 7 years ago

hello both, the simple repro didn't exhibit that behaviour. I have asked Org owner how I can invite you both. hopefully soon