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 gizmo forced implicit normalization and inconsistent rotation #51164

Merged

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Aug 2, 2021

Ref #18987.

The current behavior of Godot's Gizmo is inconsistent.

For example, if you do scaling in global transform mode and then rotate in local transform mode, implicit normalization will be done internally, resulting in an abbreviated transformation.

2021-08-01.10.22.56.mov

Also, the rotation in the global transform and the rotation in the local transform perform completely different transformations. In a child node with a scaled parent node, rotation in global transform mode will rescale the node and cause an implicit skew.

2021-08-01.10.12.25-1.mov

Correct rotation in a child node with a scaled parent node was considered to cause skew in the looks, ref: #32995.

With this PR, implicit normalization is no longer worked. So, skew will keep when transform, and matrix value will not change suddenly like first video. Moreover since the behavior of global and local transforms will be unified, I consider that it will be possible to implement something like auto-normalizing mode, keep-orthogonal mode (I think it will mainly be an option for the global transform scaling) for prevent to be generated local skew, and keep-looks mode for rotation #32995.

@JFonS
Copy link
Contributor

JFonS commented Aug 2, 2021

I did a quick test and this PR breaks multi-node editing in global transform mode.

I'm OK with removing implicit normalization, but we need to make sure we don't break existing behavior.

Ideally, we need to test the 4 combinations of single/multi node and local/global transform and make sure it works on a scene with non-uniform scaled and rotated nodes.

I think the issue comes from using _edit.center as the new origin when doing global scales and rotates. _edit.center is the global position of the transform gizmo, so it should be the center of rotation and scale when editing multiple nodes.

@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from b676058 to 72311a4 Compare August 2, 2021 16:29
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 2, 2021

@JFonS Fixed multiple-transform so that it retains its previous behavior. (It's still a little broken, I fixing now)

As for multiple-local-transform, it seems to be inconsistent from the beginning. Shouldn't this also be fixed? For example, multiple-local-translate works the same as multiple-global-translate, but multiple-local-rotate does individual rotation, unlike multiple-global-translate. As for scale, I think it is broken because it does not convert between input and axis.

@TokageItLab TokageItLab marked this pull request as draft August 2, 2021 16:56
@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from 72311a4 to aeddeb0 Compare August 2, 2021 18:29
@JFonS
Copy link
Contributor

JFonS commented Aug 2, 2021

Yup, multiple-local-scale seems to be broken.

Multiple-local-translate is inconsistent with the rest, but I think that's fine. If we change it to move all the nodes in their own local axis, it can result in each node moving in completely different directions and the transform gizmo will move around in a random direction, making it confusing for the user. I can't think of any use case for local translate on multiple nodes, usually when you want to move a group of nodes you want them to stay together, even if you are in local edit mode.

Unless we find a good use-case for it, so I would make an exception for the local "translate" tool and keep it global.

@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from aeddeb0 to ab9b6c4 Compare August 2, 2021 20:11
@TokageItLab
Copy link
Member Author

I can't think of any use case for local translate on multiple nodes

If you make the animation of explosives object manually, this could be useful as you can move them evenly in any axis direction. Blender's local transform + Individual origin option can be a good reference.

スクリーンショット 2021-08-03 5 15 45

If multiple-local-scale adopt the reference, I think we can make it a consistent system as well, how do you think?

@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch 4 times, most recently from 51ff79a to d3e915f Compare August 2, 2021 23:25
@TokageItLab TokageItLab marked this pull request as ready for review August 2, 2021 23:25
@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from d3e915f to 7864107 Compare August 2, 2021 23:28
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 2, 2021

Now I think I finished to fix the multiple-global-transform to keep the previous behavior (global-rotation is changed to not implicitly change the local scale in this PR). And, if you have any suggestions on how to make multiple-local-transform work, please let me know.

@TokageItLab TokageItLab marked this pull request as draft August 3, 2021 03:26
@TokageItLab
Copy link
Member Author

I found a bug in local-rotation, so I'm fixing it..

@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from 7864107 to e0dbcae Compare August 3, 2021 04:40
@TokageItLab TokageItLab marked this pull request as ready for review August 3, 2021 05:02
@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from e0dbcae to 31cfddd Compare August 3, 2021 05:10
@TokageItLab TokageItLab marked this pull request as draft August 3, 2021 05:15
@TokageItLab TokageItLab marked this pull request as ready for review August 3, 2021 05:17
@TokageItLab TokageItLab force-pushed the fix-gizmo-transform-scaling branch from 31cfddd to 4822499 Compare August 3, 2021 05:17
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 3, 2021

Most of the bugs have been fixed.

The only remaining problem is that the child nodes with scaled parent nodes have shaky local Y-axis rotation. Oddly enough, the problem does not occur in the X and Z axes rotation. Does anyone know anything about it? I suspected it had something to do with euler conversion order or making axis-angle process or... but that may not be the case.

2021-08-02.5.58.00-1.mov

In blender, the gizmo itself is not orthogonal in this case. So the shaking problem is probably due to the fact that Godot's gizmo system tries to keep orthogonality.

スクリーンショット 2021-08-03 15 13 33

This is probably inevitable when we rotate a skewed matrix around a particular axis. In Godot, it may be possible to rotate the matrix as it looks in gizmo if we pass gizmo's normal, but that will not rotate the matrix correctly. For example, if you grab the green ring that means the Y axis and rotate it as it looks, it is not a rotation of the Y axis, and it will not be consistent with multiple-local-rotation.

In the skewed node, I think it would be easier for the user to know that skew is occurring if the gizmo is not keep orthogonal, so I think we should change it.

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.

I assume the original reason for normalizing during rotation was to prevent this shakiness. There may be another way to fix it by taking the non-uniform scaling when computing p_motion and local_motion, but since rotating nodes in a non-uniform coordinate space is not very common, I think we can merge and test this as-is and investigate later if it proves to be problematic.

Going with a non-orthogonal gizmo like Blender is also an option, but outside the scope of this PR.

@akien-mga akien-mga merged commit dcf2a62 into godotengine:master Aug 5, 2021
@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.

4 participants