<Swing Dev> [9] Review request for 8162350 RepaintManager shifts repainted region when the floating point UI scale is used

[Jim Graham]

I wouldn't necessarily recommend Area as the replacement.  It's designed for complicated path intersection, not as an 
optimal path storage mechanism.

Also, since the constraint has to be installed on every repaint, re-rasterizing the shaped boundary on every repaint 
could potentially impact performance.  There could be cooperation between SG2D and the caller of constrain() to pass 
back the Region and only recompute it when the scale changes, but it does need the original path in case the component 
experiences a change of output DPI...

			...jim

On 11/30/16 4:07 PM, Sergey Bylokhov wrote:
> On 30.11.16 2:11, Jim Graham wrote:
>> - Region.getScaledRegion() - this method should be deleted.  I think it
>> is only used in SG2D.constrain() which needs to be rewritten as you
>> cannot scale a region and expect any amount of accuracy, but we live
>> with it for now until a better fix comes along.  I don't think changing
>> the rounding method used by this method will have any useful impact, it
>> just needs to be eliminated by fixing the callers to no longer need it.
>
> We have a bug(i cannot find a bugid) that the shaped windows are aliased on the hidpi screens. Initially it was
> implemented on OSX this way, because the value which passed to the peers is Region not Shape/Area. According to Anthony
> the shared code uses Region because of performance issues, so replacement of Region->Area was deferred by Anthony,
> because the changes in shared was out of scope of support the shaped window/HiDPI.
>
>>
>> There are a couple of more uses in the AWT that I didn't look too
>> closely into at this point, but those are the uses within the Java2D
>> area...
>>
>>             ...jim
>>
>> On 11/24/16 8:40 AM, Sergey Bylokhov wrote:
>>> I have a few questions which probably discussed already, then ignore it:
>>>
>>>  - SunGraphics2D.java: As far as I understand the clipScale() was
>>> replaced by clipRound(), because they have different
>>> round logic? It seems that when I wrote the clipScale() I was not
>>> aware about round logic, and looks like we can change
>>> the clipScale implementation to use clipRound internally instead of
>>> Math.round(newv), can be fixed by othe fix.
>>>  - Did you check the difference in performance between
>>> paintDoubleBufferedImpl vs paintDoubleBufferedFPScales? At least
>>> in terms of heavyweight operations it looks similar, and probably we
>>> can have only one of them? It has an additional
>>> benefits that the new code will be tested on the usual system as well.
>>>
>>> On 21.11.16 16:59, Alexandr Scherbatiy wrote:
>>>>
>>>> Hello,
>>>>
>>>> Could you review the updated fix:
>>>>   http://cr.openjdk.java.net/~alexsch/8162350/webrev.04
>>>>
>>>>  - isFloatingPointScale(AffineTransform) is moved from the SunGraphics2D
>>>> to the SwingUtilities2 class.
>>>>
>>>>   Thanks,
>>>>   Alexandr.
>>>>
>>>> On 11/18/2016 11:23 PM, Jim Graham wrote:
>>>>> Hi ALexandr,
>>>>>
>>>>> This looks great.
>>>>>
>>>>> BTW, when I suggested moving the FPscale test into SG2D I was
>>>>> suggesting that to avoid having to copy the transform out of it via
>>>>> getTransform(), but you've found a different solution to that issue
>>>>> (i.e. the new getTransform(g) method) so it no longer matters where
>>>>> that utility static function is located.  You can move it back to one
>>>>> of the Swing classes.
>>>>>
>>>>> In terms of the logic of choosing which repaint function to use, it
>>>>> looks like you use the old-style function if the scales don't match,
>>>>> but won't that cause rendering anomalies?  The new code is still an
>>>>> improvement for the standard HiDPI case, and I'm guessing that
>>>>> mismatched scales probably never tends to happen, but we might want to
>>>>> flag it for further investigation.
>>>>>
>>>>> +1 relative to whether you want to move the FPscale test back out of
>>>>> SG2D or not...
>>>>>
>>>>>             ...jim
>>>>>
>>>>> On 11/18/16 1:44 AM, Alexandr Scherbatiy wrote:
>>>>>>
>>>>>> Thank you. I see that using the integer device-pixel translations
>>>>>> preserves the component painting in the same way for
>>>>>> floating point scales.
>>>>>>
>>>>>> Could you review the updated fix:
>>>>>>   http://cr.openjdk.java.net/~alexsch/8162350/webrev.03
>>>>>>
>>>>>>   - translation adjustment is removed
>>>>>>   - Region.clipRound() is used for pixels coordinates rounding.
>>>>>>
>>>>>>   Thanks,
>>>>>>   Alexandr.
>>>>>>
>>>>>> On 11/16/2016 1:52 AM, Jim Graham wrote:
>>>>>>> Let me clarify something...
>>>>>>>
>>>>>>> On 11/15/16 2:49 AM, Alexandr Scherbatiy wrote:
>>>>>>>>   Let's consider the following use case:
>>>>>>>>   scale = 1.5
>>>>>>>>   A component calls fillRect(1, 1, 1, 1).
>>>>>>>>   This is (1.5, 1.5, 3.0, 3.0) in the device space
>>>>>>>>   which fills  (1, 1, 3, 3) and covers 2x2 pixels
>>>>>>>
>>>>>>> Agreed.
>>>>>>>
>>>>>>>>   Now the area (1, 1, 1, 1) needs to be repainted
>>>>>>>>     create a backbuffer
>>>>>>>>     translate(-1, -1) // move the top left corner of the area to
>>>>>>>> the zero point
>>>>>>>>     draw the component into the backbuffer:
>>>>>>>>       fillRect(1, 1, 1, 1) -> after translation fillRect(0, 0, 1,
>>>>>>>> 1) -> after scaling  (0.0, 0.0, 1.5, 1.5 ) in the
>>>>>>>> device space
>>>>>>>>       which fills (0, 0, 1, 1) and covers 1x1 pixels
>>>>>>>
>>>>>>> If you did g.setTransform(identity), g.translate(-1, -1), (then
>>>>>>> restore the scale) then the analysis is as follows:
>>>>>>>
>>>>>>> g.setTransform(identity) => [1 0 0] [0 1 0]
>>>>>>> g.translate(-1, -1) => [1 0 -1] [0 1 -1]
>>>>>>> g.scale(1.5, 1.5) => [1.5 0 -1] [0 1.5 -1]
>>>>>>> g.fillRect(1, 1, 1, 1)
>>>>>>>     => coordinates are (1.5-1, 1.5-1, 3-1, 3-1)
>>>>>>>     => (.5, .5, 2, 2)
>>>>>>>     => fills (0, 0, 2, 2)
>>>>>>>     => which covers 2x2 pixels
>>>>>>>
>>>>>>> If you did g.translate(-1, -1) on the scaled transform then the
>>>>>>> analysis is as follows:
>>>>>>>
>>>>>>> g.transform is [1.5 0 0] [0 1.5 0]
>>>>>>> g.translate(-1, -1) is [1.5 0 -1.5] [0 1.5 -1.5]
>>>>>>> g.fillRect(1, 1, 1, 1)
>>>>>>>     => coordinates are (1.5-1.5, 1.5-1.5, 3-1.5, 3-1.5)
>>>>>>>     => (0, 0, 1.5, 1.5)
>>>>>>>     => fill (0, 0, 1, 1)
>>>>>>>     => covers 1x1 pixels
>>>>>>>
>>>>>>> The second operation is what you are describing above and that would
>>>>>>> be an inappropriate way to perform damage repair
>>>>>>> because you used a scaled translation which did not result in an
>>>>>>> integer coordinate translation.
>>>>>>>
>>>>>>> Please re-read my previous analysis that shows what happens when you
>>>>>>> use integer device-pixel translations which are
>>>>>>> translations that happen using integers on a non-scaled transform.
>>>>>>> Note that you can add a scale *AFTER* you apply
>>>>>>> the integer device pixel translation and it will not affect the
>>>>>>> integer-ness of the translation.  You can see above
>>>>>>> that the difference in how the translate command is issues affects
>>>>>>> where the translation components of the matrix end
>>>>>>> up being -1,-1 or -1.5,-1.5...
>>>>>>>
>>>>>>>             ...jim
>>>>>>
>>>>
>>>
>>>
>
>