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

Fixed some Transform3DGizmo behavior to make more consistent #53684

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Oct 11, 2021

Note: According to the discussion with @reduz on the contributors chat, it was decided to go ahead with the implementation of the 8 rotation modes (euler combinations, quaternion and custom basis). After that is merged, we will consider this change again. #54084 has been merged.


This PR makes some changes in Gizmo.

  1. Implement non-orthogonal Gizmo
  2. Fixes for moving and scaling when multiple objects are selected in local transform mode
  3. Implement a mode in global transform mode that keeps orthogonality (and add an orthogonal option to Node3D for this purpose) Superseded by Refactored Node3D rotation modes #54084.

It addresses the problems those were mentioned in #51164 (comment). Fixed #18987.


Implement non-orthogonal Gizmo

Currently, even when an object's axis is non-orthogonal in the 3D view, Transform3DGizmo tries to keep it orthogonal, so local transformations along the axis are not performed correctly in such cases. This will fix that.

スクリーンショット 2021-10-12 5 05 48


Fixes for moving and scaling when multiple objects are selected in local transform mode

In Local Transform mode, when multiple objects are selected, rotation correctly takes into account the local axis, but translation and scaling do not. This will make these a consistent system.

2021-10-12.5.07.47-1.mov

Implement a mode in global transform mode that keeps orthogonality (and add an orthogonal option to Node3D for this purpose)

The current global transformation causes shear in basis. Shear is not allowed in glTF and is not compatible with the compression track that @reduz is working on, especially in bone animation. So this will implement global scaling that does not cause shear like in Blender.

2021-10-12.5.10.34-1.mov

@TokageItLab TokageItLab requested review from a team as code owners October 11, 2021 20:13
@Calinou Calinou added this to the 4.0 milestone Oct 11, 2021
@TokageItLab TokageItLab marked this pull request as draft October 11, 2021 20:16
@TokageItLab TokageItLab force-pushed the orthogonal-mode branch 11 times, most recently from 8ea951d to a302770 Compare November 19, 2021 23:04
@TokageItLab TokageItLab changed the title Fix some gizmo behavior and implement orthogonal option in Node3D Fixed some Transform3DGizmo behavior to make more consistent Nov 19, 2021
@TokageItLab TokageItLab marked this pull request as ready for review November 19, 2021 23:07
@TokageItLab TokageItLab force-pushed the orthogonal-mode branch 4 times, most recently from 80efc79 to 9c15629 Compare November 19, 2021 23:51
@TokageItLab
Copy link
Member Author

TokageItLab commented Nov 19, 2021

In previous discussions with @reduz, it was decided that Node3D should be forced to be orthogonal if the rotation mode is not basis. It may be necessary to orthogonalize with an importer or migrator for objects that are currently in shear regardless of the rotation mode is not basis.

@@ -1876,14 +1881,14 @@ void Node3DEditorViewport::_sinput(const Ref<InputEvent> &p_event) {
if (se->gizmo.is_valid()) {
for (KeyValue<int, Transform3D> &GE : se->subgizmos) {
Transform3D xform = GE.value;
Transform3D new_xform = _compute_transform(TRANSFORM_SCALE, se->original * xform, xform, motion, snap, local_coords);
Transform3D new_xform = _compute_transform(TRANSFORM_SCALE, se->original * xform, xform, motion, snap, local_coords, true); // Force orthogonal with subgizmo.
Copy link
Member Author

@TokageItLab TokageItLab Nov 21, 2021

Choose a reason for hiding this comment

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

Currently SubGizmo is forcing orthogonal, but I'm wondering how to make this work better.
At least it's independent of Node3D's RotationMode, but if you want to manage it on a per-SubGizmo make orthogonal, we need to make the SubGizmo store more info than just Transform3D.

For now, it is only the manipulation of bones in Skeleton3DGizmo that is being transformed using SubGizmo, so I don't see a problem with it, but if there is a suggestion in the future, I think it should be resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that should work for now. We can expand what's being stored for each subgizmo in the future if we find the need.

@TokageItLab
Copy link
Member Author

TokageItLab commented Nov 22, 2021

When multiple objects are translated with local mode, there is a possibility that the Gizmo will move to an unexpected position due to a shift in the center.

It's mathematically and programmatically consistent, but it's may hard to know, so it might be better to visualize the center using a bounding box or a dotted line from each origin to the center.

Or, in such a case, not moving the Gizmo during the transformation may be a solution. It might be better to find a solution for Gizmo's visual effects in conjunction with #48307.

@TokageItLab TokageItLab force-pushed the orthogonal-mode branch 3 times, most recently from ac311fb to e92ac5b Compare November 22, 2021 18:31
@TokageItLab TokageItLab marked this pull request as draft December 24, 2021 18:04
@TokageItLab TokageItLab marked this pull request as ready for review December 24, 2021 18:20
@TokageItLab
Copy link
Member Author

TokageItLab commented Dec 24, 2021

Added scale_orthogonal() in Basis, it is also necessary for retargeting nodes (godotengine/godot-proposals#3379), since shearing is not allowed in bones.

@JFonS Please review when you have time.

Copy link
Contributor

@JFonS JFonS left a comment

Choose a reason for hiding this comment

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

Looks great :)

@@ -1876,14 +1881,14 @@ void Node3DEditorViewport::_sinput(const Ref<InputEvent> &p_event) {
if (se->gizmo.is_valid()) {
for (KeyValue<int, Transform3D> &GE : se->subgizmos) {
Transform3D xform = GE.value;
Transform3D new_xform = _compute_transform(TRANSFORM_SCALE, se->original * xform, xform, motion, snap, local_coords);
Transform3D new_xform = _compute_transform(TRANSFORM_SCALE, se->original * xform, xform, motion, snap, local_coords, true); // Force orthogonal with subgizmo.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that should work for now. We can expand what's being stored for each subgizmo in the future if we find the need.

@akien-mga akien-mga merged commit 6af77c7 into godotengine:master Jan 5, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting a global rotate and scale may lead to a broken Basis
4 participants