Skip to content

Commit 3efd6df

Browse files
Copilotriccardobl
andcommitted
Fix BoundingBox.merge() to not modify receiver in place
Agent-Logs-Url: https://github.com/jMonkeyEngine/jmonkeyengine/sessions/e27f3192-530d-4525-aa2a-ad3e611ae4ed Co-authored-by: riccardobl <4943530+riccardobl@users.noreply.github.com>
1 parent f51fb3d commit 3efd6df

2 files changed

Lines changed: 34 additions & 6 deletions

File tree

jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -404,18 +404,18 @@ public Plane.Side whichSide(Plane plane) {
404404
}
405405

406406
/**
407-
* <code>merge</code> combines this bounding box locally with a second
408-
* bounding volume. The result contains both the original box and the second
409-
* volume.
407+
* <code>merge</code> combines this bounding box with a second bounding
408+
* volume. The result contains both the original box and the second volume.
409+
* The current instance is unaffected.
410410
*
411411
* @param volume the bounding volume to combine with this box (or null) (not
412412
* altered)
413-
* @return this box (with its components modified) or null if the second
414-
* volume is of some type other than AABB or Sphere
413+
* @return a new BoundingBox, or null if the second volume is of some type
414+
* other than AABB or Sphere
415415
*/
416416
@Override
417417
public BoundingVolume merge(BoundingVolume volume) {
418-
return mergeLocal(volume);
418+
return clone(new BoundingBox()).mergeLocal(volume);
419419
}
420420

421421
/**

jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,34 @@ public void testEquals() {
7171
Assert.assertEquals(bb5, bb6); // because check planes are ignored
7272
}
7373

74+
/**
75+
* Verify that merge() does not modify the original bounding box, and that
76+
* the returned box contains both inputs. This was a bug: merge() used to
77+
* call mergeLocal(), which modifies the receiver in place.
78+
*/
79+
@Test
80+
public void testMergeDoesNotModifyReceiver() {
81+
BoundingBox bb1 = new BoundingBox(new Vector3f(0f, 0f, 0f), 1f, 1f, 1f);
82+
BoundingBox bb2 = new BoundingBox(new Vector3f(4f, 0f, 0f), 1f, 1f, 1f);
83+
84+
// Record the original state of bb1 before merging.
85+
BoundingBox bb1Before = (BoundingBox) bb1.clone();
86+
87+
BoundingVolume result = bb1.merge(bb2);
88+
89+
// bb1 must be unmodified.
90+
Assert.assertEquals(bb1Before, bb1);
91+
92+
// The result must be a different object than bb1.
93+
Assert.assertNotSame(bb1, result);
94+
95+
// The result must contain bb2's extent (i.e. the merged region).
96+
Assert.assertTrue(result instanceof BoundingBox);
97+
BoundingBox merged = (BoundingBox) result;
98+
Assert.assertTrue(merged.contains(bb2.getCenter()));
99+
Assert.assertTrue(merged.contains(bb1.getCenter()));
100+
}
101+
74102
/**
75103
* Verify that isSimilar() behaves as expected.
76104
*/

0 commit comments

Comments
 (0)