Add box scaling to value tracks
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.
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.
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
- The signal sent for horizontal scrolling did not work, it was never emitted from
scrollso I hooked it up fromhscrollinstead. 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 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
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
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.
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.
Fixed failed check due to shadowed variable.
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 :).
This looks good to me from a UX perspective. I haven't seen the code tho.
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 :)