Improve slider ball and follow circle animations #14832
Conversation
| @@ -207,73 +186,35 @@ public void UpdateProgress(double completionProgress) | |||
| lastPosition = newPos; | |||
| } | |||
|
|
|||
| private class FollowCircleContainer : CircularContainer | |||
| private class FollowReceptor : CircularContainer | |||
There was a problem hiding this comment.
What was the reasoning for this FollowReceptor change? It seems unrelated to the actual goal of this pull.
(I also slightly dislike that it scrapes out the slider's ball the way it does, rather than the slider caching the ball explicitly and this component resolving it explicitly, but I suppose it's not horrible.)
There was a problem hiding this comment.
So FollowReceptor is separate from the actual follow circles because this pull request is supposed to only affect the animations of legacy skins. But because the old follow areas were tied directly to the follow circles themselves, that would mean legacy skins (with their different animations) would have different follow areas than the default skin. FollowReceptor has the same animations as the default skin, so they match perfectly. Legacy skins will be a little off, but that's inevitable if legacy skins and default skin are going to have different animations.
| if (slider.Ball.InputTracksVisualSize) | ||
| { | ||
| if (tracking) | ||
| this.ScaleTo(DrawableSliderBall.FOLLOW_AREA, 200, Easing.OutQuint); | ||
| else | ||
| this.ScaleTo(DrawableSliderBall.FOLLOW_AREA * 2, 100, Easing.OutQuint).Then().ScaleTo(1f); | ||
| } |
There was a problem hiding this comment.
This is weird. The logic is conditioned upon InputTracksVisualSize. But the DrawableSliderBall's input receptor does not scale in the same manner. So am I to understand input will actually not track visual size on legacy skins?
I almost think that skinnable components handling InputTracksVisualSize is wrong and it's actually DrawableSliderBall that should be figuring that logic out.
There was a problem hiding this comment.
(Might be misunderstanding the question?) Yeah, legacy skins won't match the actual follow area. In stable, there's an animation were the follow circle goes crazy huge when you let go, but I don't think the follow area scales outwards like that too (correct me if I'm wrong). Default/legacy skin will have different animations, and I don't think it makes sense to give them differently animated follow receptors. One of the two skins is going to have slightly unrepresentative animations. It's weird, but I couldn't think of a better solution.
There was a problem hiding this comment.
if the goal is to make animations work 1:1 (as opposed to actually improving the gameplay readability as was done before this PR), the legacy skin shouldn't be checking this variable at all.
i'm still not sure and not going to review this PR until i am, though.
There was a problem hiding this comment.
Given the explanation above I'm actually starting to (personal opinion follows) slightly oppose bringing back the legacy follow circle animation at all. It just complicates things needlessly and brings back some of the weird obscure mechanics that stable has, that in a nice world have no right to exist but people also got used to.
As things are right now on master, there are two simple options: either the follow area is exactly as you see it, or it is a constant size (equal to the one at end time). This pull makes it so that is the case kinda, only on the default skin, and on legacy skins the actual follow circle never follows the animation, but is either smaller than it and still changing size in a manner invisible to the player, or constant size. Try explaining that one to a beginner player, it just sounds insane.
There was a problem hiding this comment.
That's pretty fair, but at the very least the follow circle fade out animation that plays at the end of a slider (not the one when the player lets go early) should follow legacy animations, since it comes after slider judgement so it doesn't matter if it matches follow area, right?
There was a problem hiding this comment.
Sure, I could agree with that part of it.
|
Aside from the above review please run inspectcode locally as there are two inspections to fix. |
… disposal Implement trivial requested and InspectCode changes
|
Currently have unmerged changes (diff) that basically go all-in on making legacy followcircle anims 1:1 with stable. Not sure if they should be merged here because this PR was originally meant to focus on fade out anims. Also, there's still that unresolved dilemma (#14832 (comment)) of how closely legacy anims should match stable, as the more they're 1:1, the less accurately the followcircle represents true follow area while those anims are playing. Changes in the unmerged branch:
Comparison video with unmerged changes: |
Attempts to more accurately match some of stable's animations (#13660), specifically slider ball and follow circle fade out. On stable, slider balls disappear instantly, and follow circles fade out way slower.
Also, stable's follow circles are noticeably smaller, though this part of the PR I'm less sure about because it changes follow circles from 2.4x to 2.0x size, actually affecting gameplay (I think?). I'm not even sure if 2.0x is the right number, since follow circles on stable do a bouncing animation.Comparison video for legacy skins:
https://www.youtube.com/watch?v=tNnfiz4EOG4https://www.youtube.com/watch?v=sn6oaPXcbkM
Comparison video for default skin (which should not be changed by this PR):
https://www.youtube.com/watch?v=T6cHvewvkcI