introduce node count threshold for opacity animations on zoom #15764
introduce node count threshold for opacity animations on zoom #15764mjkkirschner merged 14 commits intoDynamoDS:masterfrom
Conversation
|
Something else that came to mind was setting the visibility property instead of the opacity for the basic styles. Tho I'm not sure if the benefit is worth the extra complexity. Since we're switching between two styles, we would have to add the visibility property to both styles. |
thanks @dimven-adsk that makes sense, I may leave that as a TODO and file a follow-up optimization so these PRS don't drag on forever though. |
mjkkirschner
changed the title
|
I will leave this here for posterity: https://learn.microsoft.com/en-us/dotnet/desktop/wpf/graphics-multimedia/how-to-animate-the-opacity-of-an-element-or-brush?view=netframeworkdesktop-4.8 |
|
The animation looks good to me |
What about the lack of animation in big graphs ? |
discussed over chat with @Jingyi-Wen - @pinzart90 I think we are good to proceed with this if you approve. We can tune the feature flag later, for now I will set it to 150. |
| StringAssert.Contains("\"TestFlag2\":\"I am a string\"", log); | ||
| StringAssert.Contains("\"graphics-primitive-instancing\":true", log); | ||
| StringAssert.EndsWith("<<<<<Eod>>>>>", log); | ||
| StringAssert.Contains("<<<<<Eod>>>>>", log); |
There was a problem hiding this comment.
@pinzart90 due to the changes you call out below.
|
|
||
| }, null); | ||
| var formattedFlags = JsonConvert.SerializeObject(AllFlagsCache, Formatting.Indented); | ||
| OnLogMessage($"retrieved feature flags with value: {formattedFlags}"); |
There was a problem hiding this comment.
is this just a debugging thing ? or it makes sense for users too?
There was a problem hiding this comment.
Logging from ff had performance issues in the past. Might OnLogMessage trigger Wpf updates ?
There was a problem hiding this comment.
It definitely has WPF updates as it triggers the log text binding to update, but this is a single log, and as we get more and more flags, the other log that we have that spits out the std.out is not readable. This makes it much easier IMO for devs to see what flags are enabled when a user gives us their log.
pinzart90
left a comment
pinzart90
left a comment
There was a problem hiding this comment.
Couple of questions. LGTM
4 participants
Purpose
extracts some changes from:
#15749 by @dimven
For a sense of what theses do in terms of interaction performance / UI feel please see the original PR description for gifs.
Essentially we do not animate opacity changes when zooming as this has very poor performance that we cannot control.
Instead, only trigger these animations if the total number of nodes in the graph is under some threshold.
TODO:
StopAnimationsproperty feels kind of static to me anyway... I just don't think it's worth messing with it at the current time.Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
improve zoom performance by only animating opacity changes if there are few nodes in the graph.