AnimationPlayer keyframe not displaying correct value

[ghost]

Windows 10 64bit
v3.1 f84893f

While animating a spritesheet, a keyframe value that's supposed to be showing at time 0.1 shows at 0.11 instead.

Different snapping values have different results.

Here is a video showing what's happening:
https://www.youtube.com/watch?v=AdmnlVSoBNo

Steps to reproduce:

1. keyframe spritesheet frames to 0 and 0.1
2. have snapping at 0.01
3. scrub timeline to 0.1, and witness the spriteframe property not change
4. scrub to 0.11 and see it update
   empty_godot.zip

[ghost]

Took a look into this and some things to add.

It appears to be a regression from 3.0, wasn't able to get the same issue in 3.0.6 stable.

I looked at what the value of the timeline position was using this snippet of code:

tool
extends AnimationPlayer

func _physics_process(delta):
	if(assigned_animation != ""):
		print("%0.12f" % current_animation_position)

At the keyframe with time as 0.1, it outputs 0.0999~

Perhaps this is a mantissa/rounding error problem, and it is always flooring to the nearest step size? As the problem will happen on keyframes with time values that are have some leading nines. If they're more like 1.000001 they work as expected.

@dualmatrix I figured I'd ask since you've spent some time in this area. Do you know what the history on the AnimationPlayer has been since 3.0.6? Was the whole thing refactored in the usability update to it? There has been quite a fair number of oddities appearing in the AnimationPlayer in 3.1.

[Piet-G]

@avencherus I've encountered this problem before as well, but I wouldn't know what causes it sorry. I'll look into it

[ghost]

@dualmatrix Thank Dual, I was probing about with my limited knowledge, and at least going back to Juan's large commit this summer, the issue appeared in a build I made of the commit prior to it. Very likely goes back much much further.

@bojidar-bg Was wondering if you might also weigh in on this. I see you had fixed up some issue in this ballpark once in the past: #15646

For some reason I couldn't get a checkout and build from this f235594 to work. so couldn't see what happened. Though it may of been very hard to tell if the commits immediately before had a time slider that didn't work at all.

[bojidar-bg]

Note that in 3.1 the animation player editor was revamped by reduz, so it is probably unrelated to f235594.

Seems to me like some precision error, perhaps in the Range class (that AnimationTimelineEdit extends). Not sure exactly what might cause it though.

[ghost]

@bojidar-bg I did go back to b659fd6 and I did a build of two commits behind it (one behind it wasn't building), to commit 0d7fa5f. It has the old animation player, but it does have the numerical precision problem on the animation position. So all I can say is that it falls between 3.0.6 and 0d7fa5f (and before b659fd6) somewhere.

I may be doing this wrong, but tried out your suggestion by creating an HSlider with the same step sizes as the animation. Then printed its signaled value and control value the same way I did with the animation time position. I figure since it also extends Range it should being doing roughly the same thing? It actually reports the steps cleanly when arriving at 0.1 with 0.01 steps.

I will see if I can build something a little after f235594.

[ghost]

A minor observation to add. When changing the track type from discrete to continuous the problem shifts to other points in the timeline. In this example it appears at 0.1 in discrete and at 0.3 for continuous.

[ghost]

I may have bisected it back to 7243695. Seems to work in the commit before this, and not from there forward.

It looks like quite some time ago, and after a change with the pool arrays?

@Xrayez Does this make sense why it would affect the AnimationPlayer?

[Xrayez]

No, I don't think it would affect at all, surely it was some other commit.

godot/editor/animation_track_editor.cpp

Lines 4733 to 4740 in a16d9c6

	float AnimationTrackEditor::snap_time(float p_value) {
	if (snap->is_pressed()) {
	p_value = Math::stepify(p_value, step->get_value());
	}
	return p_value;
	}

I think I might have had problems with stepify accumulating floating point errors in unrelated use case. Perhaps at some point of snapping the same value gets snapped again?

[ghost]

Interesting. So stepify twice in a row might create a precision error?

That would make sense. As far as I can tell the keyframe time data appears to be as expected and stored at the step size in the TSCN.

Also, sorry for the vagueness of that question. I was wondering if there was anything really different going on with the RealArray and FloatArray from back then that may of exposed this bug?

I don't know much about it, but I can't imagine it being there without more widespread issues. It at least seems like a possibility that some part of how the AnimationPlayer does its timeline seek may have been relying on something, and was unnoticed because of it?

Again, don't know anything about it yet.

My bisect went one direction the whole way and it was not what I was hoping to find. XD I was very skeptical that I did it correctly. Yet I ended up with two clean builds, one of 303370d and 7243695 where the same animation seek returns a different property value at time 0.1 with snap 0.01.

[Xrayez]

I think FloatArray was left to be backward-compatible with older versions so that the engine can parse the scene with an old format. The only difference is naming, so I see no connection with this issue.