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

WIP - introduce node count threshold for opacity animations on zoom #15764

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Jan 17, 2025

Purpose

extracts some changes from:
#15749 by @dimven

Essentially 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:

  • create feature flag
  • use feature flag for the threshold
  • - adding some logging to WPF it looks like these find ancestor calls take about 400 microseconds per node, thats about 2500 nodes in a second, and that cost should only be paid once (when the binding is first attached), even though the StopAnimations property feels kind of static to me anyway... I just don't think it's worth messing with it at the current time.
  • consider tests for checking threshold

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

improve zoom performance by only animating opacity changes if there are few nodes in the graph.

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

@dimven-adsk
Copy link
Contributor

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.

@mjkkirschner
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants