[OpenJDK 2D-Dev] [9] Review request for 8160124 SunGraphics2D.hitClip() can give wrong result for floating point scale

[Jim Graham]

How is it returning true?  If the clip really is empty, then 
intersectsQuickCheck() should never return true.  Or are you saying that 
an empty clip shape produces a non-empty composite clip region?

			...jim

On 06/30/2016 10:02 AM, Sergey Bylokhov wrote:
> It looks strange that the empty clip became "non-empty"(at least hitClip
> reports this) when we apply transform to it, no? I guess that at the
> beginning of hitClip() we should check that the clip.isEmpty(), and we
> should return "false" in this case(I think this is not strictly related
> to this bug)?
>
> On 24.06.16 1:14, Jim Graham wrote:
>> Think of this method as asking:
>>
>> I don't want you to waste a lot of time, but tell me if it is silly for
>> me to even consider rendering something with these bounds.
>>
>> And the answer is either "Oh, yeah, it is inconceivable that those
>> bounds would be rendered", or "Not sure, maybe, just render it and
>> see".  There may be some cases where it knows "I know for sure that lots
>> of stuff will render through the clip", but that is not where the
>> divining line is here in terms of when the answer becomes true - it
>> becomes true when there is a chance that it might render something.
>>
>> Arguably, the user-space comparison against the user-space clip that you
>> added here can never be accurate even if you allow for "false
>> positives".  The clip is rasterized and whole pixels are chosen based on
>> various criteria that affect clip rasterization.  Thus, while the
>> theoretical clip is at N.5, our rasterization choice has us render
>> anything that hits pixel N, even if the contribution of the rendered
>> primitive is only for the first half of N.  That pixel might be rendered
>> if anything hits any part of it, depending on what rendering operation
>> is being done.
>>
>> So, your "fix" actually breaks the functionality because it is quite
>> possible that something with a bounding box that stops before N.5 might
>> affect pixel N and cause it to be rendered even though your new answer
>> suggested that it wouldn't happen.  Your code might actually cause a
>> bug, not fix one.
>>
>> (There are also some potential theoretical failures related to how AA
>> and STROKE_CONTROL might perturb our rendering, effects which are ignore
>> by the current implementation of hitClip(), however I believe that all
>> of those effects fit within the current implementation - it's just that
>> I don't think anyone has ever proven this, or written an exhaustive test
>> suite to verify that none of our rendering hints can perturb rendering
>> by enough to create some surprises here...)
>>
>>             ...jim
>>
>> On 6/23/16 3:00 PM, Jim Graham wrote:
>>> Since "return true" would be a compliant implementation of
>>> Graphics.hitClip(), this is not a bug...
>>>
>>> Read the documentation, it is allowed to use fast math that can return
>>> true when technically the answer is false...
>>>
>>>             ...jim
>>>
>>> On 6/23/16 5:04 AM, Alexandr Scherbatiy wrote:
>>>>
>>>> Hello,
>>>>
>>>> Could you review the fix:
>>>>   bug: https://bugs.openjdk.java.net/browse/JDK-8160124
>>>>   webrev: http://cr.openjdk.java.net/~alexsch/8160124/webrev.00
>>>>
>>>>   Let's set the clip [x=5, y=5, width=5, height=5] to a graphics and
>>>> call the hitClip() with the passed rectangle [x=0,
>>>> y=0, width=5, height=5].
>>>>
>>>>   The result is false for the graphics with scale 1 and true if the
>>>> scale is floating point 1.5.
>>>>
>>>>   This is because the transformed clip which has floating point
>>>> bounds [7.5, 7.5, 7.5, 7.5] for the scale 1.5 has bounds
>>>> with rounded down upper-left  and rounded up lower-right corners [7,
>>>> 7, 8, 8] which now intersects with the transformed
>>>> rectangle [0, 0, 7.5, 7.5].
>>>>
>>>>   The proposed fix adds additional check for the user clip and the
>>>> user rectangle intersection if the intersection with
>>>> the region clip passes.
>>>>
>>>>  Thanks,
>>>>  Alexandr.
>>>>
>>>>
>
>