-
Notifications
You must be signed in to change notification settings - Fork 281
Updated gizmo scaling behaviour to match translation #489
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
Updated gizmo scaling behaviour to match translation #489
Conversation
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.
Awesome work! Tested locally and it works as expected.
Some nits:
- use tabs instead of spaces for indentations (I know we are not 100% consistent across the board, but we are trying to move away from spaces)
- use
constqualifiers whenever possible (i.e.const auto ray,const planePoint, etc...). Same as above, not 100% consistent, but we're now trying to enforce it more.
P.S. thank you for taking the time to document your PR, it's much appreciated and makes reviewing so much easier!
0a85e2b to
351a9fa
Compare
|
Thanks for the review and the positive feedback! The * 2.0f multiplier definitely feels more natural. I've implemented the suggestions and amended my previous commit:
And thank you for the kind words about the PR documentation! I always try to make the review process as smooth as possible. |
adriengivry
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.
Seems like something went wrong with the last commit 😅
OvEditor_cGVaixdWQR.mp4
On a side note, you don't necessarily need to squash/amend commits, as they'll automatically be squashed into a single commit when merged to main! Sometimes better to keep the full history on the PR so it's easier to see what has changed between each review/revert if something went wrong
adriengivry
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.
LGTM! Approved ✅
PR Description: Update Gizmo Scaling Behavior to match Ray-Based Translation
Closes #488
Problem:
The gizmo's scaling operation previously felt different in magnitude and responsiveness compared to the translation gizmo. This was because the translation gizmo had been updated to use a more accurate ray-based interaction model (as per #306), while the scaling gizmo still relied on older screen-space mouse movement calculations. This inconsistency could make precise scaling less intuitive.
Description of Changes:
This PR updates the scaling behavior of the editor gizmo to align it with the improved ray-based logic now used by the translation gizmo. The goal is to provide a more consistent and predictable user experience when manipulating objects in 3D space.
Fix Implemented:
The
ApplyScalemethod withinOvEditor::Core::GizmoBehaviourhas been refactored. Instead of calculating scale based on 2D screen-space mouse displacement, it now leverages the core ray-casting and 3D plane intersection logic from theApplyTranslationmethod, as suggested in the original issue (#488).Specifically:
ApplyScalemethod is no longerconstas it now modifies internal state (m_firstPick,m_initialOffset) consistent withApplyTranslation.By adopting this approach, the scaling operation now directly responds to how the mouse moves in the 3D world relative to the gizmo axis, rather than interpreting 2D screen movements. This should result in a scaling gizmo that feels more direct, accurate, and harmonious with the translation gizmo.
Screenshots:
before the changes in this PR:
After: