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 bezier handle type a property of keyframes, update UI #53298

Conversation

NathanLovato
Copy link
Contributor

@NathanLovato NathanLovato commented Oct 1, 2021

  • Remove unused code related to old close icon (now replaced with a button)
  • Add bezier handle options to right-click menu
  • Provide optional scale for balanced handle move
  • Remove mirror handle mode, only keep balanced. The reason is it's not one you're likely to use more than rarely. I've only seen this in Godot when it comes to animation. Balanced keeps things aligned so it does the job well.
  • Update animation reference

This PR implements godotengine/godot-proposals#3236

It makes the bezier handle mode a property of each bezier track keyframe, allowing to retain some behavior on the keyframes. It opens the path to having auto handles as explained by @TokageItLab here: godotengine/godot-proposals#3141 (comment)

Co-authored-by: Francois Belair razoric480@gmail.com
Co-authored-by: Gilles Roudière gilles.roudiere@gmail.com

@NathanLovato
Copy link
Contributor Author

The PR has two bugs left to address and I'd appreciate some help on those, mostly due to having very limited time to work on this (preparing a Kickstarter campaign).

  1. The handle mode appears in the inspector but when changing it, it doesn't get saved.

I clearly didn't get the setter right but I'm not sure what's wrong. Here's a video clip:

anim-property-not-changing.mp4
  1. The values applied to handles are off.

Before the PR the code to update the handles' position was inside the bezier editor. With this PR the code moves to the Animation class because we need to update the right handle when moving the left one and vice-versa.

There was code like this in the bezier animation editor to update the opposite of the edited handle in balanced mode ("mirrors" the direction of the edited handle but preserves the opposite handle's length):

Vector2 scale = Vector2(timeline->get_zoom_scale(), v_zoom);
moving_handle_right = (-(moving_handle_left * scale).normalized() * (moving_handle_right * scale).length()) / scale;

Now, the thing is, the coordinates used in the bezier editor seemed to be in different units on the X and Y axes: small on the X-axis, and larger on the Y-axis.

Now, because the calculation on moving_handle_right needs to happen in the Animation class now, I don't have access to the scale to get the editor scale and normalized handle correctly when calculating the opposite handle:

void Animation::bezier_track_set_key_out_handle(int p_track, int p_index, const Vector2 &p_handle) {
//...
	Vector2 out_handle = p_handle;
	if (out_handle.x < 0) {
		out_handle.x = 0;
	}
	bt->values.write[p_index].value.out_handle = out_handle;

	if (bezier_track_get_key_handle_mode(p_track, p_index) == HANDLE_MODE_BALANCED) {
		const Vector2 in_handle = -out_handle.normalized() * bt->values[p_index].value.in_handle.length();
		bt->values.write[p_index].value.in_handle = in_handle;
	}

I'm not sure how to best address that.

Right now it causes keyframes to behave like this:

anim-handles.mp4

@bruvzg
Copy link
Member

bruvzg commented Oct 1, 2021

The handle mode appears in the inspector but when changing it, it doesn't get saved.
I clearly didn't get the setter right but I'm not sure what's wrong. Here's a video clip:

Serialization seems to be done in the _set/_get methods, also enum and function bindings are missing. Something along these lines should fix it (I have not tested it):

diff --git a/scene/resources/animation.cpp b/scene/resources/animation.cpp
index caff93903b..73cfa33d6b 100644
--- a/scene/resources/animation.cpp
+++ b/scene/resources/animation.cpp
@@ -200,7 +200,7 @@ bool Animation::_set(const StringName &p_name, const Variant &p_value) {
 				Vector<real_t> times = d["times"];
 				Vector<real_t> values = d["points"];
 
-				ERR_FAIL_COND_V(times.size() * 5 != values.size(), false);
+				ERR_FAIL_COND_V(times.size() * 6 != values.size(), false);
 
 				if (times.size()) {
 					int valcount = times.size();
@@ -213,11 +213,12 @@ bool Animation::_set(const StringName &p_name, const Variant &p_value) {
 					for (int i = 0; i < valcount; i++) {
 						bt->values.write[i].time = rt[i];
 						bt->values.write[i].transition = 0; //unused in bezier
-						bt->values.write[i].value.value = rv[i * 5 + 0];
-						bt->values.write[i].value.in_handle.x = rv[i * 5 + 1];
-						bt->values.write[i].value.in_handle.y = rv[i * 5 + 2];
-						bt->values.write[i].value.out_handle.x = rv[i * 5 + 3];
-						bt->values.write[i].value.out_handle.y = rv[i * 5 + 4];
+						bt->values.write[i].value.value = rv[i * 6 + 0];
+						bt->values.write[i].value.in_handle.x = rv[i * 6 + 1];
+						bt->values.write[i].value.in_handle.y = rv[i * 6 + 2];
+						bt->values.write[i].value.out_handle.x = rv[i * 6 + 3];
+						bt->values.write[i].value.out_handle.y = rv[i * 6 + 4];
+						bt->values.write[i].value.handle_mode = (HandleMode)rv[i * 6 + 5];
 					}
 				}
 
@@ -470,7 +471,7 @@ bool Animation::_get(const StringName &p_name, Variant &r_ret) const {
 				int kk = bt->values.size();
 
 				key_times.resize(kk);
-				key_points.resize(kk * 5);
+				key_points.resize(kk * 6);
 
 				real_t *wti = key_times.ptrw();
 				real_t *wpo = key_points.ptrw();
@@ -481,11 +482,12 @@ bool Animation::_get(const StringName &p_name, Variant &r_ret) const {
 
 				for (int i = 0; i < kk; i++) {
 					wti[idx] = vls[i].time;
-					wpo[idx * 5 + 0] = vls[i].value.value;
-					wpo[idx * 5 + 1] = vls[i].value.in_handle.x;
-					wpo[idx * 5 + 2] = vls[i].value.in_handle.y;
-					wpo[idx * 5 + 3] = vls[i].value.out_handle.x;
-					wpo[idx * 5 + 4] = vls[i].value.out_handle.y;
+					wpo[idx * 6 + 0] = vls[i].value.value;
+					wpo[idx * 6 + 1] = vls[i].value.in_handle.x;
+					wpo[idx * 6 + 2] = vls[i].value.in_handle.y;
+					wpo[idx * 6 + 3] = vls[i].value.out_handle.x;
+					wpo[idx * 6 + 4] = vls[i].value.out_handle.y;
+					wpo[idx * 6 + 5] = (double)vls[i].value.handle_mode;
 					idx++;
 				}
 
@@ -1006,7 +1008,7 @@ void Animation::track_insert_key(int p_track, double p_time, const Variant &p_ke
 			BezierTrack *bt = static_cast<BezierTrack *>(t);
 
 			Array arr = p_key;
-			ERR_FAIL_COND(arr.size() != 5);
+			ERR_FAIL_COND(arr.size() != 6);
 
 			TKey<BezierKey> k;
 			k.time = p_time;
@@ -1015,6 +1017,7 @@ void Animation::track_insert_key(int p_track, double p_time, const Variant &p_ke
 			k.value.in_handle.y = arr[2];
 			k.value.out_handle.x = arr[3];
 			k.value.out_handle.y = arr[4];
+			k.value.handle_mode = (HandleMode)arr[5];
 			_insert(p_time, bt->values, k);
 
 		} break;
@@ -1120,12 +1123,13 @@ Variant Animation::track_get_key_value(int p_track, int p_key_idx) const {
 			ERR_FAIL_INDEX_V(p_key_idx, bt->values.size(), Variant());
 
 			Array arr;
-			arr.resize(5);
+			arr.resize(6);
 			arr[0] = bt->values[p_key_idx].value.value;
 			arr[1] = bt->values[p_key_idx].value.in_handle.x;
 			arr[2] = bt->values[p_key_idx].value.in_handle.y;
 			arr[3] = bt->values[p_key_idx].value.out_handle.x;
 			arr[4] = bt->values[p_key_idx].value.out_handle.y;
+			arr[5] = (double)bt->values[p_key_idx].value.handle_mode;
 			return arr;
 
 		} break;
@@ -1345,13 +1349,14 @@ void Animation::track_set_key_value(int p_track, int p_key_idx, const Variant &p
 			ERR_FAIL_INDEX(p_key_idx, bt->values.size());
 
 			Array arr = p_value;
-			ERR_FAIL_COND(arr.size() != 5);
+			ERR_FAIL_COND(arr.size() != 6);
 
 			bt->values.write[p_key_idx].value.value = arr[0];
 			bt->values.write[p_key_idx].value.in_handle.x = arr[1];
 			bt->values.write[p_key_idx].value.in_handle.y = arr[2];
 			bt->values.write[p_key_idx].value.out_handle.x = arr[3];
 			bt->values.write[p_key_idx].value.out_handle.y = arr[4];
+			bt->values.write[p_key_idx].value.handle_mode = (HandleMode)arr[5];
 
 		} break;
 		case TYPE_AUDIO: {
@@ -2709,6 +2714,9 @@ void Animation::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("audio_track_get_key_start_offset", "track_idx", "key_idx"), &Animation::audio_track_get_key_start_offset);
 	ClassDB::bind_method(D_METHOD("audio_track_get_key_end_offset", "track_idx", "key_idx"), &Animation::audio_track_get_key_end_offset);
 
+    ClassDB::bind_method(D_METHOD("bezier_track_set_key_handle_mode", "track", "index", "key_handle_mode"), &Animation::bezier_track_set_key_handle_mode);
+    ClassDB::bind_method(D_METHOD("bezier_track_get_key_handle_mode", "track", "index"), &Animation::bezier_track_get_key_handle_mode);
+
 	ClassDB::bind_method(D_METHOD("animation_track_insert_key", "track_idx", "time", "animation"), &Animation::animation_track_insert_key);
 	ClassDB::bind_method(D_METHOD("animation_track_set_key_animation", "track_idx", "key_idx", "animation"), &Animation::animation_track_set_key_animation);
 	ClassDB::bind_method(D_METHOD("animation_track_get_key_animation", "track_idx", "key_idx"), &Animation::animation_track_get_key_animation);
@@ -2746,6 +2754,10 @@ void Animation::_bind_methods() {
 	BIND_ENUM_CONSTANT(UPDATE_DISCRETE);
 	BIND_ENUM_CONSTANT(UPDATE_TRIGGER);
 	BIND_ENUM_CONSTANT(UPDATE_CAPTURE);
+
+    BIND_ENUM_CONSTANT(HANDLE_MODE_FREE);
+    BIND_ENUM_CONSTANT(HANDLE_MODE_BALANCED);
+    BIND_ENUM_CONSTANT(HANDLE_MODE_MIRROR);
 }
 
 void Animation::clear() {
diff --git a/scene/resources/animation.h b/scene/resources/animation.h
index 80e69c10ee..39afc1fa6d 100644
--- a/scene/resources/animation.h
+++ b/scene/resources/animation.h
@@ -367,5 +367,7 @@ public:
 VARIANT_ENUM_CAST(Animation::TrackType);
 VARIANT_ENUM_CAST(Animation::InterpolationType);
 VARIANT_ENUM_CAST(Animation::UpdateMode);
+VARIANT_ENUM_CAST(Animation::HandleMode);
+
 
 #endif

@NathanLovato NathanLovato force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch from daca3df to 077510c Compare October 1, 2021 21:02
@NathanLovato
Copy link
Contributor Author

Thanks much @bruvzg, I didn't check the Animation class for serialization. I thought it might happen in the editor classes.

@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch 2 times, most recently from 484c864 to 7ec030c Compare October 1, 2021 21:52
@Razoric480
Copy link
Contributor

Razoric480 commented Oct 1, 2021

Fixed issue 1.

For number 2, we need the animation to handle the balancing of the handles or else it defeats the purpose of putting the handles there. So the only real solution is to provide the scale when calling bezier_track_set_key_in/out_handle.

I've made this p_balanced_scale parameter optional and it defaults to Vector2(1, 1) to prevent breaking. For most purposes, using it in GDScript would not have the concept of being zoomed in or out unless they're making a custom editor (then it's there for them) so the (1, 1) default sounds about right.

More importantly we can bring it into animation.cpp using the original 4.0's (get_zoom_scale(), v_zoom) scale.

balanced.mp4

It is an API change, however, and needs to be added to the docs to indicate its purpose and how it only comes into effect when the key is balanced. But it's the simplest solution by far, and we were already changing the docs by adding the handle enum to the Animation resource.

@NathanLovato NathanLovato changed the title [WIP] Make bezier handle type a property of keyframes, update UI Make bezier handle type a property of keyframes, update UI Oct 2, 2021
@NathanLovato
Copy link
Contributor Author

Thanks much @Razoric480 I'll add the docs now to complete checks

@Razoric480
Copy link
Contributor

Razoric480 commented Oct 2, 2021

Might need to change the D_METHOD call in Animation.cpp's _bind_methods as well.

@NathanLovato NathanLovato force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch from f654619 to 352bc58 Compare October 2, 2021 01:16
@NathanLovato NathanLovato requested a review from a team as a code owner October 2, 2021 01:16
@NathanLovato NathanLovato force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch 5 times, most recently from 9d9e4ba to 32558cf Compare October 2, 2021 13:50
@NathanLovato
Copy link
Contributor Author

There's one last issue left, and this one might require a breaking change.

Changing a handle's mode from FREE to BALANCED in the inspector can cause the handles to get very long or very short. That's the issue with not having the scale I was mentioning earlier.

The underlying issue is that the handles' coordinates are very different on the X and Y axes: X has small values (in seconds?) while Y has large values (seems in pixels). So when changing the handle mode to balanced, we need to align the left handle with the right one, but because of the disparity between X and Y values, this gives unexpected results.

@NathanLovato NathanLovato force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch from 32558cf to 7d20bc8 Compare October 2, 2021 14:52
@Razoric480
Copy link
Contributor

Razoric480 commented Oct 2, 2021

The scale for handles, after investigation, is actually in (+/- amount of time offset from key, +/- number of units offset from key (the same unit the key uses)), which simplifies things a lot as we no longer need to think of zooming or the size of the bezier editor's control.

Committed a change which nips the issue noted above in the bud.

@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch 3 times, most recently from 6a00bd8 to c5ed846 Compare October 2, 2021 23:22
@NathanLovato
Copy link
Contributor Author

It's much better but not quite there yet. In balanced mode, the opposite handle should always preserve its length. When pulling the opposite handle at a high angle, currently, it lengthens quite a bit. If you make a handle almost vertical, the opposite handle shoots really far.

handles-getting-too-long.mp4

@fire
Copy link
Member

fire commented Oct 4, 2021

For the unresolved documentation error you can run --doctool.

@Razoric480
Copy link
Contributor

Edit: Oh my bad I misunderstood the issue. You want to keep the length on the other side! I understand the issue better now.

Was about to say, yeah, that was the main issue with flipping the handle around. :) The alternative is to provide the Animation resource with some largely unrelated data about the scale of the UI and that feels like the wrong approach.

@groud
Copy link
Member

groud commented Oct 20, 2021

Was about to say, yeah, that was the main issue with flipping the handle around. :) The alternative is to provide the Animation resource with some largely unrelated data about the scale of the UI and that feels like the wrong approach.

I am not really sure. I think the only think needed to be provided to the Animation resource would be the ratio between the Y-axis scale and the X-axis scale. So a simple float value. It might not be a big of a problem to add this as an optional argument to the bezier_track_set_key_in/out_handle functions I think.

My main problem with the implementation right now is that we are adding a property to each handle in the Animation resource, while this is something that will only stay usable from the editor. We usually try to avoid this and store the information elsewhere, not to fill the core classes with editor-only stuff.

So, IMHO, we should decide what kind of approach we want:

  • If we go full-editor version, then we should probably remove the handle_mode properties from handles. We would then implement an EditorInspectorPlugin that would add a field in the AnimationTrackHandle inspector. The value itself could then maybe be stored in the metadata dict of the handle (prefixed by "edit") as this is a trick we used often in the codebase (we may have to change that someday to a dedicated editor_metadata dictionary, but we have to decide later on).
  • We go full API-version, and then we add everything needed to the API to be able to handle the balanced mode correctly. This means adding the "YXscale_ratio" to the bezier_track_set_key_in/out_handle functions as I mentioned above.

I think the later is simpler to implement even if the argument is a little bit weird. I guess it can still be explained in the documentation. And even then, I am not sure there's a lot of people procedurally generating Animations like that anyway, so it can stay a little bit cryptic. ^^

@NathanLovato
Copy link
Contributor Author

Okay, I didn't know you would use some metadata in those cases. Here, other properties like the handle vectors themselves are already exposed, which is why we went this route. If it's fine we can just keep it simple and add this argument. Like you said, I don't think too many persons would use it, and I don't think too many would use or have used the current properties of those bezier keyframes.

The only case would be to make a custom in-game animation editor at runtime as there's no plugin support for the animation editor I think.

@Razoric480
Copy link
Contributor

Razoric480 commented Oct 20, 2021

Ultimately we can boil it down to two rects:

float left = timeline->get_value() / timeline->get_zoom_scale();
float right = (get_size().width - timeline->get_name_limit()) / timeline->get_zoom_scale();

float v_scale = get_size().height * v_zoom;
float top = v_scroll + v_scale / 2;
float bottom = v_scroll - v_scale / 2;

Rect2 bezier_scale = Rect2(Vector2(left, top), Vector2(right - left, top - bottom));
Rect2 pixel_scale = Rect2(Vector2(timeline->get_name_limit(), get_size().height), Vector2(get_size().width - timeline->get_name_limit(), get_size().height));

from which we can derive enough to range_lerp() with in either direction.

Vector2 pixel_lerped = Vector2(Math::range_lerp(time + out_handle.x, bezier_scale.get_position().x, bezier_scale.get_end().x, pixel_scale.get_position().x, pixel_scale.get_end().x),
	pixel_scale.size.y - Math::range_lerp(value + out_handle.y, bezier_scale.get_position().y, bezier_scale.get_end().y, pixel_scale.get_position().y, pixel_scale.get_end().y));

Vector2 bezier_lerped = Vector2(Math::range_lerp(pixel_pos.x, pixel_scale.get_position().x, pixel_scale.get_end().x, bezier_scale.get_position().x, bezier_scale.get_end().x),
	Math::range_lerp(pixel_scale.size.y - pixel_pos.y, pixel_scale.get_position().y, pixel_scale.get_end().y, bezier_scale.get_position().y, bezier_scale.get_end().y)) - bezier_pos;

@groud
Copy link
Member

groud commented Oct 22, 2021

@Razoric480 you are overcomplicating things. I made a few changes on my side, there's not need to deal with rects.
See my changes on the commit there: groud@2f5001b

As you can see, the ratio passed to the bezier_track_set_key_in/out_handle functions is simply double ratio = timeline->get_zoom_scale() * v_zoom;

You can use the commit if you want, there's still a few things to be done (including calling the function while moving the handle so that it always updates)

@Razoric480
Copy link
Contributor

Your Transform2D-fu is far superior to mine. I'll take a look, thanks.

@groud
Copy link
Member

groud commented Oct 22, 2021

Your Transform2D-fu is far superior to mine. I'll take a look, thanks.

Arf, to be fair the Transform2D stuff is maybe a little bit overkill. The idea was simply to multiply the Y component by the ratio, compute the length, then divide the Y component again.
As it's like "going into a given space, do an operation, then come back", I preferred to use transforms there as it makes this thing a little bit more obvious (xform.xform(...) and xform.affine_inverse().xform(...) are exactly this).

@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch from a5ca4af to a0052bc Compare October 22, 2021 15:44
@Razoric480
Copy link
Contributor

Razoric480 commented Oct 22, 2021

@groud There. I've implemented a version of your changes, with some missing parts (you did not pass ratio when undoing/redoing, for instance). Plus another pass with --doctool for the change in header file.

@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch 3 times, most recently from c827dcb to 76c510b Compare October 23, 2021 04:27
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Looks good to me! Good job!

@akien-mga akien-mga requested a review from a team October 23, 2021 10:24
@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch 4 times, most recently from 0a4c26b to 932206f Compare November 11, 2021 14:35
Comment on lines 430 to 431
void bezier_track_set_key_in_handle(int p_track, int p_index, const Vector2 &p_handle, const double &p_balanced_value_time_ratio = 1.0);
void bezier_track_set_key_out_handle(int p_track, int p_index, const Vector2 &p_handle, const double &p_balanced_value_time_ratio = 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.

const double & seems a bit overkill for a primitive type like this, double should be enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed, thanks.

@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch 3 times, most recently from 145a722 to d0ac2e9 Compare November 16, 2021 14:07
- Replaced unused code related to old close icon with a button
- Add bezier handle options to right-click menu
- Remove mirror handle mode, only keep balanced
- Update animation reference
@Razoric480 Razoric480 force-pushed the GDQuest/animation-bezier-editor-ui-improvements branch from d0ac2e9 to a5d0a74 Compare November 16, 2021 14:26
@akien-mga akien-mga merged commit 5045f46 into godotengine:master Nov 16, 2021
@akien-mga
Copy link
Member

Thanks a lot to all involved!

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.

9 participants