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

TresInstancedMesh breaks with a dynamic instance count #746

Open
darkvertex opened this issue Jun 27, 2024 · 14 comments
Open

TresInstancedMesh breaks with a dynamic instance count #746

darkvertex opened this issue Jun 27, 2024 · 14 comments
Labels
help wanted Extra attention is needed p3-minor-bug An edge case that only affects very specific usage (priority) PR welcome

Comments

@darkvertex
Copy link

Describe the bug

I took a stackblitz from @alvarosabu demonstrating TresJS with an InstancedMesh with a bunch of ondulating Suzanne monkey heads, and then I edited the code so it starts with a ref() count of 3, and on each @click of the InstancedMesh, it is SUPPOSED to increment the count by 1 but then breaks.

I am developing something with a realtime floor map showing people markers, and the number of people changes dynamically as they enter the room, potentially every few seconds. Everything worked great with static count values but breaks when it changes dynamically. :/

It errors like this:
image
(and no, I'm not setting .position on the InstancedMesh, so not sure what that's about.)

Reproduction

https://stackblitz.com/edit/tresjs-instanced-mesh-fklgmc?file=src%2Fcomponents%2FTheExperience.vue

Steps to reproduce

  1. npm install pnpm
  2. pnpm install
  3. pnpm run dev
  4. Wait for the animated Suzanne heads to appear. (It will be 3 at first.)
  5. Click on one of the monkey heads while Chrome Devtools console is open. (It should increment the instance count by +1, but breaks.)

System Info

Ran from Stackblitz:


  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 9.4.0 - ~/projects/tresjs-instanced-mesh-fklgmc/node_modules/.bin/pnpm
  npmPackages:
    @tresjs/cientos: 3.9.0 => 3.9.0 
    @tresjs/core: ^4.0.2 => 4.0.2 
    vite: ^4.1.4 => 4.5.3 


### Used Package Manager

pnpm

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/Tresjs/tres/blob/main/CODE_OF_CONDUCT.md)
- [X] Read the [Contributing Guidelines](https://github.com/Tresjs/tres/blob/main/CONTRIBUTING.md).
- [X] Read the [docs](https://tresjs.org/guide).
- [X] Check that there isn't [already an issue](https://github.com/tresjs/tres/issues) that reports the same bug to avoid creating a duplicate.
- [X] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.
@darkvertex
Copy link
Author

Also to be clear, I don't believe this is an issue with threejs as there is an official example showing dynamically changing instance counts on the same InstancedMesh working no problem:

https://threejs.org/examples/?q=inst#webgl_instancing_dynamic

@alvarosabu alvarosabu added pending-triage Ticket is pending to be prioritised investigation labels Jul 3, 2024
@alvarosabu alvarosabu self-assigned this Jul 3, 2024
@Neosoulink
Copy link

Hey @alvarosabu, is someone working on this?

I can Investigate and work on this

@alvarosabu
Copy link
Member

Hey buddy @Neosoulink, not yet, but it would be great if you could help us with it.

@alvarosabu alvarosabu added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending-triage Ticket is pending to be prioritised labels Jul 11, 2024
@Neosoulink
Copy link

Neosoulink commented Jul 11, 2024

@darkvertex it's important you understand that the count of instancedMesh can not be resized to a higher level

As mugen87 mentionned it here: https://discourse.threejs.org/t/modified-three-instancedmesh-dynamically-instancecount/18124/10

Keep in mind that InstancedMesh.count represents the maximum count of instances. You need to init the mesh with this number but can then alter the property to a lower value.
Buffers can not be resized!

From the Doc InstancedMesh.count

You can change the number of instances at runtime to an integer value in the range [0, count].
If you need more instances than the original count value, you have to create a new InstancedMesh.


With that said, why are you getting the error:

"Cannot assign to read only property position of object #<InstancedMesh>"

I'm unsure, but the error is triggered after the count is modified. count being an argument of an instance, it can not be modified again.


Now let's see alternatives,
Understand that even if the count was working, you wouldn't be able to increase the count like that.

Here I've created an alternative solution to increase InstancedMesh in Tres

const material = new MeshNormalMaterial();
const count = shallowRef(1);
const instancedMesh = shallowRef();

const createPrimitiveInstance = () =>
  h('primitive', {
    ref: instancedMesh,
    object: new InstancedMesh(susane, material, count.value ?? 1),
  });

const InstancedComponent = shallowRef(() => createPrimitiveInstance());

const resetInstancedComp = () => {
  instancedMesh.value?.dispose();
  InstancedComponent.value = () => createPrimitiveInstance();
};

onLoop(({ elapsed }) => {
 // codes ...
});

onMounted(() => {
  setInterval(() => {
    if (count.value < 100) {
      count.value++;
      resetInstancedComp();
      console.log('instancedMesh updated', count.value);
    }
  }, 3000);
});

//////////

<template>
  <TresCanvas v-bind="stateCanvas">
    <InstancedComponent />
  </TresCanvas>
</template>

The above code will add each 3 sec a new instance to the scene.
See the complete example here: https://stackblitz.com/edit/tresjs-instanced-mesh-pwbt4l?file=src%2Fcomponents%2FTheExperience.vue


@alvarosabu, I don't think it's related to Tres didn't find anything

Error coming from patchProp property of nodeOps precisely in the root Object.assign.

@darkvertex
Copy link
Author

Thanks for taking a look, @Neosoulink!

You are indeed right that "count" cannot go higher than the value fed to the constructor. I missed that in the docs, my bad. -- It turns out that the "count" property is editable however, so as long it is a lower or equal value than what was given to the constructor.

With this corrected knowledge, I forked my bug repro stackblitz and got it working given the "count" prop never exceeds the value of the constructor. I give it a higher number (10) at consturction and track the desired count separate from the max (initial) count, and ensure my desired count does not exceed the initial.

This works reasonably well now:
https://stackblitz.com/edit/tresjs-instanced-mesh-vwhmkx?file=src%2Fcomponents%2FTheExperience.vue
(click on the monkey idling in the center, and it will add up to 10 instances.)

Now, what if the stuff fed to ":args=..." (constructor) changes? Would it be a crazy expectation that if constructor (":args=...") change because the given props changed upstream, the old threejs InstancedMesh object ought to be cleanly disposed of and a new one recreated in its place?

If you hold the Shift key while clicking on the monkey in the middle in my example above, it will attempt to set the "maxCount" to a higher number (1000 instead of 10), which was passed to the ":args", so I would have thought TresJS would handle re-syncing, but instead you will see this error below:

image

I can kinda work around my issues by setting a very high initial count, but it would be nice if TresJS could autorecreate the InstancedMesh should the constructor's reactive values change.

Thoughts?

@Neosoulink
Copy link

@darkvertex, nice to know you partially solved the issue!

But this issue is more about ThreeJs itself,

If you want to increase the max-count, you have to recreate a new InstancedMesh

See my Tres examples integration
https://stackblitz.com/edit/tresjs-instanced-mesh-pwbt4l?file=src%2Fcomponents%2FTheExperience.vue

@darkvertex
Copy link
Author

darkvertex commented Jul 12, 2024

@Neosoulink Ah, I get what you did now. Okay.

With your approach of having a swapable "component", do you know how could I get @click events working again on the <InstancedComponent>? -- I incorporated your technique but they do not appear to trigger anymore (and they did when it was on the direct <TresInstancedMesh> component.)

@alvarosabu
Copy link
Member

Hi @darkvertex the event issue with dynamic components should be fixed with #763, hasn't being released yet. Probably today

@darkvertex
Copy link
Author

@alvarosabu Oh, que bueno! ❤️

Will I need to do any code gymnastics to make the raycast click stuff detect the instanceid under the click, given that this is a dynamic component? Or does TresJS figure it out by the threejs object type?

@darkvertex
Copy link
Author

darkvertex commented Jul 15, 2024

I don't have a bug repro stackblitz for you at this time, but I wanted to share some oddities:

I tried to update my real app code to 4.2.0 and it goes into an infinite loop of recreating the InstancedMesh, whereas 4.0.2 works fine but had no working click events in my component.

The trick of using a function to return a primitive seems to be the culprit somehow, as when I change directly to having a <primitive :object="myInstancedMesh" /> in my <template> instead of referencing a dynamic component then I can point it to a mesh almost the same way and the infinite creation loop goes away.

When I test with static data it seems I have click events with it, but when I have the whole thing hooked up, my instances are updating nicely, and on-demand rendering is working nicely too, but I cannot get any click events to fire at all... I'm stumped. 🤔

Is there a special debug verbosity flag or something I can enable to get more visibility on what's happening inside Tres?

@alvarosabu
Copy link
Member

Is there a special debug verbosity flag or something I can enable to get more visibility on what's happening inside Tres?

Hi @darkvertex have you tried checking the scene graph on the vue devtools?

Screenshot 2024-07-15 at 20 09 50

@darkvertex
Copy link
Author

Yes.

When I was having the issue of the instancedmesh being regenerated in perpetuity the devtools for Tres showed the mesh in question flickering in the Scene breakdown, which was probably from being trashed and recreated over and over.

Now with my functional but unclickable setup there's no flickering of the mesh and there does seem to have the click event attached:
image
but the click event is not firing.

@darkvertex
Copy link
Author

Ok, I think I found my issue with click events not firing.

Calling .computeBoundingSphere() on the InstancedMesh after doing all my instances matrix updates (.setMatrixAt(...)) seems to make clicking work again. I didn't know this was necessary.

Curiously, doing .computeBoundingBox() does not help, only .computeBoundingSphere() fixes it. Odd.

@darkvertex
Copy link
Author

When I tested just manually setting two instances at the root of the component without anything dynamic, click events worked without having to recompute any bounding volumes.

It's as if the bounding spheres are computed the very very first render and then that's it.

@alvarosabu alvarosabu added help wanted Extra attention is needed PR welcome and removed investigation labels Aug 21, 2024
@alvarosabu alvarosabu removed their assignment Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed p3-minor-bug An edge case that only affects very specific usage (priority) PR welcome
Projects
Status: Todo
Development

No branches or pull requests

3 participants