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

Add handles to control Curve3D tilt #80329

Merged
merged 1 commit into from Aug 17, 2023

Conversation

xiongyaohua
Copy link
Contributor

@xiongyaohua xiongyaohua commented Aug 6, 2023

This PR add handles to control Curve3D tilt. The implementation of in, out handles are also revamped, as the original ones are too hacky, specifically the mapping between handle ids and control point ids.

tilt_handle_showcase.mov

The behaviour of this PR is similar with PR #68873, but disks are added to indicate the tilt handles trajectories, and an blue radial line is add to indicate the "natural" up vector before tilt.

An internal helper method Curve3D::_get_posture_at_control_point() is added to facilitate tile handle implantation, it could be promoted to an public method if someone found it useful.

When a Path3D is modified by handles, the transform of PathFollow3D is not updated, hence the attached camera view also doesn't change. This issue is fixed by a related PR #80233, which makes tweaking camera easier.

editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

There are review comments that are hidden under the "conversations hidden" that you have missed

Please try to apply them as a batch, to prevent restarting CI several times

@xiongyaohua
Copy link
Contributor Author

Please try to apply them as a batch, to prevent restarting CI several times

Done. Thanks for the tip! not a regular user of Github.

@akien-mga
Copy link
Member

CC @TokageItLab @ajreckof who have been involved in reviewing or improving this plugin recently, IINM.

editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/path_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
@xiongyaohua xiongyaohua force-pushed the path3d_tilt_gizmo branch 4 times, most recently from 5c18a9c to 0e355ce Compare August 10, 2023 06:56
scene/resources/curve.h Outdated Show resolved Hide resolved
scene/resources/curve.cpp Outdated Show resolved Hide resolved
scene/resources/curve.h Outdated Show resolved Hide resolved
scene/resources/curve.cpp Outdated Show resolved Hide resolved
@xiongyaohua xiongyaohua force-pushed the path3d_tilt_gizmo branch 2 times, most recently from ea6658e to a0e26f1 Compare August 11, 2023 05:19
scene/resources/curve.h Outdated Show resolved Hide resolved
scene/resources/curve.h Outdated Show resolved Hide resolved
scene/resources/curve.h Outdated Show resolved Hide resolved
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

LGTM, the code style problems were removed and seemed to work fine when I tested.

As for the feedback of usability, I suggest that the TiltHandle should be a different color than the InOut Handle (e.g. yellow line). Also it would be better to be able to change the radius from EditorSetting. Well, we can send the PR for these improvements later.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Code style looks good

@xiongyaohua
Copy link
Contributor Author

xiongyaohua commented Aug 17, 2023

Hi @akien-mga, is it possible to merge this PR earlier than 4.x? I'd like to start working on this proposal and usability improvement mentioned in @TokageItLab's review comment. But they depend on changes in this PR.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 17, 2023
@akien-mga akien-mga merged commit 446dfdb into godotengine:master Aug 17, 2023
15 checks passed
@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.

None yet

5 participants