Fix animated tile time-slice calculation accumulating float errors

[kleonc]

TileMap's tile animation time slice starts/ends were calculated like:
((6.0f / 6.0f) + (1.0f / 6.0f) + (1.0f / 6.0f) + (1.0f / 6.0f)) -> 1.49999988079071044922
instead of:
((6.0f + 1.0f + 1.0f + 1.0f) / 6.0f) -> 1.50000000000000000000
but the total animation duration were calculated like the latter and hence there was potentially a small time gap between the last frame's end and the end of the whole animation.

Should fix #82286, @Proggle please test if you can.

[groud]

This seems a bit complicated tbh. IMO, the simplest solution is to set the end frame equal to animation_duration when you are at the last frame. Something like:

real_t slice_end = frame == atlas_source->get_tile_animation_frames_count(p_atlas_coords)-1 ? animation_duration : time + frame_duration;
RenderingServer::get_singleton()->canvas_item_add_animation_slice(p_canvas_item, animation_duration, time, slice_end, p_animation_offset);

[kleonc]

This seems a bit complicated tbh. IMO, the simplest solution is to set the end frame equal to animation_duration when you are at the last frame.

It's barely more complicated. I disagree we should be picking a little simpler solution over a more correct one (assuming there are no performance reasons etc.). And calculating things like I've done in here seems more correct to me (for the intermediate frames it's i * addition_error + division_error vs i * addition_error + i * division_error).

In the usual use cases it should indeed not matter and both solutions should be unnoticeably different. But if for some reason e.g. speed would be very small then the discrepancies could get significant.
Is it theoretization? Yes.
Does this matter in practice? Likely no.
But I still see no reason to not handle such potential edge cases properly if it's not computationally costlier, the code isn't much more complicated, or something like that. And AFAICT that's the case in here.
(if anything I'd say the most complicated is the comment I've added for clarifying things 😄; for me the code seems self-explanatory and that comment is not needed, still added it so no one would refactor it back)

I can go with your suggested solution if that's what's really preferred. Just wanted to point out that in general I advocate for correctness over a barely simpler code. 🙃 Decision up to you, you're the maintainer here.

[groud]

I don't mind either way to be honest, merging as is would be fine.

TBH I don't really understand why the CanvasItem API does need both a frame start, a frame end and a duration. Either the frame duration or the total duration should be "ignorable".

[akien-mga]

This solution makes sense to me.

[kleonc]

I don't mind either way to be honest, merging as is would be fine.

🙂

TBH I don't really understand why the CanvasItem API does need both a frame start, a frame end and a duration. Either the frame duration or the total duration should be "ignorable".

AFAICT it could be as you're saying if it would be limited to a typical looped frame by frame animations (like the ones in the TileMap). But it's not like that, the API allows full flexibility. Animation slices can potentially overlap each other time-wise, there can be periods with nothing to draw,you could even issue at once all commands to draw some layered animation with each layer having its own "fps", etc.

Each animation slice rendering command is an independent command, it doesn't know/assume anything about other such commands, hence it seems to need all that info:

godot/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp

Lines 948 to 955 in 008b08b

	case Item::Command::TYPE_ANIMATION_SLICE: {
	const Item::CommandAnimationSlice *as = static_cast<const Item::CommandAnimationSlice *>(c);
	double current_time = RendererCompositorRD::get_singleton()->get_total_time();
	double local_time = Math::fposmod(current_time - as->offset, as->animation_length);
	skipping = !(local_time >= as->slice_begin && local_time < as->slice_end);
	RenderingServerDefault::redraw_request(); // animation visible means redraw request
	} break;

[Proggle]

TileMap's tile animation time slice starts/ends were calculated like: ((6.0f / 6.0f) + (1.0f / 6.0f) + (1.0f / 6.0f) + (1.0f / 6.0f)) -> 1.49999988079071044922 instead of: ((6.0f + 1.0f + 1.0f + 1.0f) / 6.0f) -> 1.50000000000000000000 but the total animation duration were calculated like the latter and hence there was potentially a small time gap between the last frame's end and the end of the whole animation.

Should fix #82286, @Proggle please test if you can.

Thanks! I'll check this weekend when I have time.

If I'm understanding properly this particular glitch would cause a tile rendering failure only if the frame rendering fell exactly in the 0.0000001192 second window between timespans, so it's possible that I'm seeing animation failures for multiple reason beyond this one

[kleonc]

I'll check this weekend when I have time.

@Proggle Thanks! Note you don't need to build it yourself to test, you could download an artifact from the CI checks down below ("Show all checks" -> "Details" (any of them) -> "Summary" -> "Artifacts"). E.g. this one for windows (artifacts are available only for some time after the CI action is run so the link is temporary).

If I'm understanding properly this particular glitch would cause a tile rendering failure only if the frame rendering fell exactly in the 0.0000001192 second window between timespans,

Indeed.

so it's possible that I'm seeing animation failures for multiple reason beyond this one

If you're talking about that specific glitch from #82286 then AFAICT it's unlikely there's a different cause (e.g. a different reason could be Math::fposmod not working correctly). But sure, there could be some different problem I haven't noticed looking at the relevant code. 🙃

[akien-mga]

Thanks!

[Proggle]

I'll check this weekend when I have time.

@Proggle Thanks! Note you don't need to build it yourself to test, you could download an artifact from the CI checks down below ("Show all checks" -> "Details" (any of them) -> "Summary" -> "Artifacts"). E.g. this one for windows (artifacts are available only for some time after the CI action is run so the link is temporary).

If I'm understanding properly this particular glitch would cause a tile rendering failure only if the frame rendering fell exactly in the 0.0000001192 second window between timespans,

Indeed.

so it's possible that I'm seeing animation failures for multiple reason beyond this one

If you're talking about that specific glitch from #82286 then AFAICT it's unlikely there's a different cause (e.g. a different reason could be Math::fposmod not working correctly). But sure, there could be some different problem I haven't noticed looking at the relevant code. 🙃

Oh, that's handy, trying to compile on my laptop takes forever.

Anyway, running both the original and patched versions side by side, I managed to catch 4 framedrops on the old version and 0 on the new one, so tentatively looks like it's patched. I'm going to keep working on this project more and I'll yell if I see any more vanishing tiles.

Mathematically seems weird that this'd be the cause, if it were some kind of uniform distribution it seemed super unlikely a frame would fall into the crack between floats as often as I was seeing it (about a 0.0007% chance per loop, so about 3% chance of seeing a single frame drop if I was watching it a whole hour). I guess the distribution isn't actually that random and when my computer is at the right cpu load it consistently takes exactly 1.49999988079071044922 seconds to do 90 frames.

Weird, but I'll take the win. Thanks for the fix :)

[YuriSizov]

Cherry-picked for 4.1.3.