-
Notifications
You must be signed in to change notification settings - Fork 185
Streamline rounding for rectangles and add rounding mode to them #2720
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
Streamline rounding for rectangles and add rounding mode to them #2720
Conversation
43f1a89 to
c23483c
Compare
|
@HeikoKlare I see that only RoundingMode.ROUND is used in the code for now for rectangles. Could you please explain what RoundingMode is intended to be used in which case? |
|
With the current changes, we will revert to |
|
|
||
| public void setWidth(float width) { | ||
| this.width = Math.round(width); | ||
| this.width = sizeRounding.round(width + getX()) - x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember there were cases in the past where some places would have different results when you would scale the rectangle using scaleBounds or scaleUp because they both used different approach to scaling. One had height and width calculated using the difference between top left and bottom right and the other as just the width and height values in the object. Are we sure that this approach would not break that behavior for certain consumers? Visually I cannot see anything in the IDE but I just wanted to pick this out just to be careful. I am going to consider that since there's no test failing, this might not be the case anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no way to be 100% sure that we to not break any behavior for existing consumers. With all the back-and-forth of the kinds of rounding we have implemented, this will probably also have effects that we currently do not see (either positive or negative).
Regarding the different scaling methods that you mention: they are still in place. We still have Win32DPIUtils#scaleBounds() which implement the "other" way of scaling (used for things related to image). The modified way of scaling only applies to what is passed to Win32DPIUtils#pointToPixel()/pixelToPoint(). And there the only removed case distinction is that when a Rectangle.OfFloat was passed, no forwarding to scaleBounds() happens anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look fine and introduce the rounding mode while scaling the rectangle. I do not see any visual difference in the runtime (as it should be). Hence, with the introduction of rounding mode now the rectangles scale consistently as implemented in OfFloat and WinDPIUtils. Since we have a public constructor to initialize a rectangle with a specific rounding, are there any possibilities where we will be using any other rounding mode in future? If not can we hide it for the time being?
c23483c to
dc68e98
Compare
Sure, I've made it private for now. The class and all its methods are |
For the float-aware Point class, a rounding mode has been introduced to store in which way the Point needs to be rounded when necessary. For rectangles, this has not been implemented yet and is added by this change. It also correct the way in which rectangle values are rounded by considering the top-left and bottom-right corner instead of top-left corner and width/height, also done in the Win32DPIUtils. So it streamlines the rounding behavior in Win32DPIUtils and the Rectangle class to uniformly scale the upper left and bottom right corner of a rectangle. The changes also makes proper use of the floating-precision coordinates in the point/pixel conversions for rectangles in the Win32DPIUtils.
dc68e98 to
03ec9bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine. Although there is no obvious visible improvement, I'll follow the argument with consistency and would merge this. I did not find a regression when testing around with different zoom values
For the float-aware Point class, a rounding mode has been introduced to store in which way the Point needs to be rounded when necessary. For rectangles, this has not been implemented yet and is added by this change. It also correct the way in which rectangle values are rounded by considering the top-left and bottom-right corner instead of top-left corner and width/height, also done in the Win32DPIUtils. So it streamlines the rounding behavior in Win32DPIUtils and the Rectangle class to uniformly scale the upper left and bottom right corner of a rectangle.
The changes also makes proper use of the floating-precision coordinates in the point/pixel conversions for rectangles in the Win32DPIUtils.