godot icon indicating copy to clipboard operation
godot copied to clipboard

Add box scaling to value tracks

Open Arnklit opened this issue 2 months ago • 10 comments

This adds box scaling to the Value Tracks Editor, similar to what was added to the Bezier Track Editor This would resolve https://github.com/godotengine/godot-proposals/issues/3532

https://github.com/user-attachments/assets/9f8b1089-41ac-47a2-9cf0-4a66c1331a23

  • [X] hide the lines if they are outside the range
  • [X] make the scale be offset by 12 pixels so it stays offset like the handle is
  • [X] make inverted scaling work
  • [X] make the handle offset correct when doing inverted scaling
  • [X] update the lines connecting identical keys when scaling
  • [X] adjust the rects so they are 5 pixels pushed away from the keys so it's easier to move the keys.
  • [X] key snapping
  • [X] fix scale and move cancelling
  • [X] make the edit -> scale options work again.
  • [X] fix inverted commit is broken
  • [X] hide and show handles correctly when scrolling sideways
  • [X] snap the handles
  • [X] 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 avatar Nov 06 '25 23:11 Arnklit

I realise now that this might collide with the work in https://github.com/godotengine/godot/pull/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 avatar Nov 06 '25 23:11 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

  1. 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.

Arnklit avatar Nov 14 '25 15:11 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.

https://github.com/user-attachments/assets/72a3e84d-e41f-4452-ab18-c995ec27db9c

~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 avatar Nov 25 '25 11:11 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:

https://github.com/user-attachments/assets/29c9f18f-86af-4097-bfc6-3ec2b2f0bdd6

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:

https://github.com/user-attachments/assets/d50a5a07-d339-49c3-bc23-3af7923995d7

Arnklit avatar Nov 25 '25 14:11 Arnklit

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.

fire avatar Nov 25 '25 17:11 fire

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 avatar Nov 27 '25 16:11 Arnklit

Fixed failed check due to shadowed variable.

Arnklit avatar Nov 27 '25 16:11 Arnklit

Thanks so much @TokageItLab ! I think it's pretty clear that this PR won't be able to make 4.6.

I will work on these issues as soon as I can :).

Arnklit avatar Dec 02 '25 16:12 Arnklit

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

antimundo avatar Dec 08 '25 14:12 antimundo

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 avatar Dec 08 '25 14:12 Arnklit