Hi, I’ve found this topic being discussed in the forum multiple times (in 2021, 2018 and even back in 2013). But there doesn’t seem to be a definitive answer.
I made an example to trigger the exception and understand the problem better.
This is the sample code:
The problem happens when we add geometries of different vertex counts and different materials.
If a simpler geometry is added after first batching with more complex ones it breaks.
Now a break down of the problem in the BatchNode class:
When calling batch() the private method mergeGeometries() sets the variable maxVertCount with the highest vertex count of the not-yet batched geometries:
if (maxVertCount < geom.getVertexCount()) {
maxVertCount = geom.getVertexCount();
}
then the batching process continues and this information is used to allocate some temp arrays:
tmpFloat = new float[maxVertCount * 3];
tmpFloatN = new float[maxVertCount * 3];
if (useTangents) {
tmpFloatT = new float[maxVertCount * 4];
}
This is reset each time batch() is called, so after the last batch call of the example, the array will be smaller than the second call (because the geo has fewer vertices).
But later on when updateGeometricState() is called by the main loop, the updateSubBatch() method gets called for each of the children geometries.
This then calls the doTransforms() like this:
doTransforms(oposBuf, onormBuf, otanBuf, posBuf, normBuf, tanBuf, bg.startIndex, bg.startIndex + bg.getVertexCount(), transformMat);
As far as I see bg is the original non-batched geometry and look at the argument bg.startIndex + bg.getVertexCount(), it’s the original vertex count.
Then doTransforms() does this:
int length = (end - start) * 3;
// ...
bindBufPos.get(tmpFloat, 0, length);
and BAM! tmpFloat only has space for the last geometry added which has less vertices than the previously added one, so we get an IndexOutOfBoundsException.
This wouldn’t happen if we didn’t call batch twice before updateGeometricState, but there are other scenarios where this can happen I think.
I made a modified version of BatchNode and at least solves this problem here.
I simply check the size before accessing the temp array and reallocate it accordingly:
private void doTransforms(...) {
// call this before using the array
validateTempFloatArrays(end - start);
}
private void validateTempFloatArrays(int vertCount) {
if (maxVertCount < vertCount) {
maxVertCount = vertCount;
initTempFloatArrays();
}
}
// This is the same code extracted from doBatch()
private void initTempFloatArrays() {
tmpFloat = new float[maxVertCount * 3];
tmpFloatN = new float[maxVertCount * 3];
if (useTangents) {
tmpFloatT = new float[maxVertCount * 4];
}
}
Do you think my analysis is accurate? is a good solution?
I can open a PR if that’s the case.
Sorry for the long post I tried to explain the problem in detail.
