Mesh collision improvements

[LeXXik]

Fixes #5765

• Switches the generation of the mesh collider to use the indexed array, which improves the trimesh generation time. You can find the benchmarking in the original issue.
• Adds a new option to the collision component that allows to disable the duplicate vertices test: checkVertexDuplicates. By default, when we add a new vertex to a trimesh, Ammo will compare the distance against all existing vertices and ignore it, if the distance is smaller than epsilon. This option allows to skip that check. The option is true by default, forcing the duplicates check.

entity.addComponent('collision', {
    type: 'mesh',
    renderAsset: '...',
    checkVertexDuplicates: false
})

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[Maksims]

Can RefCountedObject and RefCountedCache be used here to keep reference counters on the cache so that clearMeshCache would only destroy unused objects? This is similar to the way Mesh class is implemented.

[LeXXik]

Yep, I see where it could be useful to clear the cache, excluding the ones that are in use currently. I think we can add it in a subsequent PR, as I am not yet familiar with those reference objects.

[Maksims]

Yep, I see where it could be useful to clear the cache, excluding the ones that are in use currently. I think we can add it in a subsequent PR, as I am not yet familiar with those reference objects.

If clearMeshCache will be made public with this PR, then changing its logic in future PRs should be avoided.

[LeXXik]

Perhaps I am missing something. I don't see how it would hurt adding a ref object later? The API will stay the same. There would be no change to the signature. The difference would be that it would not crash if called when some components are still using the mesh object.

[mvaligursky]

clearMeshCache could be safely deprecated at that point and just do nothing, as memory would be released automatically when there are no references left to it.

[LeXXik]

Hmm. Well, the whole point of using a cache is that it would retain the trimeshes, even when no components are using those. All for the sake of avoiding recalculating the collision mesh when the component is enabled again, which potentially helps avoiding a stutter mid-game.

clearMeshCache should be called manually, when the developer knows the meshes would no longer be used. If the mesh is auto-destroyed when the component is disabled, then we don't need cache and also clearMeshCache is not needed. We can simply destroy it on disable.

If I understood Max correctly, then the reason to use ref objects is that the developer would still call clearMeshCache manually, but only when he knows that the disabled components would no longer be used. It would then destroy disabled ones, but keep the active ones alive.

[Maksims]

If I understood Max correctly, then the reason to use ref objects is that the developer would still call clearMeshCache manually, but only when he knows that the disabled components would no longer be used. It would then destroy disabled ones, but keep the active ones alive.

Yes, it would destroy unused (0 refs) objects, and keep used (0+ refs) objects.
But I would avoid using "enabled/disabled" logic here. As it is harder to communicate and can be complex in varous cases. Simple rule: nowhere used (no components refer it, enabled or disabled doesn't matter) - clearable. Is easier to communicate and follow by developer.

One case where clearing trimesh cash with disabled component can lead to issues:

1. Level finished.
2. Destroy old scene.
3. Load new scene but keep it disabled while it is loading assets.
4. Call clear trimesh cache.

If it will ignore disabled components, then it will not re-use existing trimeshes, as new scene entities are disabled - this is not good. Efficient way is to keep trimeshes as they are referenced.

Also automatic clearing when no refs are, with disabling entities - will definitely lead to unintentional re-creations in runtime - this to be avoided at all costs.

Please, do not implement auto-clearing of the cache, and do not consider disabled entity as "clearable".

[LeXXik]

I understand your concerns, Max. However, I am not sure I can agree. I generally prefer the system to be straightforward, rather than clever. We already have a secret feature, called trimesh cache that only cool kids know about. It does more harm than good, if you don't know about it. Every other shape and rigidbody is destroyed when component is disabled. If I am asking the system to clear the cache, I would like it to do just that, without it trying to interpret my second thoughts of what I really want. It just feels we would add another secret feature. Perhaps it could be customized with options? Nevertheless, I think this can be discussed in the PR that would add ref objects. I consider this one to be ready for review.

[Maksims]

Mesh - is not destroyed, when Render component is disabled.
When clearing cache in a browser, it does not break rendering of the page, it only ensures there is no cache, so future load (creation) will be from fresh. When calling GC - it does not destroy everything and throws exceptions, it only collects what is not used.

These concepts are not "clever", but consistent within the engine, and following existing logic that used across the engine - is strongly recommended.

Disabling/enabling component - should not lead to high CPU load for the benefit of free memory. That is why when you disable any entity, components are not destroyed / re-created, they just get disabled.
If disabling/enabling component will lead to CPU blocks - this is not communicated to the user, and they will require understanding of some other mechanics - this is not good.

[LeXXik]

I agree with you. I do prefer the engine to follow defined patterns. After all, this is how the user would start to expect a behavior. I was referring to Ammo shapes and bodies, created in Wasm memory. For every other rigidbody and collision component - when they are disabled, the respected counterparts in Wasm get destroyed and memory is freed. Mesh collision is an exception here, that doesn't follow the pattern.

[Maksims]

I agree with you. I do prefer the engine to follow defined patterns. After all, this is how the user would start to expect a behavior. I was referring to Ammo shapes and bodies, created in Wasm memory. For every other rigidbody and collision component - when they are disabled, the respected counterparts in Wasm get destroyed and memory is freed. Mesh collision is an exception here, that doesn't follow the pattern.

Because creation of it is very CPU intensive compared to other objects.

🙏

[mvaligursky]

I'd be keen to merge the first two points from the description (indexed array and checkVertexDuplicates) - I wish this was a separate PR.

But I'm not super happy with the cache implementation.
When the user calls the cache to be emptied, it can create an invalid state, where the ammo data are removed, but the components still reference those. We could:

• walk components at that point and disconnect invalid objects (while logging warnings), or probably better:
• use ref counting, similar to the way meshes do, and only release cached data that is no longer referenced.

[LeXXik]

@mvaligursky I have reverted the cache back to the original one.

[mvaligursky]

this could be
const numVertices = vb.getNumVertices(); instead?

[mvaligursky]

based on this (not sure this is the latest version, not an expert here) it seems findOrAddVertex and addIndex are internal, and so perhaps we should just keep using addTriangle and pass removeDuplicateVertices as false to it.
https://github.com/bulletphysics/bullet3/blob/master/src/BulletCollision/CollisionShapes/btTriangleMesh.h#L63-L66

One additional reason I suggest this is that I'm not sure the way those functions are currently used is correct. findOrAddVertex returns a new index in case a duplicate was found, but we're not handling it, which means that this code will access vertices that do not exist (our of range).
https://github.com/bulletphysics/bullet3/blob/39b8de74df93721add193e5b3d9ebee579faebf8/src/BulletCollision/CollisionShapes/btTriangleMesh.cpp#L86

[LeXXik]

Hmm, so we have 2 methods here:

1. using the previous addTriangle method with disabled duplicates check
2. using findOrAddVertex and addIndex with disabled duplicates check

The second one is about 10% faster than the first one (which can be around 5-15ms for complex meshes). Considering there is a faster method, do we still want to use the addTriangle? I'm ok with both options. Both of them seem to be order of magnitudes faster than the current method with forced duplicates check.

As for the returned index in case of a duplicate found - I am not sure if I understand. By new index do you mean an old index that was incremented? If that would be the case, then I'd agree - we could potentially point out of bounds. However, I don't see it being the case. This line returns an index of an existing vertex when a duplicate is found, not a new one:
https://github.com/bulletphysics/bullet3/blob/39b8de74df93721add193e5b3d9ebee579faebf8/src/BulletCollision/CollisionShapes/btTriangleMesh.cpp#L115

However, said that, I do think there should be a guard. If we consider worse case scenario - a user provides a mesh with many duplicate vertices and manually disables the duplicate test. This will generate a collision mesh, but with triangles that would not match the shape. If we do use the second method, then to mitigate this case, we could use the first method for the default checkVertexDuplicates = true, and go for the second one, when it is explicitly disabled (i.e. the developer knows the mesh is clean).

[mvaligursky]

I'd be ok to just use the first option using addTriangle with optionally disabled dupe test.

The second options I do not believe work at the moment, as we're not using the return value from findOrAddVertex, which is an index of existing vertex that the current vertex is a duplicate of.

The example, for our box primitive, I added breakpoint to createBox in procedural.js, and that creates a position array with 72 floats, which means (/3) 24 positions - each face has 4 unique vertices as they need unique UVs.

But for collision, this needs to be just 8 .. and so we add 24 positions to ammo and it returns indices of only 8 unique vertices - but we ignore those values. And then later we give it the original indices from the mesh, which point to all 24 verticies. The end result is - ammo has 8 verticies, but indices point to 24 vertices I believe.

So we can either remap those to just 8 positions. Or simply use the option 1 which does it.

[mvaligursky]

On the other hand - disabling dupe test is faster (because their code is just a linear search from what I can see), but for the box we'll end up with 24 positions instead of 8, so raycasting and all that would be more expensive every frame.

We could add simple hashing acceleration structure and trivially remove dupes ourselves. Perhaps even have Map<string, number> storing vertices and their index. We'd convert x, y, z to a string this way: ${x}-${y}-${z} and search in the map, this would be super fast way to strip dupes. And we should then remove the checkVertexDuplicates flag as we'd always check for dupes.

I think this is the real solution here, best of both worlds - the speed, removal of dupes, and no additional API.

[LeXXik]

Added a local vertex cache. I decided to keep the checkVertexDuplicates property on the collision component. Results on a 24k vertices model generating collision mesh:

before this PR: ~2485ms
entity.collision.checkVertexDuplicates = true; (default, uses local cache): ~181 ms
entity.collision.checkVertexDuplicates = false;: ~135ms

[mvaligursky]

Hi and sorry for not getting to this sooner. This looks great now, happy to merge it, thanks!