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

move_and_slide compatibility breakage between 3.0 and 3.1-alpha1 #21683

Closed
Banduck opened this issue Sep 2, 2018 · 9 comments
Closed

move_and_slide compatibility breakage between 3.0 and 3.1-alpha1 #21683

Banduck opened this issue Sep 2, 2018 · 9 comments

Comments

@Banduck
Copy link

Banduck commented Sep 2, 2018

Godot version:
v3.1.alpha.official

OS/device including version:
Windows 10 Home, Version 1803

Issue description:
This code worked in previous versions of Godot:
image
image
image
But in the current version the idle animation is not played if motion.x is set to 0.
The problem also occurs with other Lerp/move_and_slide related Code, not only with animations.
For some reason, Godot doesn't realize that the value is 0.
(In another project Lerp couldn't even set the value to 0 but only to -4 for some reason, but I can't reproduce that anymore.)

Minimal reproduction project:
Bug.zip

@akien-mga akien-mga added this to the 3.1 milestone Sep 2, 2018
@groud
Copy link
Member

groud commented Sep 2, 2018

You are comparing floating point numbers, while you can't expect perfect equality of two floating point values.
Use a delta to compare the number to, like if motion.x<0.001, or clamp the value between 0 and 0.2

@akien-mga
Copy link
Member

I can confirm the issue, though it seems to me that it was a bug that it worked properly in 3.0.

lerp(motion.x, 0, 0.2) will never reach 0, it will always cut the current motion value by 20% towards 0 but not reach it. So in that sense lerp works fine both in 3.0 and 3.1-alpha in my tests.

The difference is in move_and_slide, which in 3.0 seems to return 0 on X if passed less than 5 as input, while in 3.1 it keeps going with more precision.

In 3.0:

pre-motion: 6.184752
post-motion: 6.184752
pre-lerp: 6.184752
post-lerp: 4.947802
pre-motion: 4.947802
post-motion: 0
pre-lerp: 0
post-lerp: 0

In 3.1-alpha:

pre-motion: 6.184752
post-motion: 6.184752
pre-lerp: 6.184752
post-lerp: 4.947802
pre-motion: 4.947802
post-motion: 4.947802
pre-lerp: 4.947802
post-lerp: 3.958242
pre-motion: 3.958242
post-motion: 3.958242
pre-lerp: 3.958242
post-lerp: 3.166593
pre-motion: 3.166593
post-motion: 3.166593
pre-lerp: 3.166593
post-lerp: 2.533275
pre-motion: 2.533275
post-motion: 2.533275
pre-lerp: 2.533275
post-lerp: 2.02662
pre-motion: 2.02662
post-motion: 2.02662
pre-lerp: 2.02662
post-lerp: 1.621296

and so on and so forth.

@akien-mga
Copy link
Member

And yes as @groud mentions, if you actually want to lerp the value all the way down to 0 by cutting 20% each frame, you will never reach the integer 0, so you should compare to an epsilon - but I doubt that's the logic you want to go for anyway, as it means the friction takes quite some time to slow the player down to standstill.

@akien-mga akien-mga changed the title Lerp/move_and_slide Bug move_and_slide compatibility breakage between 3.0 and 3.1-alpha1 Sep 2, 2018
@akien-mga
Copy link
Member

I think the new behaviour is correct and this was actually fixing a bug in 3.0, but as it breaks compatibility, we need to decide how to handle it properly.

CC @reduz @AndreaCatania

@groud
Copy link
Member

groud commented Sep 2, 2018

And yes as @groud mentions, if you actually want to lerp the value all the way down to 0 by cutting 20% each frame, you will never reach the integer 0, so you should compare to an epsilon

My point was about comparing floating point values in general, but indeed in that case the code also leads you to never reach the value.

@reduz
Copy link
Member

reduz commented Sep 8, 2018

@akien-mga the old behavior was pretty broken. we are breaking compat but it was not working well before.

@akien-mga
Copy link
Member

As long as we document it properly, that's fine with me. @Calinou Could you add a mention to this in your 3.1 release notes as compatibility breakage?

@Calinou
Copy link
Member

Calinou commented Sep 11, 2018

@akien-mga I added it to the changelog 🙂

@akien-mga
Copy link
Member

Thanks, closing then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants