[OpenJDK 2D-Dev] [8] Request for review: 8004859 Graphics.getClipBounds/getClip return difference nonequivalent bounds, depending from transform.

[Sergey Bylokhov]

Hi Jim.
03.01.2013 5:10, Jim Graham wrote:
> Hi Sergey,
>
> I guess my primary goal in maintaining this code is to try to find the 
> simplest and most directed fix unless a rewrite is in order - i.e. 
> simply normalizing incoming empty clips so that we end up in a stable 
> "clip is empty" state would be my preference, but perhaps the proposed 
> fix isn't so complicated that I need to worry about that.
Can you please review the updated version of the fix:
http://cr.openjdk.java.net/~serb/8004859/webrev.02
>
> I've looked deeper into the various Rectangle methods and found lots 
> of little issues with the way that the various bounds methods on 
> Rectangle/Rectangle2D are implemented that complicate all of this.  I 
> think a larger rewrite of the clip maintenance code is likely in order 
> at some point.  In particular, while your changes have shifted from 
> one set of problematic bounds-setting code to another and may have 
> solved some test case we are currently looking at, I'm not convinced 
> that other bugs, or perhaps even just as many different bugs, are not 
> waiting to bite us with the new code.  :(
Yes some cases in methods are strange. Especially considering that they 
in one hierarchy.
>
> In the meantime, with respect to the new method you've added, I'd like 
> to see the following changes:
>
> - The method has nothing really to do with "normalizing a matrix" and 
> is probably misled by the fact that the "matrix" variable in the 
> calling method should never have been called "matrix" in the first 
> place since it is really just a list of coordinates.  I'd suggest a 
> name better suited to its purpose, like "fixRectangleOrientation" or 
> "checkEmptyRectangle".
done
>
> - For performance - rather than call signum 4 times, why not use more 
> direct tests like:
>
>     if ((clip.getWidth() > 0) != (m[2] - m[0] > 0))
>     // i.e. we only care if their "non-emptiness" is different
> or:
>     if (clip.getWidth() * (m[2] - m[0]) < 0)
>     // i.e. we only care if their non-zero size is flipped
done
>
> - I still think that we don't owe anybody any need to preserve the 
> size of the empty clip so the adjustments for cases that fail the 
> above tests could simply be "m[2] = m[0]" and thus we get an empty 
> result.  Why did you feel that we need to maintain the sizes of empty 
> rectangles?  Was it just JCK test failures? 
Well I have only one reason to do it: If the user's code works with 
non-scaled Graphics, which is default, and it will fail when we change 
internal type of the graphics  -> That's a problem. This is directly 
related to the retina support, when in general nothing should be changed 
for the existing applications.

> Does flipping the results keep us from failing those tests?
>
> ---------------------
>
> Eventually I'd like to see the following simplifications to our SG2D 
> clip handling:
>
> - we only keep a shape around so we can return a shape and we spend a 
> lot of time maintaining it.  I'd like to see the shape be only 
> optionally kept if there is a non-rectangle handed in. Otherwise we 
> just maintain the clipRegion.
>
> - we use Area to intersect non-rectangular shapes even though it is 
> rare for anyone to call getClip() and even rarer that they do anything 
> with it except to use setClip() on it to restore the clip.  I'd rather 
> see keeping an array of the incoming shapes, return a special object 
> from getClip() that just holds that array, and then on setClip() 
> simply restore the region - thus avoiding ever having to use Area.  
> Only if they actually iterate the path of the returned getClip() would 
> it dynamically use Area on the fly to produce that path iteration.
>
> - Avoid use of methods on Rectangle[2D] and write our own code that 
> ensures that we perform the correct flipping and integer 
> normalization.  Most of that would happen in the Region bounds code if 
> we do the above anyway and that code is much simpler than some of the 
> corresponding methods on Rectangle[2D], plus we can fix bugs in there 
> without worrying about possibly changing the behavior for other apps 
> that may have started to rely on what we see as "bugs".
>
>             ...jim
>
> On 12/21/2012 3:21 AM, Sergey Bylokhov wrote:
>> 21.12.2012 6:05, Sergey Bylokhov пишет:
>>> Hi, Jim.
>>> 21.12.2012 4:59, Jim Graham wrote:
>>>> The Object.equals() method is not intended to compare geometries.
>>>> While Area.equals() attempts to perform geometric comparison I think
>>>> that was a bad idea in retrospect for many reasons:
>>>>
>>>> - In practice you can only really compare within a tolerance due to
>>>> the many ways for computations to every so slightly shift the results
>>>>
>>>> - There is no decent way to satisfy the equals/hashcode contract with
>>>> such a comparison so Area is forever broken with respect to that
>>>> contract.
>>>>
>>>> - It's a complicated test that can only serve to convince developers
>>>> to invite performance drains into their code by making them think it
>>>> is a reasonable thing to compare.
>>>>
>>>> For testing we should probably use Shape.contains() to verify answers
>>>> for rectangle arguments and if we want to perform verification on
>>>> arbitrary shape arguments then we'd need to write our own fuzzy
>>>> comparator.
>>> But why fix is incorrect? It just use the same code as sffd for the
>>> correct shapes and have additional code for incorrect. Only
>>> conditionchanged from width/height should be positive, to
>>> width/height should be the same direction as incoming rectangle.
>>>>>> There isn't even any guarantee that we will hand them back a 
>>>>>> rectangle
>>>>>> from getClip().
>>>>> One of the jck tests expect this, but I think that this is the wrong
>>>>> test.
>>>>
>>>> There is nothing in the documentation that guarantees a specific type
>>>> returned from the method.  If a JCK test was written to expect a
>>>> Rectangle from the method then that is a separate problem that we'll
>>>> have to address at some point.  For now I'm only suggesting modifying
>>>> the process for negative-empty rectangles.  Does that trigger the JCK
>>>> failure?  If so, we are still satisfying the documented spec and so
>>>> we will simply have to confince JCK to allow this exception.
>>> Problem in jck come from the fact that Rectangle differs from
>>> Rectangle2d. Note that everything is ok if graphics is not transformed
>>> or just translated. But if we scale graphics we get:
>>> 1 getClip() return non equivalent object.
>>> 2 getClipBounds for Rectangle return it as is, but for Rectangle2D it
>>> is call getBounds, which return an empty bounds or wrong bounds if
>>> rectangle2d was normalized by sffd.
>>> 3 Some tests expects class equivalence between set/get clip. I'll
>>> prepare CR for them soon.
>>>
>>> Also there is the difference in the real clip, if we use type for the
>>> empty Rectangle to Rectangle2D the real clip became correct.
>> typo:  Also there is the difference in the real clip, if we change type
>> from the empty Rectangle to the empty Rectangle2D, the real clip will
>> become correct.
>>> All these issues are carried out in the testcase.
>>>>>> After all, if you end up in the drop through case then we hand the
>>>>>> paths to Area to do intersections and Area will do all sorts of
>>>>>> surgery on the geometry.
>>>>> But I sure this area will be equal to the user's clip?
>>>>
>>>> For all practical purposes it should be, but not only will the Area
>>>> code encapsulate the answer in a different class (even if the result
>>>> is a rectangle), it may reverse the order of the segments, it may
>>>> represent the area as two separate rectangles that abut each other in
>>>> a way that they are equivalent to one larger rectangle, but the Area
>>>> code failed to notice that optimization, etc.  All it will guarantee
>>>> is that it returns a shape that describes the same interior, it will
>>>> make no guarantees about minimalism or association of segments...
>>>>
>>>>             ...jim
>>>
>>>
>>> -- 
>>> Best regards, Sergey.
>>
>>
>> -- 
>> Best regards, Sergey.
>>


-- 
Best regards, Sergey.