-
Notifications
You must be signed in to change notification settings - Fork 636
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
Improved zoom and navigation performance for large graphs with many nodes #15730
Improved zoom and navigation performance for large graphs with many nodes #15730
Conversation
simplify node icons
This reverts commit 68ab808.
I don't know how important the Animations are for UX, if they are required, another option would be to try to apply them for fewer UI elements (like the main node grid). It does seem like, at least. the fade animations are applied to all border types, some to FrameworkElement and others to TextBlocks |
…-wpf-improvements
Hi @pinzart90, I merged the updates from the main branch and resolved the merge conflicts |
<Style x:Key="SZoomFadeText" TargetType="{x:Type TextBlock}"> | ||
<!-- Zoom fade control --> | ||
<Style | ||
x:Key="SZoomFadeBase_Animation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's really hard to review this with all of these formatting changes, can you revert them and keep just the significant changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's possible. The changes were from a few months ago. I rebased from main and that pulled hundreds of commits. I think I'll have to make a new branch and a new PR. I'll be sure to remember to turn off code cleanup next time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dimven-adsk - let’s create a new pr and close this one.
Closing this one and continuing it here |
Purpose
Currently large graphs perform very poorly. It can take up to multiple seconds to zoom in and out, particularly when moving past the 0.4 zoom stage, where the node and port names disappear and the effect borders appear. Panning the view can also be slow.
There are multiple reasons for this slowdown but the biggest contributor by far are the many fade in/out animations that happen at that particular transition. This was particularly difficult to trace, because the slowdown does not happen in the C# managed code. Instead, it actually comes from the multiple animation bindings to the "Zoom" property of the
WorkspaceViewModel
in every single node. This is why the hotpath stops at raising the property changed event and can not be expanded further:From what I've gathered, WPF animations get pre-compiled to native platform code and that is why we can't get further details in both VS and dotTrace.
This is further exasperated by the fact that the animations target the contols' opacity property. Apparently when you modify an element’s opacity property, it can cause WPF to create temporary surfaces which results in an even bigger performance hit.
With this PR, I propose we have a cut-off point after a certain number of nodes are placed, where the animations are turned off (for example after placing 150 nodes on the canvas).
Another reason highlighted in the above image, is that every single placed node and group throws a binding failure, which slows down graph loading/rendering times. In the PR I propose that we simplify the XAML visual tree, which avoids the binding failures and is beneficial to the overall performance. Plus, since the node icons are already so tiny, we can further improve the performance of large graphs by using speedier image rendering options.
User Experience
Currently with a graph of around 350 nodes, each zoom in/out action can take multiple seconds, which totally breaks the flow:
With the proposed changes, animations still work for small graphs but for large graphs they are replaced with data trigger setters that replicate the end state:
TODO: This is based on our scope from last quarter and was targeting an older release of Dynamo (distributed with the latest version of Revit at that time). There are some merge conflicts to clear.Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of