Skip to content

Fix BoundingBox.merge() inconsistency — deprecate merge() and introduce mergeWith()#2661

Open
Copilot wants to merge 3 commits intomasterfrom
copilot/fix-bounding-box-merge-behavior
Open

Fix BoundingBox.merge() inconsistency — deprecate merge() and introduce mergeWith()#2661
Copilot wants to merge 3 commits intomasterfrom
copilot/fix-bounding-box-merge-behavior

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 28, 2026

BoundingBox.merge() was delegating directly to mergeLocal(), silently mutating this — inconsistent with BoundingSphere.merge(), which correctly returns a new object. Rather than changing the behavior of merge() (which would break existing apps relying on the mutating behavior), the old method is deprecated and a new non-mutating mergeWith() method is introduced.

Changes

  • BoundingVolume.merge() (abstract): marked @Deprecated with a Javadoc note explaining the inconsistency across subclasses and directing users to mergeWith()
  • BoundingVolume.mergeWith(): new concrete method that always returns a new non-mutating result via clone(null).mergeLocal(volume)
  • BoundingBox.merge(): restored to its original mutating behavior (return mergeLocal(volume)), marked @Deprecated with a note that it modifies the receiver in place
  • BoundingSphere.merge(): marked @Deprecated for consistency, directing users to mergeWith()
  • TestBoundingBox: added testMergeModifiesReceiver() verifying the deprecated merge() still mutates in place, and testMergeWithDoesNotModifyReceiver() verifying mergeWith() is non-mutating
BoundingBox bb1 = new BoundingBox(new Vector3f(0f, 0f, 0f), 1f, 1f, 1f);
BoundingBox bb2 = new BoundingBox(new Vector3f(4f, 0f, 0f), 1f, 1f, 1f);

// Deprecated — still mutates bb1 for backward compatibility:
BoundingVolume result = bb1.merge(bb2);

// New non-mutating alternative — bb1 is unchanged:
BoundingVolume merged = bb1.mergeWith(bb2);

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

Copilot AI changed the title [WIP] Fix BoundingBox.merge() to avoid modifying object in place Fix BoundingBox.merge() mutating receiver instead of returning new instance Mar 28, 2026
Copilot AI requested a review from riccardobl March 28, 2026 09:50
@riccardobl
Copy link
Copy Markdown
Member

@copilot This is going to break existing apps relying on the current behavior, you should deprecate the old method and annotate why it was deprecated, and create a new method that is not local

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 28, 2026

@copilot This is going to break existing apps relying on the current behavior, you should deprecate the old method and annotate why it was deprecated, and create a new method that i...

Done in commit 10e1347:

  • Reverted BoundingBox.merge() back to its original mutating behavior (return mergeLocal(volume)) to preserve backward compatibility
  • Deprecated merge() on BoundingVolume (abstract), BoundingBox, and BoundingSphere, each with a Javadoc note explaining why and pointing to mergeWith()
  • Added a new concrete BoundingVolume.mergeWith(BoundingVolume) method that always creates a new non-mutating result via clone(null).mergeLocal(volume)

Copilot AI changed the title Fix BoundingBox.merge() mutating receiver instead of returning new instance Fix BoundingBox.merge() inconsistency — deprecate merge() and introduce mergeWith() Mar 28, 2026
@riccardobl riccardobl marked this pull request as ready for review March 28, 2026 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: BoundingBox.merge() modifies object in place

2 participants