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

Replace pixel rounding with floor(x + 0.5) #93740

Merged

Conversation

markdibarry
Copy link
Contributor

Fixes: #93731

We should do some more testing to be sure this has no side effects in pixel perfect art projects.

@markdibarry markdibarry marked this pull request as ready for review June 29, 2024 14:02
@markdibarry markdibarry requested review from a team as code owners June 29, 2024 14:02
@alvinhochun
Copy link
Contributor

Thanks for making the PR. I was also looking at this more in-depth locally, and noticed

fx_offset = fx_offset.round();
might also need the same rounding change, since it checks snap_2d_transforms_to_pixel.

I do also have to note that, I realized there is a very specific edge case: the value 0.49999997 will be rounded incorrectly because 0.49999997f + 0.5f == 1.0f. But I think it is acceptable to ignore this edge case, because it is very rare to end up with this value, compared to multiples of -0.5. (For double the problematic value is 0.49999999999999994.)

Finally, there is the question whether we should do the same for GUI snap_controls_to_pixels, but that should be a separate PR.

@adamscott adamscott self-requested a review June 29, 2024 14:32
@markdibarry
Copy link
Contributor Author

Ah, yeah. I think that has always been rounded, which is why we never changed it.

@markdibarry markdibarry force-pushed the alternative-pixel-rounding branch from 468ac51 to 6b17d51 Compare June 29, 2024 14:33
@markdibarry markdibarry requested a review from a team as a code owner June 29, 2024 14:33
@adamscott
Copy link
Member

Did you try if it works with @KeyboardDanni's test scene found in #84380?

@markdibarry
Copy link
Contributor Author

@adamscott Yep. Same result.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing some games, all seems right. Tested out the issue MRP and it fixes it as well. Let's go, then?

@akien-mga akien-mga merged commit 84f4042 into godotengine:master Jun 29, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@markdibarry markdibarry deleted the alternative-pixel-rounding branch June 29, 2024 20:44
@Gnumaru
Copy link
Contributor

Gnumaru commented Jun 29, 2024

Instead of replacing round(x) with floor(x + 0.5), wouldn't it be better to replace round(x) with round(x + FLOAT_EPSILON)? (being FLOAT_EPSILON std::numeric_limits<float>::epsilon() or any equivalent constant), or any really small value like 1E-6. This way rounding -0.1 and +0.1 would both result 0 and the code would be more explict about the intention of rounding the value.

(I don't know it if is allowed to comment closed pull requests or issues. If it is not allowed, please forgive me.)

@KeyboardDanni
Copy link
Contributor

KeyboardDanni commented Jun 30, 2024

I'm not quite sure what epsilon would do for us here. Epsilon is generally used when comparing equality between two floating point numbers, not flooring or rounding. Even there, epsilon is a bit of a flawed concept, because the "minimum distance between two numbers" is different based on the current exponent.

@Chaosus
Copy link
Member

Chaosus commented Jul 13, 2024

Seems like it's cause a serious regression: #94298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants