From f51fb3d1616e889fdcffbf6d0baec1deefcf8869 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 09:43:52 +0000 Subject: [PATCH 1/3] Initial plan From 3efd6df723935a9422b9fcbc46ae89de0ed82457 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 09:49:46 +0000 Subject: [PATCH 2/3] 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> --- .../java/com/jme3/bounding/BoundingBox.java | 12 ++++---- .../com/jme3/bounding/TestBoundingBox.java | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java b/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java index 6b0e023b34..746041c952 100644 --- a/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java +++ b/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java @@ -404,18 +404,18 @@ public Plane.Side whichSide(Plane plane) { } /** - * merge combines this bounding box locally with a second - * bounding volume. The result contains both the original box and the second - * volume. + * merge combines this bounding box with a second bounding + * volume. The result contains both the original box and the second volume. + * The current instance is unaffected. * * @param volume the bounding volume to combine with this box (or null) (not * altered) - * @return this box (with its components modified) or null if the second - * volume is of some type other than AABB or Sphere + * @return a new BoundingBox, or null if the second volume is of some type + * other than AABB or Sphere */ @Override public BoundingVolume merge(BoundingVolume volume) { - return mergeLocal(volume); + return clone(new BoundingBox()).mergeLocal(volume); } /** diff --git a/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java b/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java index 7a48e553b6..d70abeca48 100644 --- a/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java +++ b/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java @@ -71,6 +71,34 @@ public void testEquals() { Assert.assertEquals(bb5, bb6); // because check planes are ignored } + /** + * Verify that merge() does not modify the original bounding box, and that + * the returned box contains both inputs. This was a bug: merge() used to + * call mergeLocal(), which modifies the receiver in place. + */ + @Test + public void testMergeDoesNotModifyReceiver() { + BoundingBox bb1 = new BoundingBox(new Vector3f(0f, 0f, 0f), 1f, 1f, 1f); + BoundingBox bb2 = new BoundingBox(new Vector3f(4f, 0f, 0f), 1f, 1f, 1f); + + // Record the original state of bb1 before merging. + BoundingBox bb1Before = (BoundingBox) bb1.clone(); + + BoundingVolume result = bb1.merge(bb2); + + // bb1 must be unmodified. + Assert.assertEquals(bb1Before, bb1); + + // The result must be a different object than bb1. + Assert.assertNotSame(bb1, result); + + // The result must contain bb2's extent (i.e. the merged region). + Assert.assertTrue(result instanceof BoundingBox); + BoundingBox merged = (BoundingBox) result; + Assert.assertTrue(merged.contains(bb2.getCenter())); + Assert.assertTrue(merged.contains(bb1.getCenter())); + } + /** * Verify that isSimilar() behaves as expected. */ From 10e134702f611102277ea033bebb02a1d9497fda Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 09:57:31 +0000 Subject: [PATCH 3/3] Deprecate merge(), restore original BoundingBox.merge() behavior, add mergeWith() Agent-Logs-Url: https://github.com/jMonkeyEngine/jmonkeyengine/sessions/223ff945-a733-4b7b-8b8b-2e90418270de Co-authored-by: riccardobl <4943530+riccardobl@users.noreply.github.com> --- .../java/com/jme3/bounding/BoundingBox.java | 16 ++++++---- .../com/jme3/bounding/BoundingSphere.java | 2 ++ .../com/jme3/bounding/BoundingVolume.java | 17 ++++++++++ .../com/jme3/bounding/TestBoundingBox.java | 32 +++++++++++++++---- 4 files changed, 55 insertions(+), 12 deletions(-) diff --git a/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java b/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java index 746041c952..079d96bfe3 100644 --- a/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java +++ b/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java @@ -404,18 +404,22 @@ public Plane.Side whichSide(Plane plane) { } /** - * merge combines this bounding box with a second bounding - * volume. The result contains both the original box and the second volume. - * The current instance is unaffected. + * merge combines this bounding box locally with a second + * bounding volume. The result contains both the original box and the second + * volume. * * @param volume the bounding volume to combine with this box (or null) (not * altered) - * @return a new BoundingBox, or null if the second volume is of some type - * other than AABB or Sphere + * @return this box (with its components modified) or null if the second + * volume is of some type other than AABB or Sphere + * @deprecated This method modifies the receiver in place, which is + * inconsistent with {@link BoundingSphere#merge}. Use + * {@link #mergeWith} instead. */ + @Deprecated @Override public BoundingVolume merge(BoundingVolume volume) { - return clone(new BoundingBox()).mergeLocal(volume); + return mergeLocal(volume); } /** diff --git a/jme3-core/src/main/java/com/jme3/bounding/BoundingSphere.java b/jme3-core/src/main/java/com/jme3/bounding/BoundingSphere.java index 67cf7263f8..e4dda2287d 100644 --- a/jme3-core/src/main/java/com/jme3/bounding/BoundingSphere.java +++ b/jme3-core/src/main/java/com/jme3/bounding/BoundingSphere.java @@ -465,7 +465,9 @@ public Plane.Side whichSide(Plane plane) { * @param volume * the sphere to combine with this sphere. * @return a new sphere + * @deprecated Use {@link #mergeWith} instead. */ + @Deprecated @Override public BoundingVolume merge(BoundingVolume volume) { if (volume == null) { diff --git a/jme3-core/src/main/java/com/jme3/bounding/BoundingVolume.java b/jme3-core/src/main/java/com/jme3/bounding/BoundingVolume.java index 3a80764910..c9d226c9e0 100644 --- a/jme3-core/src/main/java/com/jme3/bounding/BoundingVolume.java +++ b/jme3-core/src/main/java/com/jme3/bounding/BoundingVolume.java @@ -149,6 +149,18 @@ public final BoundingVolume transform(Transform trans) { */ public abstract void computeFromPoints(FloatBuffer points); + /** + * Combines this bounding volume with a second bounding volume so that the + * result contains both volumes. Returns a new bounding volume without + * modifying either input. + * + * @param volume the volume to combine with this one (may be null) + * @return a new bounding volume containing both volumes + */ + public BoundingVolume mergeWith(BoundingVolume volume) { + return clone(null).mergeLocal(volume); + } + /** * merge combines two bounding volumes into a single bounding * volume that contains both this bounding volume and the parameter volume. @@ -156,7 +168,12 @@ public final BoundingVolume transform(Transform trans) { * @param volume * the volume to combine. * @return the new merged bounding volume. + * @deprecated This method has inconsistent behavior across subclasses + * ({@link BoundingBox#merge} modifies the receiver, while + * {@link BoundingSphere#merge} returns a new instance). + * Use {@link #mergeWith} instead. */ + @Deprecated public abstract BoundingVolume merge(BoundingVolume volume); /** diff --git a/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java b/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java index d70abeca48..ed0c7c3408 100644 --- a/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java +++ b/jme3-core/src/test/java/com/jme3/bounding/TestBoundingBox.java @@ -72,27 +72,47 @@ public void testEquals() { } /** - * Verify that merge() does not modify the original bounding box, and that - * the returned box contains both inputs. This was a bug: merge() used to - * call mergeLocal(), which modifies the receiver in place. + * Verify that merge() still modifies the original BoundingBox in place + * (deprecated behavior preserved for backward compatibility). */ @Test - public void testMergeDoesNotModifyReceiver() { + @SuppressWarnings("deprecation") + public void testMergeModifiesReceiver() { BoundingBox bb1 = new BoundingBox(new Vector3f(0f, 0f, 0f), 1f, 1f, 1f); BoundingBox bb2 = new BoundingBox(new Vector3f(4f, 0f, 0f), 1f, 1f, 1f); - // Record the original state of bb1 before merging. BoundingBox bb1Before = (BoundingBox) bb1.clone(); BoundingVolume result = bb1.merge(bb2); + // merge() delegates to mergeLocal(), so bb1 IS modified. + Assert.assertNotEquals(bb1Before, bb1); + + // The result is the same object as bb1. + Assert.assertSame(bb1, result); + } + + /** + * Verify that mergeWith() does not modify the original bounding box, and + * that the returned box contains both inputs. + */ + @Test + public void testMergeWithDoesNotModifyReceiver() { + BoundingBox bb1 = new BoundingBox(new Vector3f(0f, 0f, 0f), 1f, 1f, 1f); + BoundingBox bb2 = new BoundingBox(new Vector3f(4f, 0f, 0f), 1f, 1f, 1f); + + // Record the original state of bb1 before merging. + BoundingBox bb1Before = (BoundingBox) bb1.clone(); + + BoundingVolume result = bb1.mergeWith(bb2); + // bb1 must be unmodified. Assert.assertEquals(bb1Before, bb1); // The result must be a different object than bb1. Assert.assertNotSame(bb1, result); - // The result must contain bb2's extent (i.e. the merged region). + // The result must contain both inputs' centers. Assert.assertTrue(result instanceof BoundingBox); BoundingBox merged = (BoundingBox) result; Assert.assertTrue(merged.contains(bb2.getCenter()));