[Win] StyledText with paint artifacts

[tmssngr]

Win32DPIUtils.pixelToPointWithSufficientlyLargeSize: If we need the rectangle that is large enough, the left top corner must be rounded down and the right bottom corner rounded up.

Scrollable.getClientArea and the bounds of the Paint event also need to be large enough to contain any pixel. In the worst case, it contains "half" points, but it does not matter if it draws one pixel here and there into the invisible area.

However, it might be better if Scrollable.getClientArea might return a special rectangle (similar to Rectangle.OfFloat) that can be asked for the "inner" and "outer" rectangle, because only the user of knows what rectangle is important for him: for painting the larger rect is important. But, for example, for a table that resizes a single column depending on the size, the smaller rect might be of interest to prevent horizontal scrollbars.

[github-actions]

Test Results (win32)

   30 files  ±0     30 suites  ±0   4m 1s ⏱️ -12s
4 697 tests ±0  4 622 ✅ ±0  75 💤 ±0  0 ❌ ±0 
1 227 runs  ±0  1 203 ✅ ±0  24 💤 ±0  0 ❌ ±0 

Results for commit 91677aa. ± Comparison against base commit 823aeba.

[HeikoKlare]

The finding regarding the calculation results of pixelToPointWithSufficientlyLargeSize are interesting. But the current proposal leads to incorrect results. Note that this kind of change needs to be tested with different monitor setups/zooms and different contexts (layouts etc.) to see that nothing breaks. We quite often ran into the situation that just one specific combination of monitor zoom and SWT layout (and maybe others) broke. If possible, the scenarios currently not working correctly should be reflected in test cases to Win32DPIUtilTests or ControlWin32Tests.

In this case, one scenario that breaks is at 175%. In the following screenshots you see that at least the border at the bottom is not rendered anymore.

With this change:

Without this change:

[HeikoKlare]

This calculation can lead to an unintended overcalculation. In addition, it does not consider input rectangles already being at floating-point precision, for which an exact conversion was done before this change.
Can't we solve the issue at the root (inside the pixelToPoint method), which already seems to yield unexpected results when using rounding mode "up". E.g., the scaledBottomRight calculation may incorporate the rounding mode with something like Point.OfFloat.from(pixelToPoint(floatRect.getBottomRight(), zoom, sizeRounding)).
Also note that the pixelToPoint calculation does some internal rounding (calculating based on rounded values instead of the float-based values and rounding afterwards) because everything else broke one case or another.

It would also be good to isolate this change from further changes, such as consumers of this method.

[tmssngr]

Please treat this PR as discussion base. What about the suggestion regarding the Rectangle? I think, we need to deprecate the access to the x, y, width, height fields (or to Rectangle at all), because on Windows we either need float values or better ints rounded up/down - according to the user's wish. Something like rect.getXCeil(), rect.getXFloor(), ... BTW, I do not see any use-case for the rounding mode "round", only for "up" or "down".

[HeikoKlare]

I think, we need to deprecate the access to the x, y, width, height fields (or to Rectangle at all), because on Windows we either need float values or better ints rounded up/down - according to the user's wish.

Two thoughts on this:

1. We can deprecate the fields of Rectangle (and then also Point), such as x, y, width and height. But you will never be able to deprecate them for removal, as they have been there forever and they are commonly used. So you would break (stale) consumers (like framework extensions) when removing direct access. That's the reason why we chose to provide new types of Rectangle and Point and use them at those places that require them, including the "dynamic conversion" with methods like Rectangle.OfFloat.from() and the like to avoid breaking changes of API that accepts or returns Point or Rectangle.
2. There should not be something like a "user's wish". Once we start shifting the effort of dealing with autoscaling to the user, the whole idea of SWT autoscaling dilutes. On the other hand, we know that it's impossible to deal with every limitation of autoscaling concept when it comes to fractional scaling behind the APIs. For that reason, we chose to track the problematic points and find solutions for them. This particularly includes two points: (1) the global coordinate system which became non-continuous and (2) pixel-perfect rendering scenarios (like diagrams). Primarily for (1), we provided new API to Rectangle and Point to allow for better preservation of precision outside of the API without thinking about it and without explicitly dealing with floating-point precision. For (2), we provided the concept of autoscale disablement at control level, so that specific control can deal with scaling on their own. Until now, these solutions were mostly sufficient. So I would only be in favor of further breaking encapsulation of autoscaling if we do it in a very focussed way based on specific problem scenarios that we have identified rather than doing it in a quite generic way.

BTW, I do not see any use-case for the rounding mode "round", only for "up" or "down".

I guess you mean there are only use cases for "round" and "up" but not for "down", right? We have once used rounding mode "down" for specific use cases of rectangle conversions but found that to be inaccurate in specific situation, so that we revised the code again and now ended up not using "down" anywhere. It's rather there for the sake of completeness and for easy testing when trying to further improve calculations related to conversions and rounding.

That said, I think you have a good catch with the unexpected results of pixelToPointWithSufficientlyLargeSize. A good step could be to identify where those unexpected calculation results lead to unexpected user-facing behavior and fix that. Maybe it is even related to the StyledText painting issue.

[tmssngr]

Regarding 1. "Deprecating Rectangle fields": How does overriding solve this problem? Any code which uses a rectangle (or a subclass) can modify x, y, width, height and expect other code to respect these changes. The problem: subclasses don't get notified if code changes x, y, width or height.

Regarding 2. "User's wish": only the user of a rectangle knows which values they need - rounded up or down. This depends on the use case. I don't think, SWT magically can fix everything itself, but it needs to provide API that the user can solve their problem.

I guess you mean there are only use cases for "round" and "up" but not for "down", right?
No, I mean that I do not see any use-case for "round", except of getting just a rough estimation. Until now I only can imagine use-cases for rounding up or down:

• rounding up: clipping region, drawing the full client area, drawing an upper/left border
• rounding down: drawing a lower/right border, make columns as wide as the client area without showing horizontal scrollbars

The bad thing about pixelToPointWithSufficientlyLargeSize is, that I could not find any code unit-testing it.

What do we need (from a user's perspective of SWT): getSize(), getBounds(), getClientArea() need a way to provide the rounded up or rounded down coordinate (IMHO it makes sense to think about the Rectangle as two coordinates, left-up and right-bottom). For MacOS and Linux the reported coordinates (rounded up or down) would be always the same, because they are precise. But for Windows, these coordinates rounded up or down could be off by one point. In the worst case, the different of height or width could be 2 points.

Going further: if code, like gc.drawRect() gets a rectangle, the user might be able to specify whether it means the rounded up or rounded down scaled pixel (for each coordinate).

[HeikoKlare]

I think there is a fundamental difference in our understanding of how SWT should behave here. You expect SWT to expose internal about autoscaling so that the user can (and has to) deal with things related to autoscaling on their own. As I explained in my previous post that will completely change how autoscaling in SWT is designed. Autoscaling was designed to be fully encapsulated in SWT so that consumers of SWT only operate on logical point coordinates. In a perfect world, they should not need to know that those logical point coordinates are internally converted into pixels based on monitor zooms, they should just deal with a logical coordinate space. Once you pass out any information about necessary rounding methods and require consumers to decide if they want to round up or round down, you completely break the whole design of autoscaling. I don't say that's not possible, but it will put a burden on consumers that was never intended.
Everything we did so far in terms of extensions to make monitor-specific scaling possible was made with this design in mind. That did not lead to the cleanest solution one would like to have (because that's impossible with integer-based logical points and fractional scaling) but it worked sufficiently well to not throw away the overall design idea.

No, I mean that I do not see any use-case for "round", except of getting just a rough estimation. Until now I only can imagine use-cases for rounding up or down:

You can see a bunch of consumers of the "Round" rounding mode:

[tmssngr]

I've shown you above in several examples that there is no one solution-fits-all-use-cases, and you also confirmed that (maybe unintentionally). I think, we can't hide the details of the auto-scaling because it introduces (rounding) errors; and to produce good looking results the SWT user somehow need to deal with them. Hence the API needs to provide this information.

You can see a bunch of consumers of the "Round" rounding mode:

Is rounding really correct in all of these uses? Or was it just used because the API did not had better means?

Maybe the auto-scaling with non-integer scaling factors does not work out on Windows (at least with the restrictions of the SWT API), if one cares about good looking results. There might be a good reason why Apple offered only integer scaling.

I hope we agree, that

• we want SWT to produce good looking, pixel-precise results,
• the current state of SWT has some problems with auto-scaling on Windows (especially with fractional scaling) which causes artifacts, asymmetric painting results, scrollbars were there shouldn't be some,
• we want to solve all of them,
• we only disagree in how to solve them.

Have a nice weekend! 😊

[HeikoKlare]

Is rounding really correct in all of these uses? Or was it just used because the API did not had better means?

The API (actually it's not even an API but just some internal utility) allows you to pass whatever rounding mode you need. And if there is no according method, you can add one (as it's an internal utility). So technical limitations are not the reason for the implementation. I guess we had not reason to use a different rounding mode than "round" in the according use case, probably it is even the best-fitting rounding mode for them. When you go through the history of changes and PRs on the topic, you will see that there were quite some revisions on which modes were used and why. But there is no general reason why mode "round" should not be appropriate.

Maybe the auto-scaling with non-integer scaling factors does not work out on Windows (at least with the restrictions of the SWT API), if one cares about good looking results. There might be a good reason why Apple offered only integer scaling.

Where does this conclusion come from? Monitor-specific scaling (including fractional scaling awareness) on Windows is the default for Eclipse RCP products since over a year. We also use it in our product for quite a while. And I have not heard of larger complains since then via any channel. Of course, there are issue popping up here and there, but not of them was a severe issue (maybe that's yet to come 😉). Quite the contrary, the work on this new scaling support fixed issues that were also present in previous, limited HiDPI support. And the blurry scaling we had then (or no scaling at all, if you disabled it) was obviously inferior to what we have now.
Please also note that just excluding fractional scaling does not solve rounding issues, they just appear less often. Rounding errors occur because of non-invertable conversions between differently scaled number spaces limited to integers. When you, e.g., only allow 200% (in addition to 100%), there is still no possibility to losslessly convert, e.g., pixel coordinate (3, 3), to a point coordinate (as that would be (1.5, 1.5) which you cannot express with integer values).

The comparison with Cocoa (and also GTK) is misleading because with those libraries, SWT does not do any autoscaling at all (except for scaling images). For those libraries, scaling is done inside the OS/those libraries themselves, so SWT does not have to transform into pixel coordinates internally but can just operate on point values. But as one consequence, the scaling capabilities with Cocoa and GTK are also far inferior to what Windows offers (while it makes them easier to adopt).

I hope we agree, that

• we want SWT to produce good looking, pixel-precise results,
• the current state of SWT has some problems with auto-scaling on Windows (especially with fractional scaling) which causes artifacts, asymmetric painting results, scrollbars were there shouldn't be some,
• we want to solve all of them,
• we only disagree in how to solve them.

We agree to the extent that we know about some specific issues with SWT's autoscaling on Windows that may require resolution 🙂 With the other points I partly disagree and some I cannot even relate to autoscaling. We should keep topics seperated to have a focussed discussion and not mix up SWT autoscaling capabilities with, e.g., pixel-precise rendering or asymmetric painting (which are either independent topics or already solved via autoscaling disablement due to being conceptually incompatible to the idea of autoscaling):

• Pixel-precision: SWT's autoscaling (at least when based on integer values) is conceptually incompatible with pixel-perfect rendering. That's why we have the autoscaling disablement feature for exactly such pixel-perfect rendering scenarios, as that appears to be the conceptually cleanest solution to it. Take GEF Classic as an example for how pixel-perfect rendering works with monitor-specific scaling on Windows. So this part may be basically considered solved.
• Painting: painting (with the GC) in general is an issue on its own (even without any scaling involved): if with "asymmetric painting result" you refer to [Win32] Drawing produces unsymmetric results without swt.autoScale=false for 4k monitor with zooming #1596, then I can just refer to the point above regarding pixel-perfect rendering. But there are more flaws in the GC implementation leading to inconsistent/inexpected results (as said, even independent from scaling).
• Solving all problems: I do not want to solve all problems. First, I only want to solve problems if they affect relevant use cases. Since we know that it's impossible to solve all autoscaling-related problems (for conceptual reasons), every other goal would not be reasonable either. In addition, we have a large number of known issues (like probably in every other framework/project) and we need to focus on the ones that are most relevant and worth working on them.

I think regarding potential solutions, the major, fundamental point I disagree on from a software-architectural point of view is this:

I think, we can't hide the details of the auto-scaling because it introduces (rounding) errors; and to produce good looking results the SWT user somehow need to deal with them. Hence the API needs to provide this information.

[tmssngr]

Is there a snippet available that shows how to disable auto-scaling for one control, especially in the context of monitor-dependent different zoom levels?

[HeikoKlare]

I don't think we have an explicit snippet for that. We used to implement the functionality together with the adoption in GEF Classic.
The API to disable autoscaling for a control is quite simple though (just a single method in GC). You can find information about that in the according news entry. Since you can just retrieve the zoom from the control's shell, it should also be easy to paint inside that control with a proper zoom factor applied.

[tmssngr]

I reckon, only the GC is not enough because then I'd need to know at least the exact pixel size of the client area, too.

[HeikoKlare]

Sorry, I of course meant Control, not GC.