Improved zoom and navigation performance for large graphs with many nodes #15749
Conversation
|
|
||
| public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
why should this always return false?
There was a problem hiding this comment.
- I think this approach makes a lot of sense and thanks for finding it.
- We should be using feature flags - I think one simple approach here is setup a feature flag that is an int/double that you can control the animation cut off with. I will setup that feature flag, and then can you consume it to control the cut off? I can provide an example of how to read a feature flag with a default fallback.
- I would like if you can move the image changes to another PR.
IMO It will be much simpler to get this merged using the feature flag approach because then we can get builds in front of UX and PMs, we can adjust the settings and come to an agreement on value and the code is already in master instead of needing to have a drawn out back and forth here.
| <Style x:Key="SZoomFadeText" TargetType="{x:Type TextBlock}"> | ||
| <Style | ||
| x:Key="SZoomFadeBase_Animation" | ||
| TargetType="{x:Type FrameworkElement}"> |
There was a problem hiding this comment.
why the change of target type? Is the comment still accurate?
| @@ -1800,7 +1879,7 @@ | |||
| FontSize="{TemplateBinding FontSize}" | |||
| FontWeight="Bold" | |||
| Foreground="{TemplateBinding Foreground}" | |||
| Style="{StaticResource SZoomFadeText}" | |||
| Style="{Binding Path=DataContext.StopAnimations, RelativeSource={RelativeSource FindAncestor, AncestorType={x:Type controls:WorkspaceView}}, Converter={StaticResource SZoomFadeControl}}" | |||
There was a problem hiding this comment.
I think this has the potential to be very slow given there are a bunch of elements that are making these find ancestor calls - maybe we can do something like - pass this down to the nodeView data context and have all elements reference that data context instead of searching all the way back up to the workspace view?
There was a problem hiding this comment.
Is it possible to to profile this somehow for a very large graph, thousands of nodes? I want to make sure we don't fix one issue only to introduce another.
I see that this PR adds a lot of additional find Ancestor calls.
There was a problem hiding this comment.
maybe a simpler approach would be to just make this property static... I think you can then bind directly to it without the ancestor lookup.
| @@ -479,6 +479,10 @@ public double Zoom | |||
| } | |||
| } | |||
|
|
|||
| [JsonIgnore] | |||
| public bool StopAnimations { get => stopAnimations; set { stopAnimations = value; RaisePropertyChanged(nameof(StopAnimations)); } } | |||
| @@ -19,6 +20,14 @@ | |||
| PreviewMouseLeftButtonDown="OnPreviewMouseLeftButtonDown" | |||
| PreviewMouseMove="OnNodeViewMouseMove"> | |||
|
|
|||
| <UserControl.Resources> | |||
There was a problem hiding this comment.
thanks for looking into this.
While it might seem like overkill, it's going to be much simpler in the end to get things into master if PRs make only one change at a time, especially if those are UI changes, or changes which will require buy in from multiple parties.
Can you split out the image changes into another PR?
|
there are some public methods that need to be added to the unshipped file: |
this is a cleaned up version of the original PR
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
WorkspaceViewModelin 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 also 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:
Declarations
Check these if you believe they are true
*.resxfilesRelease 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
@mjkkirschner
(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