Different fix for IndexOutOfBounds exception in BatchNode

Troubleshooting graphics
1

I wanted to bring a little more attention to this and wasn’t exactly sure where to stick it.

Back story: during my live stream, I discovered that my (3.6.0-based-local-hacked) jMonkeyEngine’s BatchNode was throwing IndexOutOfBounds exceptions. In trying to track down whether this was a “me problem” or a “jme problem”, I found the issue and implemented a fix locally.

I then looked in the JME repo to see if this had already been fixed… and it has “sort of”. See my more detailed comment here: BatchNode: Fix IndexOutOfBoundsException by jcfandino · Pull Request #2297 · jMonkeyEngine/jmonkeyengine · GitHub

That PR is really only fixing the symptom, though. (might be worth keeping that ‘last ditch effort’ style fix but it’s not really fixing the underlying problem)

I have to be pretty ruthless with where I spend my time these days so I don’t have time to make a proper PR, etc… I’m currently not even setup to push to JME on github anymore.

For those that don’t like to click through, quick synopses of the problem:

  1. have a BatchNode
  2. add some Spheres with a red material
  3. add some boxes with a blue material
  4. batch()
  5. add some arrows with a green material
  6. batch()
  7. move one of the boxes

The issue is that the maxVertCount is only calculated from what is batched in doBatch() and not what is actually being managed by the BatchNode. doBatch() will only batch geometries that aren’t already batched. The only thing that uses maxVertCount is the temporary buffer allocation and those buffers are only used by the code that handles object transforms.

The current solution in github just checks the size of these temp arrays at the point of use and reallocates them if they are too small. Which works… at the expense of perhaps several extra array allocations.

Current github solution array allocations:

  1. have a BatchNode
  2. add some Spheres with a red material
  3. batch() → reallocate the arrays nice and big
  4. add some boxes with a blue material
  5. batch() → reallocate the arrays too small for spheres
  6. add some arrows with a green material
  7. batch() → reallocate the arrays too small for boxes or spheres
  8. move one of the boxes → reallocate the arrays (where the old code threw an exception) to be big enough for boxes but not for spheres
  9. move one of the spheres → reallocate the arrays to be big enough for spheres

The patch I’ve attached to that PR instead calculates the maxVertCount when iterating over all of the geometry to gather them during doBatch(). (Which is the right place to do it.) And then only reallocates the buffers if they are the wrong size. This better matches the intent of maxVertCount and so I will refer to this as the “correct solution” (with casting aspersions to the current fix)

Correct fix:

  1. have a BatchNode
  2. add some Spheres with a red material
  3. batch() → reallocate the arrays
  4. add some boxes with a blue material
  5. batch() → arrays are already big enough
  6. add some arrows with a green material
  7. batch() → arrays are already big enough
  8. move one of the boxes
  9. move one of the spheres

Sorry I don’t have time to do more than bring attention to it.

6 Likes
2

Nice, I removed the BatchNode: Fix IndexOutOfBoundsException by jcfandino · Pull Request #2297 · jMonkeyEngine/jmonkeyengine · GitHub locally and quickly translated Paul’s pseudo code to legit Java and the original problem is easily reproduced. Should anyone want to take a stab at it. I mean, I’m also trying perhaps but I can’t give any guarantees, just to jump start people:

package jme3test.batching;

import com.jme3.app.SimpleApplication;
import com.jme3.material.Material;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Vector3f;
import com.jme3.post.FilterPostProcessor;
import com.jme3.post.filters.BloomFilter;
import com.jme3.scene.BatchNode;
import com.jme3.scene.Geometry;
import com.jme3.scene.Mesh;
import com.jme3.scene.debug.Arrow;
import com.jme3.scene.shape.Box;
import com.jme3.scene.shape.Sphere;
import com.jme3.system.AppSettings;
import java.util.function.Supplier;

/**
 *
 * @author Toni
 */
public class TestBatchNodeIOoB extends SimpleApplication {

    private static AppSettings settingst;
    private BatchNode batchNode;
    private Material mat1;
    private Material mat2;
    private Material mat3;

    public static void main(String[] args) {
        TestBatchNodeIOoB app = new TestBatchNodeIOoB();
        settingst = new AppSettings(true);
        //settingst.setFrameRate(75);
        settingst.setResolution(640, 480);
        settingst.setVSync(false);
        settingst.setFullscreen(false);
        app.setSettings(settingst);
        app.setShowSettings(false);
        app.start();
    }

    @Override
    public void simpleInitApp() {
        batchNode = new BatchNode("BatchNode");
        rootNode.attachChild(batchNode);

        mat1 = new Material(assetManager, "Common/MatDefs/Misc/Unshaded.j3md");
        mat1.setColor("Color", ColorRGBA.White);
        mat1.setColor("GlowColor", ColorRGBA.Blue.mult(10));

        mat2 = new Material(assetManager, "Common/MatDefs/Misc/Unshaded.j3md");
        mat2.setColor("Color", ColorRGBA.White);
        mat2.setColor("GlowColor", ColorRGBA.Red.mult(10));

        mat3 = new Material(assetManager, "Common/MatDefs/Misc/Unshaded.j3md");
        mat3.setColor("Color", ColorRGBA.White);
        mat3.setColor("GlowColor", ColorRGBA.Yellow.mult(10));

        addMeshes(() -> new Sphere(16, 16, 1), mat1);
        Geometry box = addMeshes(() -> new Box(16, 16, 1), mat2);
        batchNode.batch();

        addMeshes(() -> new Arrow(new Vector3f(1, 1, 1)), mat3);
        batchNode.batch();

        box.setLocalTranslation(1, 2, 3);

        FilterPostProcessor fpp = new FilterPostProcessor(assetManager);
        fpp.addFilter(new BloomFilter(BloomFilter.GlowMode.Objects));
        viewPort.addProcessor(fpp);
    }

    private Geometry addMeshes(Supplier<Mesh> meshGen, Material mat) {
        Geometry geom = null;
        for (int i = 0; i < 10; i++) {
            geom = new Geometry("fepr", meshGen.get());
            geom.setMaterial(mat);
            batchNode.attachChild(geom);
        }

        return geom;
    }

}
3 Likes
3

Yay, Improved fix for IndexOutOfBoundsException by tonihele · Pull Request #2403 · jMonkeyEngine/jmonkeyengine · GitHub

7 Likes
4

Thank you so much for running with this.

3 Likes
5

Glad you found a better way of fixing this!
I don’t remember the exact details of this code, just I wasn’t super confident of my solution, but also didn’t want to risk messing up a core class.

Here is my original post with a breakdown and an example for more context if anyone wants to read up more: BatchNode and IndexOutOfBoundsException

2 Likes
6

In my day job, I feel like I spend 50% of my time getting to the actual root of a bug because I won’t get to fix it myself… but will have to explain it to someone else in a way that gives them agency to make decisions on their own. (ie: if I give someone line-by-line instructions, I can be reasonably sure that at the first sign of trouble then they will abandon that and start over. :slight_smile: )

Bottom line is that I spend a reasonable amount of my time trying to boil down a problem into one sentence explanations and so have a lot of practice with this sort of decomposition.

I also get really curious about “why” something happens… because it often leads to a better “one sentence explanation”.

For the curious, the one sentence bug explanation here is: “BatchNode needs maxVertCount to be the maximum per-geometry vertex count for the whole batch node but only calculates it based on what was recently batched.” (Which is really close to some of the sentences in the linked post.)

The one sentence solution is: “BatchNode already iterates over all of the geometry when deciding what should be batched, so that is where it should be calculating the maxVertCount”.

In any case, this was some nice team work.

2 Likes