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

Improvements to 3D editor and gizmos #49907

Closed
wants to merge 1 commit into from

Conversation

JFonS
Copy link
Contributor

@JFonS JFonS commented Jun 25, 2021

Refactor the 3D editor selection code as well as the gizmo handle system.

Selection of nodes in the 3D viewport should now be more consistent with how selection works in the 2D viewport, and gizmos should be correctly updated when multiple nodes are selected.

The new gizmo handles can be added in separate groups, and an identifier can be set for each individual handle, making working with handles much easier.

This commit also adds the possibility of adding transform handles, which the user can select and modify using the 3D transformation tool. This is not used in any gizmo yet, but opens up the door for improvements to many of the current gizmos.

Note: this PR will most likely generate conflicts with #48307 and #49783, but since this is a bigger refactor, it would be great if we can merge this first. I can take a look at rebasing the other PRs if needed.

@JFonS JFonS added this to the 4.0 milestone Jun 25, 2021
@JFonS JFonS requested review from a team as code owners June 25, 2021 13:14
@Calinou
Copy link
Member

Calinou commented Jun 25, 2021

Does this PR make it possible to have CSG gizmos that move only one side of a box instead of moving both sides at once?

@JFonS
Copy link
Contributor Author

JFonS commented Jun 25, 2021

Does this PR make it possible to have CSG gizmos that move only one side of a box instead of moving both sides at once?

I think this is more of a matter of the CSG nodes themselves and the fact that the box is always centered in the node origin, otherwise we need to move the node itself using a handle and that doesn't seem right in terms of usability.

@JFonS JFonS force-pushed the gizmo_improvements_bis branch from 9137b10 to e3d737a Compare June 25, 2021 13:58
Refactor the 3D editor selection code as well as the gizmo handle
system.

Selection of nodes in the 3D viewport should now be more consistent
with how selection works in the 2D viewport, and gizmos should be
correctly updated when multiple nodes are selected.

The new gizmo handles can be added in separate groups, and an identifier
can be set for each individual handle, making working with handles much
easier.

This commit also adds the possibility of adding transform handles, which
the user can select and modify using the 3D transformation tool. This is
not used in any gizmo yet, but opens up the door for improvements to
many of the current gizmos.
@@ -2979,11 +3418,11 @@ void DecalGizmoPlugin::set_handle(EditorNode3DGizmo *p_gizmo, int p_idx, Camera3
Vector3 sg[2] = { gi.xform(ray_from), gi.xform(ray_from + ray_dir * 16384) };

Vector3 axis;
axis[p_idx] = 1.0;
axis[p_id] = 1.0;
Copy link
Member

Choose a reason for hiding this comment

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

Is it an improvement to rename this from idx to id? It seems like this number is the index of the edited axis. Also, if this value is always to going to be an axis index, it should be of the enum type Vector3::Axis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed all the p_idx arguments to p_id because now the gizmo system will allow you to define a custom ID for each handle. In this gizmo it's just using the index, but I renamed all the occurrences for consistency.

bool move = p_idx >= 3;
p_idx = p_idx % 3;
bool move = p_id >= 3;
p_id = p_id % 3;
Copy link
Member

@aaronfranke aaronfranke Jun 26, 2021

Choose a reason for hiding this comment

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

Here can be >= 3, so we can't use Vector3::Axis to store all values, but I'm wondering if instead that we should make moving or not be a boolean or enum that indicates the type of action being taken instead of using a second set of values for the axis index (3 to 5).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "group ID" feature added in this PR would help with that, since you can have a separate group for moving and resizing, but after some discussion with Juan it's going to be removed in order to keep things simpler to use.

@JFonS
Copy link
Contributor Author

JFonS commented Jun 28, 2021

After some discussion with Juan on RocketChat we decided to go a slightly different route with the gizmo changes. This PR will be used as a base, but it doesn't make much sense to keep it open until I'm done with the new version, so closing for now.

@JFonS JFonS closed this Jun 28, 2021
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.

3 participants