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

Make Path3D handles visible and consistent with 2D. #43408

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Nov 9, 2020

Fixes godotengine/godot-proposals#1246.

It is difficult to tell the difference between the handles for adjusting
curves and the points themselves when looking at a Path gizmo.

f1b35cb8ab introduces a public API change to allow specifying a custom texture
for a handle. I can find another approach if this isn't ideal, but it seemed generally useful.

02f3d271d8 is the main change, which uses the Path2D icons for Path3d.

0f5ba127c2 resizes the icons to the standard 16x16 (they were 10x10).

Unlike Path2D, this does not use a different icon for smooth vs sharp
points, as using a potentially different material for each point would
prevent batching the points in add_handles (and adding them out-of-order
messes up other logic based on handle indices).

@rcorre rcorre requested a review from a team as a code owner November 9, 2020 12:24
@akien-mga akien-mga added this to the 4.0 milestone Nov 9, 2020
@akien-mga akien-mga requested review from JFonS, Calinou and groud November 9, 2020 12:26
@@ -6807,7 +6807,7 @@ void EditorNode3DGizmoPlugin::_bind_methods() {

ClassDB::bind_method(D_METHOD("create_material", "name", "color", "billboard", "on_top", "use_vertex_color"), &EditorNode3DGizmoPlugin::create_material, DEFVAL(false), DEFVAL(false), DEFVAL(false));
ClassDB::bind_method(D_METHOD("create_icon_material", "name", "texture", "on_top", "color"), &EditorNode3DGizmoPlugin::create_icon_material, DEFVAL(false), DEFVAL(Color(1, 1, 1, 1)));
ClassDB::bind_method(D_METHOD("create_handle_material", "name", "billboard"), &EditorNode3DGizmoPlugin::create_handle_material, DEFVAL(false));
ClassDB::bind_method(D_METHOD("create_handle_material", "name", "billboard"), &EditorNode3DGizmoPlugin::create_handle_material, DEFVAL(false), DEFVAL(Variant()));
Copy link
Member

Choose a reason for hiding this comment

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

You need to add the "texture argument" there.

@groud
Copy link
Member

groud commented Nov 9, 2020

Looks good, but maybe the three commits should squashed together. I let @akien-mga decide on that. :)

@JFonS
Copy link
Contributor

JFonS commented Nov 10, 2020

Changes are good, but I would also squash them into a single commit :)

@rcorre
Copy link
Contributor Author

rcorre commented Nov 11, 2020

Squashed. For future reference, do reviewers prefer that contributors continually squash during review, or avoid force pushes and perform a squash merge at the end?

@Calinou
Copy link
Member

Calinou commented Nov 11, 2020

Squashed. For future reference, do reviewers prefer that contributors continually squash during review, or avoid force pushes and perform a squash merge at the end?

We don't use GitHub's Squash and merge feature in the main repository, since it removes the merge commit. We'd prefer keeping the merge commit in the main repository.

However, in other repositories like godot-docs and godot-demo-projects, we can use Squash and merge if needed since we're not as strict with the Git history there.

Other than that, I don't think we specifically have a preference during the review process.

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.

LGTM

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Sorry for the two step review, was on phone yesterday and couldn't review in depth.

Should be good to merge after the last comments are addressed.

editor/plugins/node_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
doc/classes/EditorNode3DGizmoPlugin.xml Outdated Show resolved Hide resolved
Resolves godotengine/godot-proposals#1246.

It is difficult to tell the difference between the handles for adjusting
curves and the points themselves when looking at a Path gizmo.
This re-uses the icons used for Path2D.

Unlike Path2D, this does not use a different icon for smooth vs sharp
points, as using a potentially different material for each point would
prevent batching the points in add_handles (and adding them out-of-order
messes up other logic based on handle indices).

This includes a public API change to allow specifying a texture for a
handle material. This allows spatial gizmo plugins to customize the way
a handle is rendered, if desired, but does not break existing behavior
(as providing no texture uses the default).

The path handle icons were resized as well.  16x16 is the standard icon
size. These icons were 10x10 rather than 16x16, and appeared rather
small in the editor.

To resize, I:

- Opened the original in Inkscape
- Resized the document to 16x16
- Opened the transform dialog
- Scaled by 160% proportionally
- Used Align/Distribute to center on the page
- Saved the document
- Cleaned with `svgcleaner --multipass`
@akien-mga akien-mga merged commit a7d610d into godotengine:master Nov 12, 2020
@akien-mga
Copy link
Member

Thanks!

@rcorre rcorre deleted the path-gizmos-4.0 branch November 12, 2020 12:17
@DeerTears
Copy link

Thanks so much for taking my proposal! Very looking forward to using 3D Paths again with 4.0 👏

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.

Make it possible to distinguish Curve3D handles from Curve3D points (Path 3D)
6 participants