gkjohnson / three-mesh-bvh

A BVH implementation to speed up raycasting and enable spatial queries against three.js meshes.
https://gkjohnson.github.io/three-mesh-bvh/example/bundle/raycast.html
MIT License
2.39k stars 247 forks source link

Serialize / Deserialize: Clean up, think through use cases #337

Closed gkjohnson closed 2 years ago

gkjohnson commented 2 years ago
bzztbomb commented 2 years ago

A really good use case is pre-computing the BVH and storing it either inside of a model (via GLTF extensions) or along side it.

We currently have a processing pipeline that performs some transforms on a GLTF and one of the last steps is to compute the BVH and store it in a GLTF file. For every mesh in the GLTF we write out the BVH roots to a buffer and add the extension to the mesh to point at the buffer. On the Three side, we hook our extension in and it reads the roots and calls deserialize on them to setup the BVH. It works quite well!

BTW, this is a really nice library, it's great to work with!

gkjohnson commented 2 years ago

Hey @bzztbomb!

A really good use case is pre-computing the BVH and storing it either inside of a model (via GLTF extensions) or along side it.

That's a great use -- I was hoping people would be able to pre-build the BVHs and load them. I assume you have your own custom GLTF Extension you're using? If you have a public example of this it would be fun to see.

For this issue I mostly wanted to think through the usability of the API, default values, and the ability to share the same BVH buffers across WebWorkers so they can be used in multiple contexts using the same memory. Since you're using this maybe you'll have some thoughts?

First I was thinking about changing the "copyIndexBuffer" option (which currently defaults to "true") to be "cloneBuffers" and default to "true" which will indicate whether both the geometry index buffer and BVH root buffers will be cloned when creating the new serialized js object. Currently "copyIndexBuffer" only indicates that the geometry index buffer will be cloned.

And then if a user wanted to use the same buffers across WebWorkers they would have to do the following:

Let me know if you have any thoughts on the above. This is one of the last things to get done before the next v0.5.0 release.

BTW, this is a really nice library, it's great to work with!

Great to hear. If you're able to share your project or how you're using MeshBVH I'd love to hear more. It's always nice to see how the work is helping and hearing use cases always helps development direction, as well.

Thanks again!

bzztbomb commented 2 years ago

That's a great use -- I was hoping people would be able to pre-build the BVHs and load them. I assume you have your own custom GLTF Extension you're using? If you have a public example of this it would be fun to see.

Here's a gist of the extension and the script to pre-compute and store the BVH.

Let me know if you have any thoughts on the above. This is one of the last things to get done before the next v0.5.0 release.

That sounds great! API seems clear to me and that use case is awesome!

Great to hear. If you're able to share your project or how you're using MeshBVH I'd love to hear more. It's always nice to see how the work is helping and hearing use cases always helps development direction, as well.

I'm working with @dbuck. Our project isn't public yet, but I'll definitely point you at it when it is!

donmccurdy commented 2 years ago

FYI @bzztbomb and @gkjohnson, I'm working on refactoring the glTF Transform internals a bit, and this would affect how custom extensions are implemented. For the example above, I think the ThreeMeshBVH class would be replaced with something like the snippet below. The TypeScript syntax is optional (unlike before, when the TypeScript Decorators were required).

interface IBVH {
  roots: Accessor[];
}

class BVH extends ExtensionProperty<IBVH> {
  public readonly propertyType = 'BVH';
  public readonly parentTypes = [PropertyType.PRIMITIVE];
  public readonly extensionName = 'THREE_mesh_bvh';
  public static EXTENSION_NAME = 'THREE_mesh_bvh';

  protected getDefaultAttributes(): Nullable<IBVH> {
    return { roots: [] };
  }

  addRoot(root: Accessor): this {
    return this.addRef('roots', root);
  }

  removeRoot(root: Accessor): this {
    return this.removeRef('roots', root);
  }

  listRoots(): Accessor[] {
    return this.listRefs('roots');
  }
}

That should be enough to ensure the BVH's ownership of the roots Accessors is tracked correctly, so it won't be lost by processing functions like prune() or dedup().

More details:

bzztbomb commented 2 years ago

@donmccurdy Thanks for the heads up, the new version seems pretty straightforward. I think I may have run into the ownership issue and worked around it by making sure this step always happens last. I really like gltf-transform it's a great project!