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

Display a matrix for Node2D #52236

Closed

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Aug 29, 2021

This was closed, but I still think this is worth having, so here's a branch: https://github.com/aaronfranke/godot/tree/node2d3d-transform


EDIT: The comments from @reduz are now out-of-date because they refer to when this PR used to change Node3D.

The matrix is now displayed for Node2D, while previously it just wasn't displayed at all. I also updated the docs for Node3D's basis property (not super related but it's a holdover from a previous version of this PR).

Before/after pic (left is before, right is after):

node2d

@reduz
Copy link
Member

reduz commented Aug 29, 2021

I am sorry for closing the previous one, I have an alternative proposal for doing this same thing that I believe makes more sense. It just takes time to write.

@groud
Copy link
Member

groud commented Aug 30, 2021

I have not a strict opinion about the changes, but I'd rather have those changes base on user needs than what we feel is right.

Regarding exposing the raw 2D transform, I don't mind, but we have to make sure this is useful. If no one end up using it, then it's bloat. I'd rather be sure this is needed exposed in the inspector.

For the 3D part, I guess the changes make sense, though I hope users are familiar enough with what a basis is. Also, @reduz pointed out on RocketChat that all of this impacts how you can animate things. If users cannot animate the whole transform easily anymore, this might be problematic.

I don't know what reduz has in mind as a proposal, but I wonder if, instead of deleting the duplicate "translation" property (as an individual property and in the matrix), if it would not be possible instead to change the inspector to provide two views (via tabs or maybe an enum ?). That could be a little bit similar to how we discussed things to be in the Control node UI rework : godotengine/godot-proposals#2841. You would then be able to switch between the position/rotation/skew view and the raw matrix view.

@reduz
Copy link
Member

reduz commented Aug 30, 2021

My idea is to instead, do it more like Blender does, and show a list of ways to show rotation and scale:
image
AxisAngle probably should not be included because its not super useful, but instead we add a Basis mode, which shows a 3x3 matrix and hides the scale.

The reason for exposing the rotation modes is that this makes it easier for animating, and also allows importing animation curves from FBX and other formats directly:

  • Choosing Euler angle order usually helps for animation
  • Having an option for quaternion helps animating rotation between two specific positions without having Gimbal lock problems.
  • Animating Basis allows to interpolate both rotation and scale, as well as inspecting the actual transform matrix used by Godot internally.

@aaronfranke aaronfranke marked this pull request as draft October 21, 2021 20:59
@aaronfranke aaronfranke changed the title Display a matrix for Node2D and don't display a duplicate origin Display a matrix for Node2D Oct 30, 2021
@aaronfranke aaronfranke marked this pull request as ready for review October 30, 2021 06:03
@aaronfranke
Copy link
Member Author

aaronfranke commented Oct 30, 2021

Rebased and updated after #54084, now it only adds the matrix to Node2D. The screenshot has also been updated.

Another option is to do a dropdown that has an option to replace rotation/scale/skew with the basis matrix like in #54084, but for 2D since there are only ever just those two things, I think it's simpler / better to just have it the way it is in this PR.

Also update Node3D basis doc
@mhilbrunner
Copy link
Member

Closing as discussed in the PR meeting for lack of consensus/support

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.

5 participants