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

improve InstancedMesh so that it grows buffers directly instead of replacing THREE.InstancedMesh #273

Open
trusktr opened this issue Aug 10, 2023 · 2 comments

Comments

@trusktr
Copy link
Member

trusktr commented Aug 10, 2023

@keywizzle on asked on Discord:

Not sure if maybe I'm doing something wrong, but I think there's a bug with InstancedMesh where it creates a GL effect, and does some stuff if this.count > this.#biggestCount:

this.createGLEffect(() => {
    // Increase the InstancedMesh size (by making a new one) as needed.
    if (this.count > this.#biggestCount) {
        this.#biggestCount = this.count

        untrack(() => {
            this.recreateThree()

            // Be sure to trigger all the instance components so that the new
            // InstancedMesh will be up-to-date.
            this.rotations = this.rotations
            this.positions = this.positions
            this.scales = this.scales
            this.colors = this.colors
        })  
    }

    untrack(() => (this.three.count = this.count))

    this.needsUpdate()
})

I think the issue lies somewhere in recreateThree (which is a part of SharedApi), in which a custom-defined geometry and material will be overwritten by the default geometry/material behaviors for some reason. I noticed this when the meshes count property exceeded 10 (this is when this.count > this.#biggestCount becomes true, because 10 is the default count), and suddenly the custom geometry/material stopped applying. Basically, any time this.count > this.#biggestCount, any custom-defined geometry or material has to be reset, ie:

instancedMesh.three.geometry = new THREE.RingGeometry(1.75, 2, 32)

will be overwritten by the default geometry when this.count > this.#biggestCount. I did find a stupid way around this, by setting count to something like 999999, then the next animation frame setting it back to 1. This sets biggestCount to 999999, meaning that logic will never run and it will work as expected (as far as my testing goes). This allows me to then add whatever instances I want without issue, while also not actually having the GPU render 999999 instances (both LUME and THREE's InstancedMesh both have identical count equal to however many instances I add, while biggestCount remains 999999).

Maybe I'm applying custom geometry and material improperly? Not sure, was pretty tricky to find out what was going on with this though.

@trusktr
Copy link
Member Author

trusktr commented Aug 10, 2023

When the count is bigger than the previous allotted space, more space needs to be made, and right now the simple way to do that is to re-create the underlying THREE.InstancedMesh. The new instance's geometry or material properties will re-gain the geometry or material from the element's behaviors so that the rendering will match what was specified via the HTML/DOM. This needs to happen because, if we're using the HTML interface, if we grow the count, we'd expect for it to simply work and for whatever behaviors we applied (via the HTML/DOM description) to be exactly as stated.

Here's a solution to use as a consumer of Lume:

const geometry = new THREE.RingGeometry(1.75, 2, 32)

// Any time the count changes, put back the same geometry
createEffect(() => {
  instancedMesh.count
  instancedMesh.three.geometry = geometry
})

Something we can do inside Lume is improve lume-instanced-mesh so that instead of it creating a new THREE.InstancedMesh under the hood when count increases (which also means that the element.three reference is a new instance), we can modify the underlying arrays to increase their size manually


We should also look at is making it as easy as possible to implement new behaviors with custom geometries/materials, so that the underlying THREE resources will be managed in the Lume life cycles.


Here's the thing we'd want to update directly inside of THREE.InstancedMesh:

https://github.com/mrdoob/three.js/blob/master/src/objects/InstancedMesh.js#L25

We'd need to make a new InstacedBufferAttribute instead of replacing the whole InstancedMesh

We can update Lume's InstancedMesh class to do that instead of having it call this.recreateThree() when count grows.

The recreateThree() call causes this to be called:

https://github.com/lume/lume/blob/develop/src/meshes/InstancedMesh.ts#L160-L189

It is reactive, so if there's anything inside of makeThreeObject3d that is a signal, that logic re-runs. This is useful for when we need to map state to a constructor, and there's otherwise no way to update the state because it has to be passed in via constructor, or otherwise we were being lazy and just making the things anew because it was easy to write that way.


One main thing is that, this is all for the ideal purpose of the HTML API and everything about it being always correct. I.e. if someone changes the HTML/DOM, the rendering should always be correct. Accessing .three could lead to issues because then it may break expectations because changes to Three.js object are not necessarily monitored by Lume (and perhaps these properties should even be marked as private with a warning like "use at your own risk because Lume does not monitor underlying Three.js properties for changes, and instead maps state to them in a one-way flow".

We should of course try to improve it however we can on both sides:

  • make it less likely that reaching into internals will have unexpected effects (f.e. we should re-create deeper parts of THREE.InstancedMesh instead of the whole THREE.InstancedMesh)
  • improve the HTML interfaces so they can do more of what we want without having to reach in, and make it easy to add new elements/behaviors to Lume for custom use cases that are connected to Lume's life cycle and reactivity.

As an example, it currently isn't too much work to create a new geometry behavior:

https://github.com/lume/lume/blob/develop/src/behaviors/mesh-behaviors/geometries/BoxGeometryBehavior.ts

But we can think about how to make it easier.

I also want to move away from behaviors for some things like geometries and materials (though they may be useful for other things that are less declarative), and instead make them custom elements that have simpler class/type definitions and that are easier to maintain (later the improved static types will be easier to compile to WebAssembly with ByteScript). The new way can look something like this:

<lume-mesh>
  <!-- these are not scene graph nodes, but they will manage certain sub-object on the parent element instances (f.e. .geometry or .material of the .three instance), similar to behaviors but wired up differently: -->
  <lume-box-geometry wireframe></lume-box-geometry>
  <lume-phong-material color="pink"></lume-phong-material>

  <!-- ... scene graph node as usual ... -->
</lume-mesh>

With this approach the attributes for all elements are known up front with static type definitions. Right now behaviors can add any numbers of attributes and JS properties to their host mesh, which is not very easy to keep type safe or to add new types for. A user can currently make a new behavior for that can be applied onto meshes, but there is no easy way to ensure any new properties are part of the lume-mesh type definition, plus we don't want lume-mesh to become a bag of arbitrary property definitions. So, instead, each of these new types of elements will have their own static type definition and will not modify the parent element's types, and users can easily define property types simply by making them part of their new subclass definition.

We could then also have something like a lume-custom-geometry element that doesn't have a particular geometry pre-defined (and similar for materials), and make it easy to do things like

<lume-mesh>
  <lume-custom-geometry id="geo"></lume-custom-geometry>
  <lume-phong-material color="pink"></lume-phong-material>

  <!-- ... scene graph node as usual ... -->
</lume-mesh>

<script>
  const geo = document.querySelector('#geo')
  geo.wireframe = true
  geo.threeGeometry = new SomeSpecialGeometry(...)
</script>

or something, to easily provide Threejs geometries/materials without making a new wrapper (actually that might be a base class for new wrappers to extend from). This is now getting fairly threejs specific with such an element, so I'm also thinking maybe naming it lume-three-geometry and providing it as a unique feature of the threejs Lume implementation.

Later on we'll have Babylon.js- and PlayCanvas-backed implementations of Lume so people can choose their underlying engine.

I'd like to eventually have our own rendering impl too. Probably starting as our fork of Three.js, but we'll probably really do this later once we have resources for it.


Anyway, that got a little side-tracked, but TLDR: let's make it really easy to add new wrappers for things like custom geometries and materials.

@trusktr
Copy link
Member Author

trusktr commented Aug 10, 2023

The most ideal solution would be making a new GeometryBehavior like SphereGeometryBehavior, including new properties to map to the geometry if needed. Here's how SphereGeometryBehavior is implemented:

https://github.com/lume/lume/blob/develop/src/behaviors/mesh-behaviors/geometries/SphereGeometryBehavior.ts

It is pretty simple, and it maps any properties specific to that geometry as needed. Would be great to add more geometries to Lume.

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

No branches or pull requests

1 participant