Skip to content

Conversation

mickleness
Copy link
Contributor

@mickleness mickleness commented Aug 10, 2025

4197755 is marked as a duplicate of JDK-8176501 , but I think that is a mistake. The complaint in that ticket still reproduces, and it is resolved in this PR.

The complaint is: Arc2D.getBounds() is too large. If an Arc2D represents a small slice of an ellipse, then Arc2D.getBounds() always returns the size of the total ellipse. Arc2D.getBounds2D(), by contrast, has been "high-precision" for decades.

This PR makes Arc2D.getBounds() resemble the same implementation already used in Area, CubicCurve2D, Line2D, Path2D, and QuadCurve2D:

    public Rectangle getBounds() {
        return getBounds2D().getBounds();
    }

This modifies (simplifies) the javadoc for Arc2D.getBounds2D(). If we generally like this PR I assume we'll need a CSR to approve the javadoc change.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-4197755: Arc2D.getBounds() returns an unnecessarily large bounding box (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26715/head:pull/26715
$ git checkout pull/26715

Update a local copy of the PR:
$ git checkout pull/26715
$ git pull https://git.openjdk.org/jdk.git pull/26715/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26715

View PR using the GUI difftool:
$ git pr show -t 26715

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26715.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 10, 2025

👋 Welcome back jwood! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Aug 10, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Aug 10, 2025

@mickleness The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added client [email protected] rfr Pull request is ready for review labels Aug 10, 2025
@mlbridge
Copy link

mlbridge bot commented Aug 10, 2025

Webrevs

@bourgesl
Copy link
Contributor

LGTM

* the starting and ending angles of this {@code Arc2D}.
*
* @return the {@code Rectangle2D} that represents the arc's
* framing rectangle.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused. Isn't this deleted doc, in fact the doc that you want on both methods now.
Although 'high precision' if it means float, not int return coords, might not be part of the other method spec.
Also there is a JCK test that fails with this change. Looks like it expects the looser bounds for getBounds().
So we'd definitely need a CSR.
And there'd be a practical compatibility issue as well for someone who relied on those looser bounds.
That's what worries me most.

Also (minor) from JBS I see this comment -
"It looks like the math in getBounds2D could be greatly simplified - at which
point it would make sense to have getBounds also return a tighter bounding
box."

which suggests to me that performance was part of the original reason for the looser bounds of getBounds()

Copy link
Contributor Author

@mickleness mickleness Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Everything that follows is very subjective; feel free to disagree.)

Isn't this deleted doc, in fact the doc that you want on both methods now..

The previous javadoc existed because it was explaining a very unusual distinction between Arc2D.getBounds() and Arc2D.getBounds2D() that does not exist in any other Shape. That distinction/difference is the crux of JDK-4197755. It was worth explaining precisely because it was unintuitive/weird.

IMO we don't need any additional javadoc around these methods now (with this PR), because now they are behaving like most developers assume they would. Most other Shape implementations already follow the same pattern, and they don't offer any accompanying method documentation. If I was redesigning the Shape interface today from scratch, I would probably go so far as to include:

    default public Rectangle getBounds() {
        return getBounds2D().getBounds();
    }

    default public Rectangle2D getBounds2D() {
        return Path2D.getBounds2D(getPathIterator(null));
    }

I'd argue this is the "normal" behavior that most developers expect. We should only override those methods if a specific alternative is faster and approximately as accurate.

(This case study is also weird because Arc2D extends RectangularShape. The shape of a slice of pie does not have a "is-a" relationship with the shape of its pie tin. And yet without this PR: Arc2D inherits getBounds() from RectangularShape.)

And there'd be a practical compatibility issue as well for someone who relied on those looser bounds.

I agree; that's always a risk. We took the same risk when we resolved JDK-8176501 ; is there room to argue this PR is riskier than JDK-8176501 's resolution?

If this PR is too controversial: I'm fine with closing it. I see you reopened JDK-4197755, which is the thing I feel most strongly about here. (That is: keeping the ticket database up-to-date and fixing 4197755's status is enough for me.)

I just thought if I was going to call out reopening 4197755: it'd be trivial to also submit this PR for consideration.

which suggests to me that performance was part of the original reason for the looser bounds of getBounds()

Hmm. Maybe. I'd just add: RectangularShape provides a getBounds() implementation, but it does NOT provide a getBounds2D() implementation. So I'd suggest getBounds() is looser simply because of legacy design decisions. I do think it's peculiar the Arc2D authors anticipated this complaint and didn't act on it, but I'll probably never know exactly how those decisions were made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client [email protected] rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

3 participants