[OpenJDK 2D-Dev]  Request for review: 8004859 Graphics.getClipBounds/getClip return difference nonequivalent bounds, depending from transform.
Sergey.Bylokhov at oracle.com
Mon Jan 14 19:44:37 UTC 2013
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:
> 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
> - For performance - rather than call signum 4 times, why not use more
> direct tests like:
> if ((clip.getWidth() > 0) != (m - m > 0))
> // i.e. we only care if their "non-emptiness" is different
> if (clip.getWidth() * (m - m) < 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 = m" 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".
> 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
>>>> - 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
>>> 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
>>>>>> from getClip().
>>>>> One of the jck tests expect this, but I think that this is the wrong
>>>> 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...
>>> Best regards, Sergey.
>> Best regards, Sergey.
Best regards, Sergey.
More information about the 2d-dev