Bug: BoundingBox.merge() modifies object in place

[capdevon]

1. The BoundingBox.merge() method internally invokes the BoundingBox.mergeLocal() method, which alters the internal state of the BoundingBox object on which it is invoked. In contrast, the BoundingSphere.merge() method correctly creates a new object without altering the internal state of the BoundingSphere object on which it is invoked.

Does anyone else think this behavior is definitely wrong, or is there a hidden reason behind this choice?

https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java#L417

https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-core/src/main/java/com/jme3/bounding/BoundingSphere.java#L470

---

2. Even in the whichSide() method, the < and > comparisons could be standardized:

// from BoundingBox

    @Override
    public Plane.Side whichSide(Plane plane) {
        float radius = FastMath.abs(xExtent * plane.getNormal().getX())
                + FastMath.abs(yExtent * plane.getNormal().getY())
                + FastMath.abs(zExtent * plane.getNormal().getZ());

        float distance = plane.pseudoDistance(center);

        //changed to < and > to prevent floating point precision problems
        if (distance < -radius) {
            return Plane.Side.Negative;
        } else if (distance > radius) {
            return Plane.Side.Positive;
        } else {
            return Plane.Side.None;
        }
    }

// from BoundingSphere

    @Override
    public Plane.Side whichSide(Plane plane) {
        float distance = plane.pseudoDistance(center);

        if (distance <= -radius) {
            return Plane.Side.Negative;
        } else if (distance >= radius) {
            return Plane.Side.Positive;
        } else {
            return Plane.Side.None;
        }
    }

[codex128]

I agree, both behaviors are inconsistent.

It's a no-win situation for the first problem. If we fix it, users could possibly have been relying on merge in BoundingBox to modify the current instance. If we leave it as is, it could result in some very confusing bugs later on. It helps that merge isn't used anywhere in the engine, but still, the options aren't great.

One option is to deprecate merge and create a new method that is implemented consistently. It would at least be very obvious that merge is dangerous to use.

The second problem is minor enough that I vote for documenting it and leaving it alone.

[riccardobl]

+1 for deprecating the current methods on both BoundingBox and BoundingSphere

I just have no idea on how to call the new methods. Maybe mergeWith ?

[codex128]

mergeWith sounds fine, or maybe merge with an extra parameter to store the merge result?

[riccardobl]

mergeWith sounds fine, or maybe merge with an extra parameter to store the merge result?

I don't know if this is consistent with the rest of the engine. I'd expect a null argument to make a copy, but i think in the engine usually we have this behavior in local methods