Win32DPIUtils:pointToPixel shouldn't have residual #2364
Conversation
Test Results 118 files ±0 118 suites ±0 9m 52s ⏱️ -24s For more details on these failures, see this check. Results for commit 341a86b. ± Comparison against base commit 810cbcc. ♻️ This comment has been updated with latest results. |
| Point scaledPoint = new Point.OfFloat(scaledX, scaledY); | ||
| return new Point.OfFloat(scaledPoint.x, scaledPoint.y); |
There was a problem hiding this comment.
No need to instantiate twice, do either:
// Cast to int so that there are no residuals
return new Point.OfFloat((int) scaledX, (int) scaledY);or even better, simply declare scaledX and scaledY as ints
// Use ints so that there are no residuals
int scaledX = ...;
int scaledY = ...;
return new Point.OfFloat(scaledX, scaledY);There was a problem hiding this comment.
(int) will not round it properly. We will need to use Math.round instead.
There was a problem hiding this comment.
See https://github.com/eclipse-platform/eclipse.platform.swt/pull/2364/files/ca6ef3a35062a41e9e47b39d9e3b071fabbc3811#r2379074441 since you are probably trying to do the same here and you have to preserve the other information (monitor) here too.
There was a problem hiding this comment.
Isn't the point of this code to ensure that we have a consistent way of rounding (by delegating the responsibility to Point.OfFloat), which would break with replicating the actual rounding implementation (via Math.round) here?
| private static Rectangle pointToPixel(Rectangle.OfFloat rect, int zoom) { | ||
| return scaleBounds(rect, zoom, 100); | ||
| Rectangle scaledRectangle = scaleBounds(rect, zoom, 100); | ||
| return new Rectangle.OfFloat(scaledRectangle.x, scaledRectangle.y, scaledRectangle.width, scaledRectangle.height); |
There was a problem hiding this comment.
Why do you need to create yet another copy? scaleBounds(...) already returns a shallow copy. In fact, in doing so, you might loose information about the monitor since you're not using clone, you're using a constructor.
There was a problem hiding this comment.
Just thinking if we could somehow reset the residual values.
There was a problem hiding this comment.
Ah, I see. Tricky to do without exposing too much about the internal workings of OfFloat.
I would be OK with doing this:
Rectangle scaledRectangle = scaleBounds(rect, zoom, 100);
scaledRectangle.setX(Math.round(scaledRectangle.getX());
scaledRectangle.setY(Math.round(scaledRectangle.getY());It's not perfectly encapsulated (exposes the fact that you use Math.round(...) instead of just casting to int) but at least it preserves the monitor (if it's there).
If you don't want to expose that detail then you should add yet another method to Rectangle.OfFloat that does the rounding and looses the residuals. Something like:
void round() {
setX(Math.round(getX());
setY(Math.round(getY());
}Your call.
There was a problem hiding this comment.
@akoch-yatta are you taking over this PR too?
If a better (and simple) solution can't be found then I suggest we merge it as-is. I don't think this is going to be a performance bottleneck anyway and the rest of the PR looks fine.
I'll withdraw my RFC.
This commit contributes to scaling points and rectangles from point to pixels while making sure they do not have any residuals since the OS doesn't support sub-pixel drawing. contributes to eclipse-platform#62
fedejeanne
force-pushed
the
amartya4256/point_to_pixel_offloat
branch
from
ca6ef3a to
341a86b
Compare
|
@amartya4256 I rebased on |
fedejeanne
dismissed
their stale review
Only NIT comments. No need to stop this PR.
3 participants
This commit contributes to scaling points and rectangles from point to pixels while making sure they do not have any residuals since the OS doesn't support sub-pixel drawing.
contributes to #62