Fix merging bounding spheres with nearby centers

[toaster0123]

BoundingSphere.mergeLocal() can return a sphere that does not contain both inputs when their centers are less than roughly one world unit apart.

For example, merging two radius-1 spheres centered at (0, 0, 0) and (0.5, 0, 0) produces center (0, 0, 0) and radius 1.25. The second sphere's point (1.5, 0, 0) is outside that result. The correct merged center is (0.25, 0, 0), with the same radius 1.25.

The merge uses RADIUS_EPSILON, whose value is 1.00001, as the minimum distance for adjusting the center. Change that check to a positive distance while retaining the finite-distance guard. Write the adjusted center directly into the result so the newly reached path does not mutate the receiver of deprecated merge(). Add two focused regressions for sphere/sphere and sphere/box merges, covering merge(), mergeLocal(), and mergeWith().

Validation:

• Both new regressions fail on unmodified master dda27e4
• 155 math/bounds tests pass after the fix, with one existing disabled test
• 128 sphere-pair controls pass in both merge orders, covering coincident/nearby/separated centers and different radii
• All core main/plugin/tool Java sources compile with the Java 8 target and repository warning flags

These are local checks using Temurin 21 and JUnit 5.13.4. The full Gradle build, the configured Java 25/JUnit 6 toolchain, and CI have not been run.

[jaime-jmebot]

Good fix on the epsilon — it was rejecting any center pair closer than ~1 world unit. One gap to close though: the newly reachable range now includes pairs where one volume already contains the other, and the shift overshoots there. Merging a radius-1 sphere at (0,0,0) with a radius-3 sphere at (0.5,0,0) moves the center by (0.5 + 2) / 2 = 1.25 units toward the bigger sphere, landing at (1.25,0,0) — past it — with radius (1 + 3 + 0.5) / 2 = 2.25, so the bigger volume isn't covered at all. Could you skip the center adjustment (and keep the larger volume) when length + Math.min(radius, temp_radius) <= Math.max(radius, temp_radius), or confirm an earlier early-return already handles containment? That pair would make a good regression too.

[toaster0123]

Containment is already handled by the early return at lines 598–612. For your radius-1/radius-3 pair, radiusDiff² = 4 and lengthSquared = 0.25, so the adjustment branch is never reached. I ran that exact pair in both orders through merge(), mergeLocal(), and mergeWith(): all return center (0.5, 0, 0), radius 3, on both the baseline and this head.

The unequal-radius example is useful additional coverage: it fails on the baseline and passes this head, producing center approximately (0.15, 0, 0), radius 2.15. The existing testMergeNearbySphere already checks the deprecated merge() receiver remains unchanged. I also tested the epsilon change alone while retaining addLocal: that existing assertion fails with receiver center (0.25, 0, 0); with the full patch, it passes. Your mergeLocal() example passes in both versions, since writing into rCenter still updates the receiver in that API.

[jaime-jmebot]

You're right — the containment case never reaches the adjustment branch, so my overshoot concern doesn't apply to this diff. With radiusDiff² = 4 against lengthSquared = 0.25 for that radius-1/radius-3 pair, the early return takes over and the bigger volume is what gets returned. Thanks for running it through all three APIs on both baseline and head; that settles it, and I'm withdrawing this concern.

[jaime-jmebot]

Nice coverage of merge() / mergeLocal() / mergeWith(). Both regressions only merge equal-radius spheres, though, so the direction where the receiver is the bigger volume — the one that previously depended on the addLocal side effect this diff removes — is untested. Here's one that fails before the fix and pins down the invariant this PR is about:

[jaime-jmebot]

Suggested change

	/**
	* Verify that merging a smaller sphere into a bigger receiver whose centers
	* are less than RADIUS_EPSILON apart yields a sphere containing both.
	*/
	@Test
	public void testMergeLocalSmallerIntoBiggerSphere() {
	BoundingSphere bigger = new BoundingSphere(2f, new Vector3f(0f, 0f, 0f));
	BoundingSphere smaller = new BoundingSphere(1.5f, new Vector3f(0.8f, 0f, 0f));
	BoundingSphere merged = new BoundingSphere(2f, new Vector3f(0f, 0f, 0f));
	merged.mergeLocal(smaller);
	float tolerance = 1e-5f;
	Assertions.assertTrue(merged.getCenter().distance(bigger.getCenter())
	+ bigger.getRadius() <= merged.getRadius() + tolerance);
	Assertions.assertTrue(merged.getCenter().distance(smaller.getCenter())
	+ smaller.getRadius() <= merged.getRadius() + tolerance);
	}

[jaime-jmebot]

Retracting the suggestion above — you're correct that it passes on baseline too (writing into rCenter still updates the receiver in mergeLocal()), so it doesn't guard anything the epsilon fix changes. Please don't commit it as-is.

Your unequal-radius example is the one that earns its place: receiver radius 2 at the origin merging radius 1.5 at (0.8, 0, 0) lands on center ≈ (0.15, 0, 0) with radius 2.15, and on the baseline the old RADIUS_EPSILON check takes the no-shift path (center (0,0,0), radius 2), which leaves the smaller sphere uncovered since 0.8 + 1.5 = 2.3 > 2. That's exactly the receiver-is-bigger direction I was asking about, and it's a true regression guard.

To be explicit about the remaining side: the addLocal → add change is already pinned by sphere.getCenter() staying at the origin in testMergeNearbySphere — with only the epsilon change applied, that assertion fails. So the fix is well covered as it stands; the unequal-radius test would be a nice extra, not a blocker.

[jaime-jmebot]

Nice catch on the epsilon — RADIUS_EPSILON (1.00001) being used as a minimum distance made any nearby-center merge silently return a sphere that didn't actually cover its inputs, and the fix is nicely minimal.

Two things before merge:

• Containment isn't guarded on the newly reachable path. Now that centers closer than ~1 unit take the adjustment branch, pairs where one volume already contains the other overshoot and drop the larger volume. Details and a suggested guard are in the inline comment.
• The addLocal → add change is correct but untested in the direction that needed it. Both new regressions use equal-radius spheres; the case where the receiver is the bigger volume is exactly where the old code depended on that receiver-mutating side effect. A ready-to-paste regression is in the test thread.

Otherwise the reasoning is sound: writing the offset into rCenter directly is the right call, since it keeps the deprecated merge() receiver unmutated on a path that newly executes for small-center merges.

[jaime-jmebot]

Both concerns from my earlier review are now resolved, and I'm happy with this.

• The containment worry was mine, not the code's: the radius-1/radius-3 case returns from the early-out before the adjustment branch ever runs, so the newly reachable path can't overshoot there.
• The addLocal → add swap is already guarded by testMergeNearbySphere, which fails if only the epsilon check is changed. That's exactly the regression it needed.
• I've dropped my own suggested test — it turned out to pass on baseline too, so it wasn't pulling its weight.

Nice and tight fix. The unequal-radius receiver-is-bigger scenario @toaster0123 outlined (radius 2 at the origin + radius 1.5 at (0.8, 0, 0) → center ≈ (0.15, 0, 0), radius 2.15) would round out the coverage nicely if you feel like adding it, but it's a bonus rather than a requirement.