[OpenJDK 2D-Dev]  Request for review: 8004859 Graphics.getClipBounds/getClip return difference nonequivalent bounds, depending from transform.
Sergey.Bylokhov at oracle.com
Mon Jun 10 12:49:19 UTC 2013
Additional note is that this fix is targeted to 7u40.
On 06.06.2013 22:38, Sergey Bylokhov wrote:
> HI, Jim.
> Can you review the updated version of the fix:
> I decided to implement an option, where transformed graphics
> (identity, translated, scaled) returns equivalent clip. For this in
> the transformShape() we preserve orientation of Rectangle, and in the
> getClipBounds() we do not lose this information because we use
> getBounds2D() instead of getBounds(). Note that I have simplified
> getClipBounds() from the previous version:
> Also there is no additional check for empty rectangles.
>> The original code in getClipBounds would end up returning a "new
>> Rectangle()" if the clip was an empty rectangle due to the way that
>> "Rectangle2D/Path2D.getBounds()" works. You now use
>> setFrame(getBounds2D()) which will attempt to preserve the dimensions
>> of empty clips. So, the "preserve the size of an empty clip" support
>> is new.
> Yes it is new in case of Rectangle2D, but now it works in the same way
> as for Rectangle.
> On 23.04.2013 1:11, Jim Graham wrote:
>> I think if empty (un)transforms to something else that is also
>> classified as empty naturally without having to special case it or
>> test it then returning the "transformed empty thing" is a fine return
>> But, if it comes down to "Eeek, it was empty so I need to calculate a
>> specifically similar empty thing to return" then I feel that the test
>> should just result in "return new Rectangle()". No use pulling hair
>> out to make a "more useful non-answer" unless it falls out of the
>> calculations for free - as in "unless it avoids even having to check
>> for the exceptional condition in the first place". In the cases I
>> reviewed I seem to recall that we were doing the tests anyway (or we
>> were doing equally complex tests in order to normalize the answer
>> even if we never explicitly tested for "isEmpty()") so it would make
>> the code much less problematic to just weed the empty answers out early.
>> As far as "are any Developers expecting a usefully non-degenerate
>> empty answer" then I don't think so...
>> On 4/18/13 6:24 AM, Sergey Bylokhov wrote:
>>> Hello Jim.
>>> On 1/17/13 4:56 AM, Jim Graham wrote:
>>>> The original code in getClipBounds would end up returning a "new
>>>> Rectangle()" if the clip was an empty rectangle due to the way that
>>>> "Rectangle2D/Path2D.getBounds()" works. You now use
>>>> setFrame(getBounds2D()) which will attempt to preserve the dimensions
>>>> of empty clips. So, the "preserve the size of an empty clip" support
>>>> is new.
>>>> I then mentioned that we don't need to go out of our way to preserve
>>>> the dimensions of an empty clip, but you seem to be saying that we
>>>> don't want to change this behavior - but your new code represents the
>>>> break with existing behaviors, no?
>>> I returned to this problem and studied it a little more deeply.
>>> Assume we set clip=Rectangle[100, 100,-100,-100] for sg2d with a
>>> different transform.
>>> 1 identity/translate (default mode for non-retina): getClip () will
>>> return Rectangle[100, 100,-100,-100].
>>> 2 scale (default mode for retina): getClip() will return
>>> Rectangle[0,0,100,100] <- bug and it should be fixed.
>>> 3 generic: getClip will return Rectangle[0,0,0,0], because we convert
>>> Rectangle to the Shape through RectIterator, which ignores the negative
>>> width and height. Note that x and y were not preserved only if w and
>>> h<0, but if w = h =0, then x and y will be preserved.
>>> In an original fix I made case 2 like case 1, so from the point of view
>>> of users there was no difference in case of retina display.
>>> But now I have doubts and I think that all cases shall work equally
>>> in case 3.
>>> What do you think?
Best regards, Sergey.
More information about the 2d-dev