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

Simplify annotations #12924

Merged
merged 17 commits into from
Jun 2, 2022
Merged

Simplify annotations #12924

merged 17 commits into from
Jun 2, 2022

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented May 25, 2022

Purpose

Purpose:
Improve performance when dragging/moving Annotations.
https://jira.autodesk.com/browse/DYN-4600

Changes in this PR:

  1. Reduce number of CollectionChanged notifications by adding support for batch operations.
  2. Reuse previously computed proxy input ports and proxy output ports for annotations (stop recomputing them on each Position change)
  3. Reduce the number of PropertyChanged notifications by checking if values are modified
  4. Reduce number of PropertyChanged notifications by not trying to move the Annotations in the first place (since their position/boundary is always recomputed when the inner nodes are moved)

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

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. 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

@pinzart90 pinzart90 marked this pull request as ready for review May 25, 2022 19:05
@QilongTang
Copy link
Contributor

@QilongTang Is the overlay of annotation boundaries a deal breaker ? With a bit more effort we could try to keep the cutout logic...but it will be messier ...

I remember it is used when user drag stuff into the group, you might want to give it a test there

@pinzart90
Copy link
Contributor Author

@QilongTang Is the overlay of annotation boundaries a deal breaker ? With a bit more effort we could try to keep the cutout logic...but it will be messier ...

I remember it is used when user drag stuff into the group, you might want to give it a test there

I reverted to using cutouts ...and I fixed the performance by limiting the ObservableCollection notifications

pinzart added 2 commits May 26, 2022 12:35
Merge with existing observableCollection wrapper
switch (e.PropertyName)
{
case nameof(PortModel.Center):
RaisePropertyChanged(nameof(CurvePoint1));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is the right property for which to raise an event.
Does CurvePoint1 correspond to the end Node ?
I did see mentioned somewhere in the code that CurvePoint0 corresponds to the start Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the Redraw function...it looks like CurvePoint3 should correspond to the end Node

@@ -198,7 +198,10 @@ public Point2D Center
}
internal set
{
if (center.Equals(value)) return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is Point2D a struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs Outdated Show resolved Hide resolved
src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs Outdated Show resolved Hide resolved
src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs Outdated Show resolved Hide resolved
src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs Outdated Show resolved Hide resolved

// Annotations always update their position relative to all nested Nodes
// So there is no need to move the Annotation since it will be updated later anyway (performance improvement)
if (locatable is AnnotationModel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this actually end up correctly setting the x and y of the annotationModel or does it draw it correctly, but leave the model data incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how the notifications flow:
NestedNode.PositionUpdate => AnnotationNode.ModelDataUpdate => AnnotationViewModel.ViewModelDataUpdate
X and Y (and all other annotation model data) are set correctly in the annotation model then it draws (correctly).

/// This class supports batch operations that should defer CollectionChaned notifications.
/// </summary>
/// <typeparam name="T"></typeparam>
public class SmartObservableCollection<T> : ObservableCollection<T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why make this public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it is used to bind to WPF controls

src/DynamoUtilities/SmartObservableCollection.cs Outdated Show resolved Hide resolved
@pinzart90 pinzart90 merged commit 9edca69 into master Jun 2, 2022
@pinzart90 pinzart90 deleted the simplify_annotations branch June 2, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants