-
Notifications
You must be signed in to change notification settings - Fork 668
DYN-6738: Fix gizmo redraw and camera drift issues #16852
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
DYN-6738: Fix gizmo redraw and camera drift issues #16852
Conversation
Fixes two issues with gizmo manipulation: 1. Gizmo redraw failure during drag - fixed by adding forceAsyncCall=true to render queue 2. Camera drift when zoomed in - fixed by filtering mouse events and clearing stale hit state Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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-6738
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.
Pull request overview
This PR fixes two critical issues with gizmo manipulation in Dynamo: redraw failures during drag operations and camera drift when zoomed in close. The fixes ensure proper async rendering during manipulation and prevent mouse event conflicts between gizmo manipulation and camera operations.
Changes:
- Added mouse button filtering to restrict gizmo manipulation to left-button-only events
- Implemented
ClearHitState()method to clear cached hit data after manipulation - Fixed async rendering during drag by adding
forceAsyncCall=trueparameter
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/DynamoManipulation/NodeManipulator.cs |
Added mouse button filtering, pan/orbit detection guards, ClearHitState invocation, and forceAsyncCall parameter fix |
src/DynamoManipulation/TranslationGizmo.cs |
Implemented ClearHitState() method to clear hitAxis and hitPlane fields |
test/DynamoCoreWpf2Tests/ViewExtensions/GizmoManipulationTests.cs |
Added comprehensive test suite with 8 tests covering button filtering, pan/orbit detection, and hit state clearing |
test/DynamoCoreWpf2Tests/ViewExtensions/GizmoManipulationTests.cs
Outdated
Show resolved
Hide resolved
| // Doing this triggers a graph update on scheduler thread | ||
| OnGizmoMoved(GizmoInAction, offset); | ||
|
|
||
| // redraw manipulator at new position synchronously |
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.
Do we need to update this comment? The fact that it was specifically mentioned synchronously before, I wonder if the behavior should be changed to async in the first place.
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 bug does this address exactly?
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.
The comment was not updated when the change was made, but it should have
Changing from synchronous to async was an effort to make sure the gizmo does not stutter when dragging it. In my testing there was a minor lag when dragging. The change did make it more responsive, but did not completely eliminate the lag.
Since it is just a minor improvement, should I revert the change? Or leave it and update the comment? What do you think?
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.
So, do you agree then that the comment should be updated to "asynchronously"?
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. Either change the comment or revert this change to calling AddGeometryForRenderPackages. Like I said, it was a marginal improvement and might be unnecessary. Based on the comment, it seems pretty intentional that it was supposed to be synchronous. I'm good reverting the change.
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'm not sure about deciding between synchronous vs async, or how to assess that. If you feel that the dragging is smoother with the latter, then let's keep the change.
|
@jasonstratton, you mention in the description - Secondly you mention |
There were 2 issues mentioned in the Jira issue.
So the question about the "Gizmo redraw failure during drag" refers to the latter. When clicking on one of the arrows in the gizmo and dragging, the gizmo remains stationary until MouseUp. So the PR will redraw the Gizmo when dragging it so that it follows the cursor motion. If I understand the PR you located, it was an optimization so that the geometry associated with the selected node does not move. I imagine there was a performance issue when updating the geometry, but the regression after that change was that the Gizmo did not update. Does that sound correct to you?
I while fixing the bug I got into a state where the Gizmo drift did not happen, but when I zoomed in and then panned/orbited, the gizmo drift did happen. ... This may have been an issue with my testing, but it did happen multiple times. I then zoomed out and the drift would continue. But if I tried translating the gizmo again and then pan\orbit again, the drift would not occur again until I zoomed in again. I do not recall which line change corrected this behavior. |
@jasonstratton, the code changes seem fine to me. However, your observations above are confusing and not what I've experienced. Maybe, we could discuss this online tomorrow? The PR I located was indeed an optimization so that the geometry associated with the selected node does not move until mouse-up. This may not be that big a deal, but if you have heavy downstream logic that depends on that point position updating, the graph will reexecute that many times for each point position update, which could be repeated performance-heavy computation of largely disposable intermediate results. The regression after that change was that the Gizmo drifted on panning and orbiting - at least that is what I observed. I also have an explanation for it: the PR introduced changes to the mouse-up handler that executed even after a camera change (pan/orbit) event, which was unnecessary and caused the gizmo to jump. |
|
|
||
| /// <summary> | ||
| /// Verifies that ClearHitState method exists and properly clears hitAxis and hitPlane fields. | ||
| /// This is the core fix for Issue 2 (camera drift prevention). |
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.
This comment is misleading. ClearHitState is just a guardrail, not the main fix.
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.
Agreed. I will update.
test/DynamoCoreWpf2Tests/ViewExtensions/GizmoManipulationTests.cs
Outdated
Show resolved
Hide resolved
| { | ||
| // Act - MouseUp should not throw and should be ignored in orbit mode | ||
| var leftButtonArgs = CreateMouseButtonEventArgs(MouseButton.Left); | ||
| Assert.DoesNotThrow(() => mouseUpMethod.Invoke(manipulator, new object[] { null, leftButtonArgs }), |
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 does not throwing from the method verify that mouseUp is ignored?
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.
If panning or orbiting, the MouseUp event should simply return
https://github.com/DynamoDS/Dynamo/pull/16852/changes#diff-0195a9bd38735814af7fb83cb7845540c0de8391a1dae99901a56a9adf06b871R250
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, but if it doesn't return, that doesn't mean that it will throw.
aparajit-pratap
left a comment
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.
Minor comments and questions, then LGTM.
Summary
Fixes two issues with gizmo manipulation:
forceAsyncCall=trueparameter to ensure render operations are properly queued on the dispatcherRoot Causes
forceAsyncCallparameter prevented proper async rendering during dragChanges
src/DynamoManipulation/NodeManipulator.cs- Mouse button filtering, pan/orbit detection, ClearHitState call, forceAsyncCall fixsrc/DynamoManipulation/TranslationGizmo.cs- Added ClearHitState() methodtest/DynamoCoreWpf2Tests/ViewExtensions/GizmoManipulationTests.cs- Comprehensive test coverage (8 tests, all passing)Performance
Preserves PR #12614's performance optimization - graph updates occur only on MouseUp, not during drag.
Testing
All 8 new unit tests passed in Visual Studio Test Explorer.