Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstancedBufferGeometry._maxInstanceCount preventing instances from being rendered #26363

Closed
amandaghassaei opened this issue Jul 3, 2023 · 3 comments · Fixed by #26365
Closed

Comments

@amandaghassaei
Copy link

Description

I ran into this issue when I tried to increase the count on a large InstancedInterleavedBuffer. During the initial render geometry._maxInstanceCount was set to InstancedInterleavedBuffer.count, and increasing InstancedInterleavedBuffer.count and geometry.instanceCount later had no effect, causing additional instances to not render.

Some options:

  1. Would it be possible to remove _maxInstanceCount from the library, as it seems to be a bit of a patch anyway? An alternative could be to calculate an appropriate instanceCount only in the case that geometry.instanceCount is not defined by looping through all InstancedBufferAttributes or InstancedInterleavedBuffers on the InstancedBufferGeometry:

} else if ( geometry.isInstancedBufferGeometry ) {
const maxInstanceCount = geometry._maxInstanceCount !== undefined ? geometry._maxInstanceCount : Infinity;
const instanceCount = Math.min( geometry.instanceCount, maxInstanceCount );
renderer.renderInstances( drawStart, drawCount, instanceCount );
} else {

  1. If we must keep _maxInstanceCount, could we change the following to support the full size of an InstancedInterleavedBuffer rather than its currently set count:

if ( object.isInstancedMesh !== true && geometry._maxInstanceCount === undefined ) {
geometry._maxInstanceCount = data.meshPerAttribute * data.count;
}

to

geometry._maxInstanceCount = data.array ? data.meshPerAttribute * data.array.length / data.stride : 0;

Option 2 still has the downside of persisting an incorrect _maxInstanceCount if the InstancedInterleavedBuffer and its associated attributes were to be swapped out with a larger buffer at a later time. I use this pattern in cases where I don't know exactly how big to init the buffers for user-generated geometry, and (while I do give them plenty of headroom) I don't want to init the buffers too big upfront until they are needed. This issue of replacing the buffers came up previously in Issue #21488

Maybe an alternative on option 2 would be to update geometry._maxInstanceCount each time a new InstancedBufferAttribute or attribute referencing an InstanceInterleavedBuffer is attached to the geometry?

Anyway, in the current code _maxInstanceCount seems to only be calculated based on attached InstancedInterleavedBuffers, when it should also depend on attached InstancedBufferAttributes.

Thanks!

Reproduction steps

  1. Init a mesh with an InstancedBufferGeometry with instanceCount = 0 and attributes referencing an InstancedInterleavedBuffer containing a large array, but count = 0.
  2. Render the mesh at least once.
  3. Increase the count of the InstancedInterleavedBuffer and increase the instanceCount of the InstancedBufferGeometry to match.
  4. Render the mesh again - no instances rendered.

Code

const MAX_NUM_SEGMENTS = 1000;

// Init geometry with zero instances to start.
const geometry = new THREE.InstancedBufferGeometry();
geometry.instanceCount = 0;

// Init interleaved buffer with zero count to start.
const positionsArray = new Float32Array(6 * MAX_NUM_SEGMENTS);
const interleavedBuffer = new THREE.InstancedInterleavedBuffer(positionsArray, 6, 1);
interleavedBuffer.count = 0;

// Init attributes referencing InterleavedBufferAttribute.
geometry.setAttribute('instanceStart', new THREE.InterleavedBufferAttribute(interleavedBuffer, 3, 0));
geometry.setAttribute('instanceEnd', new THREE.InterleavedBufferAttribute(interleavedBuffer, 3, 3));

......

const mesh = new THREE.Mesh( geometry, material );
scene.add( mesh );

// Animation loop.
function render() {
  requestAnimationFrame( render );
  renderer.render( scene, camera );
}
// During the first render(), geometry._maxInstanceCount is set to interleavedBuffer.count, which is zero.
render();

.....

// Add new geometry on each pointerdown.
window.addEventListener('pointerdown', e => {
  const count = interleavedBuffer.count;
  // Set random position.
  positionsArray[6 * count] = (Math.random() - 0.5) * window.innerWidth;
  positionsArray[6 * count + 1] = (Math.random() - 0.5) * window.innerHeight;
  positionsArray[6 * count + 3] = (Math.random() - 0.5) * window.innerWidth;
  positionsArray[6 * count + 4] = (Math.random() - 0.5) * window.innerHeight;
  interleavedBuffer.needsUpdate = true;
  // Update counts.
  interleavedBuffer.count = count + 1;
  geometry.instanceCount = count + 1;
});

The following line fixes the issue:

// Manually force _maxInstanceCount to support the full size of the buffer.
geometry._maxInstanceCount = interleavedBuffer.array ? interleavedBuffer.meshPerAttribute * interleavedBuffer.array.length / interleavedBuffer.stride : 0;

Live example

This example should add new geometry on each pointerdown on the canvas.
Uncommenting line 24 makes the example work.

Screenshots

No response

Version

r0.154

Device

Desktop

Browser

Chrome

OS

MacOS

@amandaghassaei amandaghassaei changed the title InstancedBufferGeometry._maxInstanceCount preventing instances from being rendered when InstancedInterleavedBuffer.count is increased InstancedBufferGeometry._maxInstanceCount preventing instances from being rendered Jul 3, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 3, 2023

Your use case is not supported since the count property represents the maximum number of instances. Meaning after the first render, you can only have instances in the range [0, instanceCount]. This restriction is also true for InstancedMesh.

The reason behind this restriction is the fact that buffers can't be resized after the first render. We do not want to implement some sort of auto-detection and resizing in the renderer since this comes with a performance hit. Especially the resize itself is costly and should be avoided whenever possible.

If you have to increase the count of your instances over time, you have to dispose the old geometry and create a new one. The more recommended approach though is to create the geometry with a sufficient size so a resize is not necessary over time.

@amandaghassaei
Copy link
Author

ok so what I'm hearing is that InstancedInterleavedBuffer.count and BufferAttribute.count are read-only and should not be set through the api. is that correct?

instead, set InstancedBufferGeometry.instanceCount between renders to control the number of instances drawn.

I think this read-only distinction could be made a bit more clear in the docs, as there are a number of other "counts" that are meant to be set (e.g. drawRange, updateRange, instanceCount, etc).

thanks for the quick reply.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 3, 2023

Yes, BufferAttribute.count is read-only (since it is derived from a fixed-length array and constant item size).

Indeed, it does make sense to clarify the docs and mark this property as read-only.

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 a pull request may close this issue.

2 participants