[3.x] Add half frame to floor() for animated particles UV

[metinc]

Depending on the hardware or drivers it can occur on some devices that animated sprites do not render in the correct order. This happens because of an inprecision when calling floor(). Adding a 0.5 before using floor fixes this bug.

Closes #36435

[Calinou]

Thanks for opening a pull request 🙂

Pull requests should be opened against the master branch instead of 3.x first. This is because feature development occurs in the master branch – we then cherry-pick commits to 3.x if they don't break compatibility with existing projects.

Please keep this PR open as the fix is also relevant for 3.x, and the master PR is most likely not easy to cherry-pick.

[metinc]

Sorry for that! Here is the PR on master: #53237

[clayjohn]

I am a little skeptical. Adding 0.5 before calling floor() is the same as calling round() Adding 0.5 doesn't just adjust for a minor imprecision, it completely changes the behaviour.

My guess is that #36435 is actually an issue with how particle_frame is calculated.

[JFonS]

I think in this case it works out. The 0.5 is added before the division, not after. Assuming both particle_frame and h_frames are integers and greater than 0 the result after floor() should be the same but with more margin for precision errors.

[lawnjelly]

I also think it looks good. I'm wondering if it is also susceptible to a similar bug for the x? Should we make a similar adjustment for the x?

[metinc]

@clayjohn Thanks for your feedback. I understand your point but please note the brackets I added in my changes. As @JFonS pointed out the 0.5 is added before division to add a margin to the floor that is exactly a half frame. Let's assume we have a texture with 3x2 frames. Here is what happens:

@lawnjelly I think the code for x is good because there is only a mod() and not a floor(). An imprecision would only mean a tiny non-visible offset in the UV coordinates.

[lawnjelly]

Well for example, if you had e.g. 31.9999999 coming in, with a row of 16. Because of the mod operation, this will come out as 15.9999, instead of 0.0. I only say this because we not long ago had a similar bug in another area of Godot (I forgot the details).

Essentially mod is susceptible to an analogous bug on boundaries to floor.

[akien-mga]

I guess to address @lawnjelly's comment this should be:

Suggested change

	code += "\tUV += vec2(mod(particle_frame, h_frames) / h_frames, floor((particle_frame + 0.5) / h_frames) / v_frames);\n";
	code += "\tUV += vec2(mod(floor(particle_frame + 0.5), h_frames) / h_frames, floor((particle_frame + 0.5) / h_frames) / v_frames);\n";

[akien-mga]

But then this might be worth doing further up when particle_frame is calculated?

Maybe there's a better operation that could be done on integer values to calculate particle_frame in the first place instead of using floating point?

[lawnjelly]

It's kind of hard to read and follow in the stringifyed format 😕 but I think it could be:

floor (mod(particle_frame+0.5, h_frames)) / h_frames

Or do the particle_frame += 0.5; earlier as a once off as @akien-mga points out.

You could also in theory do something like a:

particle_frame = round(particle_frame) + epsilon;

And then not need the later bodges .. but for being a little more 'clever' this is something that would be be bound to screw up on some hardware with some special precision etc.

GLES has next to no support for integers unfortunately, which makes this kind of juggling super annoying but necessary.

[lawnjelly]

Am approving this because, despite my comment about the x coordinate, the PR as is fixes a bug, and there's no reason why we can't do the x in a separate PR after this is merged.

[akien-mga]

Thanks!