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

[Jim Graham]

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.

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.  :(

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".

- 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

- 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?  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.
>