| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8305: Implement Bitmap Cache for nodes when zoomed out #16174
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
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8305
| //disable bitmap caching if max zoom scale set to 0, or feature flag was unable to fetch; | ||
| if (ViewModel.MaxZoomScaleForBitmapCache == 0) return; | ||
|
|
||
| if (!ViewModel.StopNodeViewOpacityAnimations) |
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.
Why is this bitmap cache being tied to this other animation property?
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.
Graphs with lower number of nodes, does not really require this optimization, so it makes sense to trigger this beyond a certain number of nodes. In this case the animation property also uses another feature flag that is set to 150, which we are re-using in this case.
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.
what do you think about renaming this property?
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.
How about NodeCountOptimizationEnabled ? (As suggested by copilot)
| return; | ||
| } | ||
|
|
||
| var newRenderScale = newZoomScale <= .2 ? .2 : newZoomScale <= .5 ? .5 : 1; |
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 thought this was a bit hard to read - copilot suggests:
var newRenderScale = newZoomScale switch
{
<= 0.2 => 0.2,
<= 0.5 => 0.5,
_ => 1
};
which I think looks a bit simpler?
|
|
||
| private void ClearNodeViewCache() | ||
| { | ||
| var nodes = this.ChildrenOfType<NodeView>(); |
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.
hmm - have you tested this change on a custom node workspace?
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.
yes
| BitmapCache sharedCache = null; | ||
| foreach (var node in nodes) | ||
| { | ||
| if (node.CacheMode is BitmapCache cache) |
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.
is this just a null check?
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.
yes
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 might be more clear if you just check it's null.
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.
We need to cast anyway if the condition is true, I think it is better since it checks for null and also gives us the appropriate object
| @@ -94,7 +94,7 @@ internal void CacheAllFlags() | |||
| /// <returns></returns> | |||
| internal T CheckFeatureFlag<T>(string featureFlagKey, T defaultval) | |||
| { | |||
| if(!(defaultval is bool || defaultval is string || defaultval is long)){ | |||
| if(!(defaultval is bool || defaultval is string || defaultval is long || defaultval is double)){ | |||
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.
comment is now out of date on this method.
Purpose
The PR is based on changes here
changes from the original PR:
As described in the original PR,
It introduces a mechanism by which, when the number of nodes on canvas is higher than a certain threshold(150) and zoom scale is at a certain level , we replace all node views with rendered bitmaps, using WPF's existing mechanism called BitmapCache.
Based on the current zoom, we can vary the resolution of the images and create a pseudo Load On Demand system that switches between a low, medium, high and native scale.
So at the zoomed out state we load node views at 0.2 scale (low res), then as zoom increases we switch to 0.5, and then to native scale.
Improved graph movement:
Before:
After:
Profile results: When scrolling to change zoom level, the cache update and clear takes an additional ~350ms (0.3s) at certain zoom level when the bitmap kicks in, but has no significant change from previous state.
Tested with Autodesk-MaRS-office-example-v3.dyn and Entry 05_Christmas Tree with ornaments over and around.dyn graphs with identical results.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes