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

Fixed Pos/Rot/Scl 3D Tracks insertion in SkeletonEditor #53885

Merged

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Oct 16, 2021

Fixed Pos/Rot/Scl 3D Tracks insertion in SkeletonEditor to be adopt #53689. Also, implemented to pop up a dialog when inserting multiple tracks with SkeletonEditor.

スクリーンショット 2021-10-17 0 56 11

I also added Animation::find_tracks(), since we now have multiple (Pos/Rot/Scl) tracks with the same path name. However, now it coexists with Animation::find_track(), so if we are not considering performance, we may as well unify Animation::find_track() into Animation::find_tracks() for maintainability.

By the way, anyone mentioned about the recording-mode in the 2D animation feature, but I think it can be added as another enhancement PR to apply it to 3D later.

@TokageItLab TokageItLab requested review from a team as code owners October 16, 2021 15:56
@Calinou Calinou added this to the 4.0 milestone Oct 16, 2021
@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch from 2771ec1 to 5b417bc Compare October 16, 2021 16:03
@akien-mga akien-mga requested a review from reduz October 16, 2021 16:13
@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch from 5b417bc to a6d5662 Compare October 16, 2021 16:21
@TokageItLab TokageItLab requested a review from a team as a code owner October 16, 2021 16:21
@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch 2 times, most recently from a737906 to 32f62b7 Compare October 16, 2021 19:56
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 16, 2021

@lyuma want to get rid the popup, so I'm working on changing UI to popup's toggle into the button above and moving rest option. Probably the method itself will not change significantly, so this can be reviewed at the same time.

@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch 2 times, most recently from 061da57 to c1052f4 Compare October 16, 2021 22:27
@TokageItLab TokageItLab requested a review from a team as a code owner October 16, 2021 22:27
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 16, 2021

Okay, it seems to be organized.

2021-10-17.7.29.33-1.mov

The reason there are two types of keying in 3DSkeleton is that, unlike 2DNodes, there are some cases where we need to avoid adding unnecessary tracks. For example, you basically don't want to insert tracks for hair or clothing bones controlled by spring bone addon, or for bones controlled by IK. Alternatively, it may be better to have the property of "keyable" on the bone itself, but for now, I am assuming the use case of generating all the bones at once, deleting the tracks that are not needed, and inserting keys only for the tracks that already exist.

Also, rest options has been moved into skeleton options, if there are more options related to rest, consider moving to another location.

スクリーンショット 2021-10-17 7 31 33

@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch 3 times, most recently from db5547c to ebfa42f Compare October 17, 2021 14:43
@TokageItLab
Copy link
Member Author

Added some shortcuts. Key insertion defaults to the "Insert" key which same as in 2D, but no default shortcuts are assigned for Init-Pose and Pose-to-Rest others due to consider to conflicts.

Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Seems good overall to me.

Vector<int> Animation::find_tracks(const NodePath &p_path) const {

The only place I see this used is to search for a specific track by type (e.g. TYPE_ROTATION_3D). Is there ever a case where you have multiple tracks with the same path and the same type?

Maybe find_track_with_type() would be better?

@TokageItLab TokageItLab requested a review from a team as a code owner October 18, 2021 23:19
@TokageItLab
Copy link
Member Author

@lyuma Make sense. The previous find_track() is now unsafe and we should to replace it.

@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch 2 times, most recently from 01f4f42 to 70fad87 Compare October 18, 2021 23:27
@TokageItLab
Copy link
Member Author

Replaced all previous Animation::find_track(const NodePath &p_path) with Animation::find_track(const NodePath &p_path, const TrackType p_type).

@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch from 70fad87 to 74c2801 Compare October 23, 2021 07:41
@TokageItLab TokageItLab force-pushed the fix-bone-animation-insertion branch from 74c2801 to 653e2a5 Compare October 23, 2021 07:50
@akien-mga akien-mga merged commit c7b78b9 into godotengine:master Oct 24, 2021
@akien-mga
Copy link
Member

Thanks!

@TokageItLab TokageItLab deleted the fix-bone-animation-insertion branch September 16, 2022 21:02
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