-
Notifications
You must be signed in to change notification settings - Fork 6.2k
4197755: Arc2D.getBounds() returns an unnecessarily large bounding box #26715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
mickleness
wants to merge
4
commits into
openjdk:master
Choose a base branch
from
mickleness:JDK-4197755
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+100
−14
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
/* | ||
* Copyright (c) 1999, 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
/* | ||
* @test | ||
* @bug 4197755 | ||
* @summary Verifies that Arc2D.getBounds() is similar to Arc2D.getBounds2D() | ||
*/ | ||
|
||
import java.awt.geom.Arc2D; | ||
import java.awt.geom.Point2D; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class Arc2DGetBoundsTest { | ||
public static void main(String[] args) { | ||
// Imagine a circle that represents a compass. | ||
// This arc represents the northern / top quarter. | ||
Arc2D arc = new Arc2D.Double(0, 0, 1000, 1000, 45, 90, Arc2D.PIE); | ||
|
||
// Create 8 pie slices, and place a dot in the center of each | ||
List<Point2D> samples = new ArrayList<>(); | ||
for (int segment = 0; segment < 8; segment++) { | ||
double theta = -(segment + .5) / 8.0 * 2 * Math.PI; | ||
Point2D p = new Point2D.Double( | ||
500 + 100 * Math.cos(theta), | ||
500 + 100 * Math.sin(theta) | ||
); | ||
samples.add(p); | ||
} | ||
|
||
// these assertions have never been known to fail: | ||
assertTrue(!arc.contains(samples.get(0))); | ||
assertTrue(arc.contains(samples.get(1))); | ||
assertTrue(arc.contains(samples.get(2))); | ||
assertTrue(!arc.contains(samples.get(3))); | ||
assertTrue(!arc.contains(samples.get(4))); | ||
assertTrue(!arc.contains(samples.get(5))); | ||
assertTrue(!arc.contains(samples.get(6))); | ||
assertTrue(!arc.contains(samples.get(7))); | ||
|
||
assertTrue(arc.getBounds2D().contains(samples.get(0))); | ||
assertTrue(arc.getBounds2D().contains(samples.get(1))); | ||
assertTrue(arc.getBounds2D().contains(samples.get(2))); | ||
assertTrue(arc.getBounds2D().contains(samples.get(3))); | ||
assertTrue(!arc.getBounds2D().contains(samples.get(4))); | ||
assertTrue(!arc.getBounds2D().contains(samples.get(5))); | ||
assertTrue(!arc.getBounds2D().contains(samples.get(6))); | ||
assertTrue(!arc.getBounds2D().contains(samples.get(7))); | ||
|
||
|
||
assertTrue(arc.getBounds().contains(samples.get(0))); | ||
assertTrue(arc.getBounds().contains(samples.get(1))); | ||
assertTrue(arc.getBounds().contains(samples.get(2))); | ||
assertTrue(arc.getBounds().contains(samples.get(3))); | ||
|
||
// these are the assertions that failed before resolving 4197755 | ||
assertTrue(!arc.getBounds().contains(samples.get(4))); | ||
assertTrue(!arc.getBounds().contains(samples.get(5))); | ||
assertTrue(!arc.getBounds().contains(samples.get(6))); | ||
assertTrue(!arc.getBounds().contains(samples.get(7))); | ||
} | ||
|
||
private static void assertTrue(boolean b) { | ||
if (!b) | ||
throw new Error(); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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()
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.)
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: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.)
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.
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.