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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handles to control Curve3D tilt #68873

Closed
wants to merge 2 commits into from

Conversation

Ademan
Copy link
Contributor

@Ademan Ademan commented Nov 19, 2022

First PR, please be gentle, I tried to read the relevant contributing guides but I'm happy to correct anything I've missed.

This change allows interactive modification of the tilt values of a Curve3D.

Because the up vector is difficult to predict ahead of time, and changes in ways that are unintuitive (at least to me!) it was difficult to make good use of them. There's at least two of us who think this 馃槈 See #47445

This change allows for a more intuitive way to edit tilts and affect the up vector along the path. The handles point in the direction of the up vector, with tilt applied, and only appear when up vectors are enabled on the underlying curve.

2022-11-18-224955_1916x989_scrot
2022-11-18-225017_1916x989_scrot

I think its usefullness is more apparent with a CSGPolygon3D that uses the tilt values (cherry pick the tip of https://github.com/Ademan/godot/tree/wip_csg_use_up_vector and you'll get something more exciting like this:

2022-11-18-224450_1916x989_scrot

The major drawbacks of this change are

  1. More handles create more clutter
  2. These handles aren't visually distinguished from in/out handles
  3. The way this code calculates offset into the curve is prone to some inaccuracy, but it's been acceptable in my experience.

@Ademan Ademan requested a review from a team as a code owner November 19, 2022 04:56
@xiongyaohua
Copy link
Contributor

xiongyaohua commented Nov 19, 2022

I am also quite new to Godot. Here is my two cents

These handles aren't visually distinguished from in/out handles

Perhaps add a translucent desk, perpendicular to the tangent, at each control points, so the intention of the tilt handle is more clear. Something like this

Screenshot 2022-11-19 at 17 45 11

@Ademan
Copy link
Contributor Author

Ademan commented Nov 19, 2022

I like the disk idea for UI, though I do think the up vector should probably be indicated too (line + disk?)

@xiongyaohua
Copy link
Contributor

xiongyaohua commented Nov 20, 2022

I like the disk idea for UI, though I do think the up vector should probably be indicated too (line + disk?)

Agree. And perhaps the 锛坙ine + disk + tilt handle锛塩ould use a color different than default handles(white), to indicate they are in a separate group.

@Bimbam360
Copy link

Bimbam360 commented Jan 2, 2023

The real value (for me given I came looking) is in the "PATH_ROTATION_PATH_FOLLOW_TILT" addition to CSGPolygon noted in https://github.com/Ademan/godot/tree/wip_csg_use_up_vector as currently CSGPolygon seem to completely ignore tilt values.

@Ademan Is the intent to merge this also in a separate PR? or can it be included here?

@@ -448,6 +569,13 @@ EditorPlugin::AfterGUIInput Path3DEditorPlugin::forward_3d_gui_input(Camera3D *p
ur->add_undo_method(c.ptr(), "set_point_in", i, c->get_point_in(i));
ur->commit_action();
return EditorPlugin::AFTER_GUI_INPUT_STOP;
} else if (dist_to_p_up < click_dist && c->is_up_vector_enabled()) {
Ref<EditorUndoRedoManager> &ur = EditorNode::get_undo_redo();
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be changed to EditorUndoRedoManager *ur = EditorUndoRedoManager::get_singleton(); to compile with latest mainline.

Copy link
Member

@Calinou Calinou Aug 1, 2023

Choose a reason for hiding this comment

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

@Ademan Bump 馃檪

@jitspoe
Copy link
Contributor

jitspoe commented Mar 4, 2023

Seems to work pretty well from my quick testing. I don't think the handles looking the same is a deal breaker. That could potentially be adjusted in a later pass. From the context of the curve, it seems easy to tell what's what.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of dfebfd1). I like the idea, it mostly works as expected, but I have 2 noticeable issues:

  • Input seems to be reversed when I drag the tilt gizmos in the 3D scene. To rotate clockwise, I need to make counterclockwise movements with the mouse and vice versa.
  • Tilt gizmos look the exact same, both for the line and icon. This should be changed before the PR is merged, as it's too easy to confuse them for the curve smoothing ones.

That said, even with this PR, I can't quite get smooth camera movement with tilting to work. This PR will help a little, but it's still hard to control the curve editor in general (also because it's hard to see whether the curve's tilt is even the right way up 鈥撀爐he fishbones could be less "open" to improve on this.)

Testing project: 4.zip

@xiongyaohua
Copy link
Contributor

Hi, I looked over the code and feels it is too hacky, so make this PR #80329, trying to do it properly.

@Ademan
Copy link
Contributor Author

Ademan commented Aug 16, 2023

@xiongyaohua thank you for taking this idea further! I have not had any time to work on it.

@akien-mga
Copy link
Member

Superseded by #80329. Thanks for your contribution!

@akien-mga akien-mga closed this Aug 17, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Aug 17, 2023
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.

None yet

10 participants