Add box scaling to value tracks

[Arnklit]

This adds box scaling to the Value Tracks Editor, similar to what was added to the Bezier Track Editor
This would resolve godotengine/godot-proposals#3532

godot.windows.editor.dev.x86_64_CuTQ7kKVd0.mp4

• hide the lines if they are outside the range
• make the scale be offset by 12 pixels so it stays offset like the handle is
• make inverted scaling work
• make the handle offset correct when doing inverted scaling
• update the lines connecting identical keys when scaling
• adjust the rects so they are 5 pixels pushed away from the keys so it's easier to move the keys.
• key snapping
• fix scale and move cancelling
• make the edit -> scale options work again.
• fix inverted commit is broken
• hide and show handles correctly when scrolling sideways
• snap the handles
• fix zooming the timeline

This ended up being a bit of a bigger PR than I had hoped, so it needs a thorough review. I'll set it for draft now until I've had the chance to give it a thorough lookover myself.

---

Edit: OK this should be ready for a review.

[Arnklit]

I realise now that this might collide with the work in #105151

I had not realised someone else was working on this.

EDIT: Having a look at that PR, it looks like it was decided to go with an implementation more similar to what I have done here, so it might make sense to supercede.

[Arnklit]

Based on some feedback I got from @mihe on chat I did a few fixes. I also rebased it again.

A few to notes for anyone reviewing this:

1. I had to subtract 16px for the right edge of the editor to get the alignment right in this line, I'm not sure why, so if anyone can give any suggestions on how to fix that: This was resolved

2. The signal sent for horizontal scrolling did not work, it was never emitted from scroll so I hooked it up from hscroll instead. I believe this is now correct, but if anyone wonders why I made a change there, that's why.

hscroll->connect(SceneStringName(value_changed), callable_mp(this, &AnimationTrackEditor::_h_scroll_changed));

https://github.com/Arnklit/godot/blob/1135d7d09b3fb84de8f38b7d8c9d95a57ab86bd6/editor/animation/animation_track_editor.cpp#L8165

3. I went for drawing the scale handle lines yellow rather than blue because the box selection was blue as well. It's now become grey with the new theme, so maybe blue is fine, but the selected keys are also blue, so it felt nice to have it stand out a bit. I'm happy to go with whatever the animation / UX team thinks is best, I just liked the yellow. I changed these to the blue accent color as requested by Calinou.

[Calinou]

Tested locally, it works as expected (including undo/redo).

In terms of UX, I like how it works.

Visually, I would prefer the scaling lines to match the accent color (blue) so it feels more in line with the selected keyframe colors. Also, I think the lines could stand be made a tad thinner (one pixel thinner than currently would be good). They can easily overlap most of a keyframe icon right now:

The change would be visual only, it wouldn't impact the "hitbox" of the line so to speak.

I noticed two issues:

• Editing is still allowed in read-only (i.e. imported) Animation resources, even though dragging individual keyframes is already prevented:

animation_box_scale_read_only.mp4

• When scaling down to a near-zero value, the selection will constantly flicker back-and-forth. This occurs both with snapping enabled and disabled, but it's more noticeable with snapping enabled as shown here:

animation_box_scale_flicker.mp4

[Arnklit]

@Calinou thanks for the review. I changed the lines to blue and made it thinner and fixed the read-only issue and the issue with flickering when scaling close to 0.

godot.windows.editor.dev.x86_64_cRYUSFIuu9.mp4

I have however discovered a new bug that happens when scaling multiple keys in the same track negatively. Some of the keys don't render due to the code that helps make sure that the function name strings render correctly when moving keys around. I'm working on fixing it. Fixed

EDIT: I see now as well watching over that video that the cursor is no longer correctly alligned when scaling negatively due to one of the fixes I made, so I'll need to fix that as well. Fixed

[Arnklit]

I believe I fixed the issue I was seeing before. The issue is in master as well and has to do with how the "string limit" is calculated for when to clip a keys rendering based on the next key, especially important for the function keys. The issue as far as I could tell was that limit was found by searching the next two keys, but didn't take into account that the order of the key could be completely different during a transformation event.

Current Master behavior when moving keys can cause other keys to hide:

godot.windows.editor.dev.x86_64_iWSjAQEiwh.mp4

The code that calculated the limit:

int limit_string = (editor->is_key_selected(track, i + 1) && editor->is_moving_selection()) ? int(offset_last) : int(offset_n);
if (editor->is_key_selected(track, i) && editor->is_moving_selection()) {
	limit_string = int(MAX(limit_end, offset_last));
}

My solution, which does require a loop:

float next_visual_pos = limit_end;
if (editor->is_moving_selection() || editor->is_scaling_selection()) {
	for (int j = 0; j < animation->track_get_key_count(track); j++) {
		if (j == i) {
			continue;
		}
		float test_offset = animation->track_get_key_time(track, j) - timeline->get_value();
		test_offset = test_offset * scale + limit;
		if (test_offset > offset && test_offset < next_visual_pos) {
			next_visual_pos = test_offset;
		}
	}
	next_visual_pos = MIN(next_visual_pos, limit_end);
} else {
	next_visual_pos = offset_n;
}
int limit_string = int(next_visual_pos);

The new correct behaviour:

godot.windows.editor.dev.x86_64_itxib0V1wl.mp4

[Calinou]

Works great now 🙂 Code looks good to me.

I think it's good to go from an UX perspective.

[fire]

I don't know if there is an animation meeting this Friday because of thanksgiving. @lyuma @TokageItLab but maybe we can check and approve async.

[Arnklit]

I ended up going through the PR on more time with @mihe and making a handful of other small improvements and rebasing it. I think it's in a really good place now if the Animation team can look it over in their upcoming meeting.

The biggest change was that we changed how the limit_string is calculated again and simply don't show the function names when any key on that track is being moved, since the code was complicated and never worked perfectly for updating the clipping while moving keys anyway.

[Arnklit]

Fixed failed check due to shadowed variable.

[antimundo]

This looks good to me from a UX perspective. I haven't seen the code tho.

[Arnklit]

Setting this back to draft as there is a good bit of work left to do with some of the stuff Tokage pointed out. Hopefully I'll find some time to work on it soon :)

[Arnklit]

@TokageItLab I believe I resolved all the issues now. The only issue I have not resolved is that you can still scale keys into negative space, I feel like its a bit outside this PR to block transforming keys into negative space, but if you feel it makes the most sense that I include that in the PR I'll add that in for both scaling and moving.

[Arnklit]

I've rebased this.

Note I did some related fixes in #113903 that got merged into 4.6.

This should be ready for a re-review @TokageItLab when you have time. I think it's in a really good state now.

EDIT: The one additional force push I did after this was a small change of a Vector into a LocalVector as it wasn't used anywhere else.

[fire]

Animation Meeting Discussion:

@TokageItLab Will review and report back the next animation meeting.

[Arnklit]

rebased

[Arnklit]

Rebased and fixed an alignment issue.

The alignment issue came from #114634

The issue seems to have arisen from margins being added to the timeline so the timeline control and the box_selection_container no longer align on the x-axis.

I made a hacky fix. that just finds the margin by comparing the two.

const float timeline_margin_left = timeline->get_global_position().x - box_selection_container->get_global_position().x;
...
key_rect.position.x = offset + timeline_margin_left;

https://github.com/Arnklit/godot/blob/7ded43c5ce8ffaaa309e70a508516d49a356a245/editor/animation/animation_track_editor.cpp#L6363-L6374

@YeldhamDev would you have a moment to look this over and see what the best way to make sure all of this aligns correctly? Maybe the way I set up the scale_control can be changed around or maybe this margin can just be safely fetched from the theme and used, but I am very unsure of the best way to do this and how to make sure it doesn't break with RTL.

[YeldhamDev]

First of all, the best way to get the value of those margins correctly, even with RTL, is by simply repeating what the _update_timeline_margins() function does to fetch them:
https://github.com/godotengine/godot/pull/114634/changes#diff-65574de4f5d5144beb709666187e245933e5d9d962ca180a367fd1b116528021R7885

Second, if necessary, you could simply make so that box_selection_container is resized to fit those margins. They are just borders, so no need for those areas to be selectable.

[Arnklit]

Sorry, putting this back to draft, I keep finding small issues that cropped up after the last upstream changes. I will set it back to ready for review and ping Tokage when I've resolved them.

[Arnklit]

OK, I have fixed all misalignment issues as far as I can tell. @TokageItLab if you have a chance to take another look? I don't know if it's possible for this to make it into 4.7, but I'd love to try and aim for that so I'll try and resolve any other issues raised as quick as possible.