Fix: Animated models should have proper model bound (issue #343)

Copilot · riccardobl · Copilot · commit d20beb7c50d9 · 2026-03-28T10:10:31.000Z
Agent-Logs-Url: https://github.com/jMonkeyEngine/jmonkeyengine/sessions/47f6b3d5-00bd-4253-a800-382c8b76d5ad Co-authored-by: riccardobl <4943530+riccardobl@users.noreply.github.com>
diff --git a/jme3-core/src/main/java/com/jme3/anim/SkinningControl.java b/jme3-core/src/main/java/com/jme3/anim/SkinningControl.java
@@ -31,6 +31,8 @@
  */
 package com.jme3.anim;
 
+import com.jme3.bounding.BoundingBox;
+import com.jme3.bounding.BoundingVolume;
 import com.jme3.export.InputCapsule;
 import com.jme3.export.JmeExporter;
 import com.jme3.export.JmeImporter;
@@ -297,6 +299,8 @@ private void controlRenderSoftware() {
             // NOTE: This assumes code higher up has already ensured this mesh is animated.
             // Otherwise, a crash will happen in skin update.
             applySoftwareSkinning(mesh, boneOffsetMatrices);
+            // Update the mesh bounding volume to reflect the animated vertex positions.
+            geometry.updateModelBound();
         }
     }
 
@@ -306,6 +310,16 @@ private void controlRenderSoftware() {
     private void controlRenderHardware() {
         boneOffsetMatrices = armature.computeSkinningMatrices();
         jointMatricesParam.setValue(boneOffsetMatrices);
+
+        // Hardware skinning transforms vertices on the GPU, so the CPU-side vertex
+        // buffer is not updated. Compute the animated bounding volume from the bind
+        // pose positions and the current skinning matrices so culling is correct.
+        for (Geometry geometry : targets) {
+            Mesh mesh = geometry.getMesh();
+            if (mesh != null && mesh.isAnimated()) {
+                updateSkinnedMeshBound(geometry, mesh, boneOffsetMatrices);
+            }
+        }
     }
 
     @Override
@@ -751,6 +765,108 @@ private void applySkinningTangents(Mesh mesh, Matrix4f[] offsetMatrices, VertexB
         tb.updateData(ftb);
     }
 
+    /**
+     * Computes the bounding volume of an animated mesh from the bind pose
+     * positions and the current skinning matrices, then sets it on the geometry.
+     * This is used during hardware skinning to keep culling correct, since the
+     * GPU-transformed vertex positions are not reflected in the CPU-side vertex
+     * buffer.
+     *
+     * @param geometry       the geometry whose bound needs to be updated
+     * @param mesh           the animated mesh
+     * @param offsetMatrices the bone offset matrices for this frame
+     */
+    private static void updateSkinnedMeshBound(Geometry geometry, Mesh mesh,
+            Matrix4f[] offsetMatrices) {
+        VertexBuffer bindPosVB = mesh.getBuffer(Type.BindPosePosition);
+        if (bindPosVB == null) {
+            return;
+        }
+        VertexBuffer boneIndexVB = mesh.getBuffer(Type.BoneIndex);
+        VertexBuffer boneWeightVB = mesh.getBuffer(Type.BoneWeight);
+        if (boneIndexVB == null || boneWeightVB == null) {
+            return;
+        }
+        int maxWeightsPerVert = mesh.getMaxNumWeights();
+        if (maxWeightsPerVert <= 0) {
+            return;
+        }
+        int fourMinusMaxWeights = 4 - maxWeightsPerVert;
+
+        FloatBuffer bindPos = (FloatBuffer) bindPosVB.getData();
+        bindPos.rewind();
+        IndexBuffer boneIndex = IndexBuffer.wrapIndexBuffer(boneIndexVB.getData());
+        FloatBuffer boneWeightBuf = (FloatBuffer) boneWeightVB.getData();
+        boneWeightBuf.rewind();
+        // Use array() when available (heap buffer), otherwise copy to a local array.
+        float[] weights;
+        if (boneWeightBuf.hasArray()) {
+            weights = boneWeightBuf.array();
+        } else {
+            weights = new float[boneWeightBuf.limit()];
+            boneWeightBuf.get(weights);
+        }
+        int idxWeights = 0;
+
+        int numVerts = bindPos.limit() / 3;
+        float minX = Float.POSITIVE_INFINITY, minY = Float.POSITIVE_INFINITY,
+                minZ = Float.POSITIVE_INFINITY;
+        float maxX = Float.NEGATIVE_INFINITY, maxY = Float.NEGATIVE_INFINITY,
+                maxZ = Float.NEGATIVE_INFINITY;
+
+        for (int v = 0; v < numVerts; v++) {
+            float vtx = bindPos.get();
+            float vty = bindPos.get();
+            float vtz = bindPos.get();
+
+            float rx, ry, rz;
+            if (weights[idxWeights] == 0) {
+                idxWeights += 4;
+                rx = vtx;
+                ry = vty;
+                rz = vtz;
+            } else {
+                rx = 0;
+                ry = 0;
+                rz = 0;
+                for (int w = 0; w < maxWeightsPerVert; w++) {
+                    float weight = weights[idxWeights];
+                    Matrix4f mat = offsetMatrices[boneIndex.get(idxWeights++)];
+                    rx += (mat.m00 * vtx + mat.m01 * vty + mat.m02 * vtz + mat.m03) * weight;
+                    ry += (mat.m10 * vtx + mat.m11 * vty + mat.m12 * vtz + mat.m13) * weight;
+                    rz += (mat.m20 * vtx + mat.m21 * vty + mat.m22 * vtz + mat.m23) * weight;
+                }
+                idxWeights += fourMinusMaxWeights;
+            }
+
+            if (rx < minX) minX = rx;
+            if (rx > maxX) maxX = rx;
+            if (ry < minY) minY = ry;
+            if (ry > maxY) maxY = ry;
+            if (rz < minZ) minZ = rz;
+            if (rz > maxZ) maxZ = rz;
+        }
+
+        // Reuse the existing BoundingBox if possible to avoid allocation.
+        BoundingVolume bv = mesh.getBound();
+        BoundingBox bbox;
+        if (bv instanceof BoundingBox) {
+            bbox = (BoundingBox) bv;
+        } else {
+            bbox = new BoundingBox();
+        }
+        TempVars vars = TempVars.get();
+        try {
+            vars.vect1.set(minX, minY, minZ);
+            vars.vect2.set(maxX, maxY, maxZ);
+            bbox.setMinMax(vars.vect1, vars.vect2);
+        } finally {
+            vars.release();
+        }
+        // setModelBound() updates the mesh bound and triggers a world-bound refresh.
+        geometry.setModelBound(bbox);
+    }
+
     /**
      * Serialize this Control to the specified exporter, for example when saving
      * to a J3O file.
diff --git a/jme3-core/src/main/java/com/jme3/animation/SkeletonControl.java b/jme3-core/src/main/java/com/jme3/animation/SkeletonControl.java
@@ -32,6 +32,8 @@
 package com.jme3.animation;
 
 import com.jme3.anim.SkinningControl;
+import com.jme3.bounding.BoundingBox;
+import com.jme3.bounding.BoundingVolume;
 import com.jme3.export.*;
 import com.jme3.material.MatParamOverride;
 import com.jme3.math.FastMath;
@@ -259,12 +261,24 @@ private void controlRenderSoftware() {
             // already ensured this mesh is animated.
             // Otherwise a crash will happen in skin update.
             softwareSkinUpdate(mesh, offsetMatrices);
+            // Update the mesh bounding volume to reflect the animated vertex positions.
+            geometry.updateModelBound();
         }
     }
 
     private void controlRenderHardware() {
         offsetMatrices = skeleton.computeSkinningMatrices();
         boneMatricesParam.setValue(offsetMatrices);
+
+        // Hardware skinning transforms vertices on the GPU, so the CPU-side vertex
+        // buffer is not updated. Compute the animated bounding volume from the bind
+        // pose positions and the current skinning matrices so culling is correct.
+        for (Geometry geometry : targets) {
+            Mesh mesh = geometry.getMesh();
+            if (mesh != null && mesh.isAnimated()) {
+                updateSkinnedMeshBound(geometry, mesh, offsetMatrices);
+            }
+        }
     }
 
     @Override
@@ -703,6 +717,108 @@ private void applySkinningTangents(Mesh mesh, Matrix4f[] offsetMatrices, VertexB
         tb.updateData(ftb);
     }
 
+    /**
+     * Computes the bounding volume of an animated mesh from the bind pose
+     * positions and the current skinning matrices, then sets it on the geometry.
+     * This is used during hardware skinning to keep culling correct, since the
+     * GPU-transformed vertex positions are not reflected in the CPU-side vertex
+     * buffer.
+     *
+     * @param geometry       the geometry whose bound needs to be updated
+     * @param mesh           the animated mesh
+     * @param offsetMatrices the bone offset matrices for this frame
+     */
+    private static void updateSkinnedMeshBound(Geometry geometry, Mesh mesh,
+            Matrix4f[] offsetMatrices) {
+        VertexBuffer bindPosVB = mesh.getBuffer(Type.BindPosePosition);
+        if (bindPosVB == null) {
+            return;
+        }
+        VertexBuffer boneIndexVB = mesh.getBuffer(Type.BoneIndex);
+        VertexBuffer boneWeightVB = mesh.getBuffer(Type.BoneWeight);
+        if (boneIndexVB == null || boneWeightVB == null) {
+            return;
+        }
+        int maxWeightsPerVert = mesh.getMaxNumWeights();
+        if (maxWeightsPerVert <= 0) {
+            return;
+        }
+        int fourMinusMaxWeights = 4 - maxWeightsPerVert;
+
+        FloatBuffer bindPos = (FloatBuffer) bindPosVB.getData();
+        bindPos.rewind();
+        IndexBuffer boneIndex = IndexBuffer.wrapIndexBuffer(boneIndexVB.getData());
+        FloatBuffer boneWeightBuf = (FloatBuffer) boneWeightVB.getData();
+        boneWeightBuf.rewind();
+        // Use array() when available (heap buffer), otherwise copy to a local array.
+        float[] weights;
+        if (boneWeightBuf.hasArray()) {
+            weights = boneWeightBuf.array();
+        } else {
+            weights = new float[boneWeightBuf.limit()];
+            boneWeightBuf.get(weights);
+        }
+        int idxWeights = 0;
+
+        int numVerts = bindPos.limit() / 3;
+        float minX = Float.POSITIVE_INFINITY, minY = Float.POSITIVE_INFINITY,
+                minZ = Float.POSITIVE_INFINITY;
+        float maxX = Float.NEGATIVE_INFINITY, maxY = Float.NEGATIVE_INFINITY,
+                maxZ = Float.NEGATIVE_INFINITY;
+
+        for (int v = 0; v < numVerts; v++) {
+            float vtx = bindPos.get();
+            float vty = bindPos.get();
+            float vtz = bindPos.get();
+
+            float rx, ry, rz;
+            if (weights[idxWeights] == 0) {
+                idxWeights += 4;
+                rx = vtx;
+                ry = vty;
+                rz = vtz;
+            } else {
+                rx = 0;
+                ry = 0;
+                rz = 0;
+                for (int w = 0; w < maxWeightsPerVert; w++) {
+                    float weight = weights[idxWeights];
+                    Matrix4f mat = offsetMatrices[boneIndex.get(idxWeights++)];
+                    rx += (mat.m00 * vtx + mat.m01 * vty + mat.m02 * vtz + mat.m03) * weight;
+                    ry += (mat.m10 * vtx + mat.m11 * vty + mat.m12 * vtz + mat.m13) * weight;
+                    rz += (mat.m20 * vtx + mat.m21 * vty + mat.m22 * vtz + mat.m23) * weight;
+                }
+                idxWeights += fourMinusMaxWeights;
+            }
+
+            if (rx < minX) minX = rx;
+            if (rx > maxX) maxX = rx;
+            if (ry < minY) minY = ry;
+            if (ry > maxY) maxY = ry;
+            if (rz < minZ) minZ = rz;
+            if (rz > maxZ) maxZ = rz;
+        }
+
+        // Reuse the existing BoundingBox if possible to avoid allocation.
+        BoundingVolume bv = mesh.getBound();
+        BoundingBox bbox;
+        if (bv instanceof BoundingBox) {
+            bbox = (BoundingBox) bv;
+        } else {
+            bbox = new BoundingBox();
+        }
+        TempVars vars = TempVars.get();
+        try {
+            vars.vect1.set(minX, minY, minZ);
+            vars.vect2.set(maxX, maxY, maxZ);
+            bbox.setMinMax(vars.vect1, vars.vect2);
+        } finally {
+            vars.release();
+        }
+        // setModelBound() updates the mesh bound and triggers a world-bound refresh.
+        geometry.setModelBound(bbox);
+    }
+
     @Override
     public void write(JmeExporter ex) throws IOException {
         super.write(ex);
diff --git a/jme3-core/src/test/java/com/jme3/test/PreventCoreIssueRegressions.java b/jme3-core/src/test/java/com/jme3/test/PreventCoreIssueRegressions.java
@@ -40,6 +40,7 @@
 import com.jme3.app.state.ScreenshotAppState;
 import com.jme3.asset.AssetManager;
 import com.jme3.asset.DesktopAssetManager;
+import com.jme3.bounding.BoundingVolume;
 import com.jme3.input.InputManager;
 import com.jme3.input.dummy.DummyKeyInput;
 import com.jme3.input.dummy.DummyMouseInput;
@@ -130,4 +131,47 @@ public void testIssue1138() {
                     Vector3f.isValidVector(joint.getLocalTranslation()));
         }
     }
+
+    /**
+     * Test case for JME issue #343: Animated models should have proper model bound.
+     *
+     * <p>When software skinning is used, calling controlRender should update the
+     * bounding volumes of the animated geometries to reflect their current pose.
+     */
+    @Test
+    public void testIssue343() {
+        AssetManager am = JmeSystem.newAssetManager(
+                PreventCoreIssueRegressions.class.getResource("/com/jme3/asset/Desktop.cfg"));
+        Node cgModel = (Node) am.loadModel("Models/Elephant/Elephant.mesh.xml");
+        cgModel.scale(0.04f);
+
+        AnimComposer composer = cgModel.getControl(AnimComposer.class);
+        SkinningControl sControl = cgModel.getControl(SkinningControl.class);
+
+        // Force software skinning so bounds are computed from CPU vertex positions.
+        sControl.setHardwareSkinningPreferred(false);
+
+        // Record the world bound in the bind pose.
+        cgModel.updateGeometricState();
+        BoundingVolume bindPoseBound = cgModel.getWorldBound().clone();
+
+        // Advance the "legUp" animation, which raises a leg well beyond the bind pose.
+        composer.setCurrentAction("legUp");
+        cgModel.updateLogicalState(0.5f);
+
+        // Simulate the render pass: controlRender applies software skinning and
+        // calls geometry.updateModelBound() on each target.
+        RenderManager rm = new RenderManager(new NullRenderer());
+        ViewPort vp = rm.createMainView("test", new Camera(1, 1));
+        sControl.render(rm, vp);
+
+        // Propagate the refreshed bounds up the scene graph.
+        cgModel.updateGeometricState();
+        BoundingVolume animatedBound = cgModel.getWorldBound().clone();
+
+        // The bounding volume must differ from the bind pose bound.
+        Assert.assertFalse(
+                "Model bound should change after animation is applied via software skinning",
+                bindPoseBound.equals(animatedBound));
+    }
 }
