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

Animation.get_length() doesn't return correct value #52127

Closed
floppyhammer opened this issue Aug 26, 2021 · 8 comments
Closed

Animation.get_length() doesn't return correct value #52127

floppyhammer opened this issue Aug 26, 2021 · 8 comments

Comments

@floppyhammer
Copy link
Contributor

Godot version

v4.0.dev.custom_build [6e87d62]

System information

Windows 11, Vulkan Clustered

Issue description

Animation.get_length() doesn't return correct value. The issue doesn't occur to 3.x.

for anim in anim_player.get_animation_list():
	var frame_count : float = randi() % 20
	var frame_fps : float = randi() % 10 + 1
	
	var anim_length = round(frame_count / frame_fps * 1000.0) / 1000.0
	
	anim_player.get_animation(anim).set_length(anim_length)
	
	var actual_length = anim_player.get_animation(anim).get_length()
	
	# Print A
	print("\nA: Supposed length ", anim_length, ", actual length ", actual_length)
	
	# Print B
	print("B: Supposed length %f, actual length %f" % [anim_length, actual_length])
	
	# Compare
	print("Supposed length equals to actual length: ", anim_length == actual_length)

	print(" ")

Output:

A: Supposed length 1.667, actual length 1.66700005531311
B: Supposed length 1.667000, actual length 1.667000
Supposed length equals to actual length: False

A: Supposed length 0.5, actual length 0.5
B: Supposed length 0.500000, actual length 0.500000
Supposed length equals to actual length: True

A: Supposed length 6.5, actual length 6.5
B: Supposed length 6.500000, actual length 6.500000
Supposed length equals to actual length: True

A: Supposed length 0, actual length 0.0010000000475
B: Supposed length 0.000000, actual length 0.001000
Supposed length equals to actual length: False

Steps to reproduce

Run the scene in the minimal project.

Minimal reproduction project

Animation Length.zip

@fire
Copy link
Member

fire commented Aug 26, 2021

@aaronfranke Can you help me describe the nature of floating point exact equality comparisons?

I remember there was a blog post somewhere.

@aaronfranke
Copy link
Member

@fire We should not use float exact equality comparisons, instead we use is_equal_approx. This would solve the first three cases in this post, but the bottom case is not a situation of floating point error, it's too big of an error (~0.001).

@pouleyKetchoupp
Copy link
Contributor

It looks like there's a minimum of 0.001 when calling set_length:

void Animation::set_length(real_t p_length) {
if (p_length < ANIM_MIN_LENGTH) {
p_length = ANIM_MIN_LENGTH;
}
length = p_length;
emit_changed();
}

That explains the last case, and it should be the case on both 3.x and 4.0.

@fire
Copy link
Member

fire commented Aug 26, 2021

If all the cases are covered and explained, I don't think we have incorrect values.

That suggests we can close this bug.

@aaronfranke
Copy link
Member

Yes, these values are expected. There is no bug here unless we want to support animation lengths < 0.001.

@floppyhammer
Copy link
Contributor Author

I made a bad description of the issue. I modified the code a bit to make it clearer.

New code:

randomize()
for anim in anim_player.get_animation_list():
	var frame_count : float = randi() % 20 + 1

	var frame_fps : float = randi() % 10 + 1
	
	var anim_length = round(frame_count / frame_fps * 1000.0) / 1000.0
	
	anim_player.get_animation(anim).set_length(anim_length)
	
	var actual_length = anim_player.get_animation(anim).get_length()
	
	print("Set length ", anim_length, ", actual length ", actual_length)
	
	print("Actual length is larger than the set length : ", actual_length > anim_length)
	
	print(" ")

In 4.0:

Set length 2.333, actual length 2.33299994468689
Actual length is larger than the set length : False
 
Set length 4.333, actual length 4.33300018310547
Actual length is larger than the set length : True
 
Set length 1.556, actual length 1.55599999427795
Actual length is larger than the set length : False
 
Set length 1.833, actual length 1.83299994468689
Actual length is larger than the set length : False

In 3.x:

Set length 1.375, actual length 1.375
Actual length is larger than the set length : False
 
Set length 0.625, actual length 0.625
Actual length is larger than the set length : False
 
Set length 7, actual length 7
Actual length is larger than the set length : False
 
Set length 1.75, actual length 1.75
Actual length is larger than the set length : False

@rsubtil
Copy link
Contributor

rsubtil commented Aug 27, 2021

The difference in the printed values probably comes from #21922, which changed get_length()from a float to a real_t. But the boolean conditions are consistent between 3.x and 4.0 from my tests, so while the 3.x rounds the values when you print them, on 4.0 it's showing with much more detail.

@aaronfranke
Copy link
Member

@floppyhammer Every one of those examples is due to floating point error. You have to compare using is_equal_approx().

I'm not sure if you deliberately did this, but pay special attention to the values you're using. All of the ones at the bottom are perfectly representable with floats because they are sums of powers of two (ex: 0.625 = 0.5 + 0.125). These numbers will also be exact in 4.0. On the other hand, a number ending in 0.333 cannot be represented perfectly with floats so it's expected to have imprecision. We can get closer by increasing float precision (compiling with float=64) but we can never get to exactly 4.333.

Since the bug you've encountered is expected float imprecision and there is no feature request or confusion about zero-length animations, there is nothing that can be done here, so I'm closing this.

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