Snap half-px sprite offset the opposite way as canvas items

[alvinhochun]

For the sprite offset of Sprite2D and AnimatedSprite2D, rounding towards positive direction changes the snapping direction of odd-sized sprites with centered=true compared to 4.2 or earlier. This is likely an undesirable change, so it should be changed to round halfway values the opposite direction using ceil(x - 0.5).

• Fixes [4.3beta3] Sprite pivot changed since 4.3beta2 #94298

[Chaosus]

It makes sense to also change it in rich_text_label and renderer_canvas_cull (for compatibility)?

[markdibarry]

We should hold off a bit on this one until we can discuss it more. I left some comments in the original ticket, as this seems to be expected behavior. This change would make rounding behavior inconsistent with the rest of Godot as well.

[Lielay9]

I'm not in favor of this pr. I'd argue the previous behavior (the one reintroduced here) was undesirable. Ideally the snapping should work the same no matter how a position is reached. This wasn't the case before (demo with non-centered sprites 4.2/this-pr):

[alvinhochun]

I will leave this open for now, but feel free to close if the consensus is to close #94298 as not a bug.

[alvinhochun]

I think I'll just replace this with #94626.

[KeyboardDanni]

Why was this one closed? I'd think this would be a far more consistent and less corner-case-y way to address the current issue.

I'm not in favor of this pr. I'd argue the previous behavior (the one reintroduced here) was undesirable. Ideally the snapping should work the same no matter how a position is reached. This wasn't the case before (demo with non-centered sprites 4.2/this-pr):

The previous behavior would round in a different direction based on whether the coordinate was positive or negative. The recent change to floor(x + 0.5) was to ensure that coordinates always round in the same direction regardless of location. This PR isn't doing anything to break that consistency. In fact it would make things more consistent, since sprite offsets are essentially subtracting a position. We would be essentially doing the same rounding as floor(x + 0.5) but because it's backwards, we invert the equation to ceil(x - 0.5).

Right now I'm a pretty firm believer that this is the fix we need. While it needs testing, it's more consistent, less corner-case-y, and addresses the specific issue in a general way by making the math more consistent, not less.

Could we please reopen this?

[KeyboardDanni]

...Not to mention, Godot 4.3 just now hit Release Candidate. I really don't think we have room to introduce any changes that have more potential for breakage than this one.

[adamscott]

@KeyboardDanni It breaks your PixelPerfectTest, one of the blocks show red.

[KeyboardDanni]

@adamscott Seems fine to me. Did you make any modifications to the test, or are you using any additional patches?

[markdibarry]

I tried it with KeyboardDanni's project and not getting any red. From all my tests it doesn't help or hurt anything. If the majority of users prefer that half values round up for positions but offsets work in reverse and round down for half values then this seems to do the trick, but I believe if we reopen it the PR should include some documentation of this behavior somewhere for when questions arise... or like in all other cases two weeks go by and I forget. 😅

[alvinhochun]

Why was this one closed? I'd think this would be a far more consistent and less corner-case-y way to address the current issue.

I'm not in favor of this pr. I'd argue the previous behavior (the one reintroduced here) was undesirable. Ideally the snapping should work the same no matter how a position is reached. This wasn't the case before (demo with non-centered sprites 4.2/this-pr):

The previous behavior would round in a different direction based on whether the coordinate was positive or negative. The recent change to floor(x + 0.5) was to ensure that coordinates always round in the same direction regardless of location. This PR isn't doing anything to break that consistency. In fact it would make things more consistent, since sprite offsets are essentially subtracting a position. We would be essentially doing the same rounding as floor(x + 0.5) but because it's backwards, we invert the equation to ceil(x - 0.5).

This PR makes things inconsistent between the sprite offset and node2d translation. Both offset and translation visually move the sprite, the two are not doing opposite things, so it does not really make sense for them to be rounded differently.

Right now I'm a pretty firm believer that this is the fix we need. While it needs testing, it's more consistent, less corner-case-y, and addresses the specific issue in a general way by making the math more consistent, not less.

Could we please reopen this?

I am more inclined to consider #94298 to be not a bug, but not too strongly. Regardless, I don't think this is the correct "fix".