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

Fixed interpolation for discrete types #53548

Closed
wants to merge 1 commit into from

Conversation

DavidSichma
Copy link
Contributor

@DavidSichma DavidSichma commented Oct 7, 2021

This fixes the animation player interpolating discrete types like Vector2i.

int: int is now interpolated using Math::round() instead of standard cast to int.
This aligns better with real types. Also only (-.5,0.5) is mapped to 0 instead of (-1,1) so interpolation is more 'linear' around zero.

Other types have their formula corrected. They use the same rounding behavior as int for consistency.

Somewhat unrelated: Should Variant::blend() be dropped? I can not find any uses in the engine, but it is exposed to GdNative. It does less than Variant::interpolate() and contains more bugs.

Edit: For testing be sure to interpolate values other than 0, otherwise the changes aren't visible.

discrete interpolation now uses Math::round to closely match real types.
fixed interpolation formula for discrete types other than int.
@DavidSichma DavidSichma requested a review from a team as a code owner October 7, 2021 21:28
@YeldhamDev YeldhamDev added this to the 4.0 milestone Oct 8, 2021
@fire fire requested a review from a team October 8, 2021 15:16
@akien-mga
Copy link
Member

I believe this might have been superseded by some PRs by @TokageItLab, could you confirm?

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 12, 2023
@TokageItLab
Copy link
Member

TokageItLab commented Feb 12, 2023

I refactored it, but the algorithm was still retained, so I don't think it is a replacement for this PR.

At a quick look this change seems to make sense. But we need to test how ilerp is done elsewhere and whether this will cause problems (well, still I feel positive towards this change).

@TokageItLab
Copy link
Member

TokageItLab commented Feb 12, 2023

cc @KoBeWi I think this relates to also tween.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 12, 2023

Tweens have a problem (which is not a bug) with interpolating sprite's frames (although it's the same with AnimationPlayer). When you interpolate from 0 to last_frame - 1, the last animation frame will be displayed for one render frame before the interpolation ends (which is not ideal when you want to e.g. free the node after animation).

This short code showcases the issue:

var tween = get_editor_interface().get_edited_scene_root().create_tween().set_process_mode(Tween.TWEEN_PROCESS_PHYSICS)
tween.tween_method(func(v): print(v), 0, 10, 1)

The output is:

0
0
0
0
0
1
1
1
1
1
1
2
2
2
2
2
2
3
3
3
3
3
3
4
4
4
4
4
4
5
5
5
5
5
5
6
6
6
6
6
6
7
7
7
7
7
7
8
8
8
8
8
8
9
9
9
9
9
9
10

Notice how 10 appears once.

I tested how this behaves with ilerp:

0
0
1
1
1
1
1
1
2
2
2
2
2
2
3
3
3
3
3
3
4
4
4
4
4
4
5
5
5
5
5
5
6
6
6
6
6
6
7
7
7
7
7
7
8
8
8
8
8
8
9
9
9
9
9
9
10
10
10
10

The first frame appears less times and the last frame appears more times.

While it's still not perfect, it improves the usability a bit I guess.
(by perfect I mean each number should appear same amount of times, but that's not mathematically correct how interpolation works, probably)

@TokageItLab
Copy link
Member

TokageItLab commented Feb 12, 2023

Ah indeed. At least as far as we think about int, the current master's code may be more ideal for controlling frames in the timeline for AnimatedSprite, since the borders are clearer.

But for simple interpolation of values, a transition like this PR would be preferable. Well, allowing them to be switched would be ideal, but seems a bit too expensive. It is not impossible to do such a transition only for Vector2i, but it does not seem like a good idea from a consistency standpoint...

It might be better to leave them as they are for now.

@TokageItLab
Copy link
Member

Superseded by #84815.

@TokageItLab TokageItLab removed this from the 4.x milestone Dec 30, 2023
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.

6 participants