-
Notifications
You must be signed in to change notification settings - Fork 668
DYN-6738: Gizmo Display Issues #16844
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
base: master
Are you sure you want to change the base?
DYN-6738: Gizmo Display Issues #16844
Conversation
Fix for Gizmo drag updates and drift when panning/orbiting Unit tests
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 addresses gizmo display issues during drag operations and camera navigation (DYN-6738). The fix implements incremental node updates during dragging for real-time redraw, prevents camera pan/orbit events from interfering with gizmo manipulation, and queues render updates asynchronously to reduce UI thread contention.
Changes:
- Updated gizmo drag behavior to provide incremental node updates during mouse move and apply final rounding on mouse up
- Added filtering in MouseDown/MouseUp to ignore events when camera is panning or orbiting, and added ClearHitState call to prevent drift
- Changed render package updates to use asynchronous queuing during drag operations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/DynamoCoreWpf2Tests/ViewExtensions/GizmoManipulationTests.cs | New test file covering drag updates, camera interaction filtering, hit-state clearing, and origin separation |
| src/DynamoManipulation/TranslationGizmo.cs | Added ClearHitState method to reset cached hit axis and plane |
| src/DynamoManipulation/NodeManipulator.cs | Updated MouseDown/MouseUp/MouseMove to filter camera events, implement incremental drag updates, and clear hit state |
| dynamic uiNode = inputNode; | ||
| uiNode.Value = amount; |
Copilot
AI
Jan 21, 2026
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 code uses 'dynamic' without type checking, which could fail at runtime if inputNode doesn't have a Value property. Consider adding type validation or using a more specific interface/base type that guarantees the Value property exists.
| pointNode.IsSelected = true; | ||
|
|
||
| var manipulator = new MousePointManipulator(pointNode, manipulationExtension); | ||
| var slider = new DoubleSlider { Value = 1.23456 }; |
Copilot
AI
Jan 21, 2026
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 test creates a DoubleSlider but doesn't add it to the graph model or verify it's properly connected. Consider adding the slider to the graph through ExecuteCommand to ensure it's in a valid state, or add a comment explaining why this simplified setup is sufficient for the test.
| dynamic uiNode = inputNode; | ||
| uiNode.Value = Math.Round((double)uiNode.Value, ROUND_UP_PARAM); |
Copilot
AI
Jan 21, 2026
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 code uses 'dynamic' without type checking, which could fail at runtime if inputNode doesn't have a Value property of type double. Consider adding type validation or using a more specific interface/base type.
| var updatedNodes = updatedNodesField.GetValue(manipulator) as HashSet<NodeModel>; | ||
| Assert.IsNotNull(updatedNodes, "inputNodesUpdatedDuringDrag should be a HashSet"); | ||
|
|
||
| updatedNodes.Add(slider); |
Copilot
AI
Jan 21, 2026
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 test modifies internal state through reflection but doesn't verify that the slider node is compatible with the manipulator's expected node types. Consider verifying the node is properly registered with the model or documenting why this direct manipulation is safe for testing.
| dynamic uiNode = inputNode; | ||
| uiNode.Value = Math.Round(amount, ROUND_UP_PARAM); | ||
| dynamic uiNode = inputNode; | ||
| uiNode.Value = Math.Round(amount, ROUND_UP_PARAM); |
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.
maybe a good idea here to check if the final value we are setting for uiNode.Value is not the same or close as the current value?
dynamic uiNode = inputNode;
var currentValue = (double)uiNode.Value;
var roundedValue = Math.Round(currentValue, ROUND_UP_PARAM);
if (Math.Abs(currentValue - roundedValue) > MIN_OFFSET_VAL)
{
uiNode.Value = roundedValue;
}
| uiNode.Value = Math.Round((double)uiNode.Value, ROUND_UP_PARAM); | ||
| } | ||
|
|
||
| inputNodesUpdatedDuringDrag.Clear(); |
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 think this should also be called in the Dispose() method, since the HashSet contains NodeModel references which will not be cleared in case of early exit from MouseUp event.
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.
@jasonstratton I identified the PR that broke this behavior - #12614 in Dynamo 2.14. This bug is a regression and didn't exist prior to version 2.14. Could you take a look at #12614 and identify reasons why those changes could have caused this regression in the first place?
| // Skip processing if user is panning or orbiting the camera. | ||
| // This prevents the manipulator from capturing mouse events intended for camera navigation, | ||
| // including cases where pan/orbit tools use the left mouse button. | ||
| if (BackgroundPreviewViewModel is DefaultWatch3DViewModel viewModel) |
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 condition to check if the backgroundpreview mode is on and not the graph view mode?
| } | ||
| else | ||
| { | ||
| var inputNodesToManipulate = InputNodesToUpdateAfterMove(Vector.ByTwoPoints(originBeforeMove, originAfterMove)); |
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.
When will control reach here inside this else?
| } | ||
| } | ||
|
|
||
| //Update gizmo graphics after every camera view 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.
Will this statement ever be true? If there's a camera view change, it doesn't look to me like control will ever reach here in the first place.
| // This ensures stale axis/plane hit data doesn't affect pan/orbit movements | ||
| if (gizmo is TranslationGizmo translationGizmo) | ||
| { | ||
| translationGizmo.ClearHitState(); |
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 really necessary, even after preventing mouseup or mousedown events from doing anything after a camera change event?
| // Doing this triggers a graph update on scheduler thread | ||
| OnGizmoMoved(GizmoInAction, offset); | ||
|
|
||
| UpdateInputNodesDuringDrag(offset); |
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.
Calling this function inside MouseMove seems like the changes in #12614 to improve performance are now being undone since you're now updating the graph for every incremental mouse move event rather than waiting until the mouse button is released.
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 wonder if simply preventing camera change events (pan and orbit specifically) from doing any damage in MouseUp will do the trick in fixing this bug. To me, intuitively, it looks like it will.
From what I understand, this bug can be fixed by simply adding this code to the start of MouseDown and MouseUp and calling it a day:
// Skip processing if user is panning or orbiting the camera.
// This prevents the manipulator from capturing mouse events intended for camera navigation,
// including cases where pan/orbit tools use the left mouse button.
if (BackgroundPreviewViewModel is DefaultWatch3DViewModel viewModel)
{
if (viewModel.IsPanning || viewModel.IsOrbiting)
return;
}
// Only process left mouse button releases for gizmo manipulation
// Right/middle button releases are for camera navigation (pan/orbit)
if (e.ChangedButton != MouseButton.Left)
{
return;
}
Summary
Test plan