Update three-to-ammo to 1.0.4

[vincentfretin]

This closes #66

[vincentfretin]

I am now getting
ammo.wasm.wasm:0x1fa79 Uncaught RuntimeError: memory access out of bounds
on most of the examples and we didn't have the error with three-to-ammo 1.0.3. @Tpleme any idea?
I'll have to revert otherwise.

[Tpleme]

Hi, the fact that this error is not thrown in some examples tells me that it might have to do with some components used in the examples.

I can investigate this and try to find the culprit and go from there

[Tpleme]

I found out that importing aframe-physics-system with local files throws this error.
<script src="../../dist/aframe-physics-system.js></script>

While doing it with the following does no trhow any error:
<script src="https://cdn.jsdelivr.net/gh/c-frame/aframe-physics-system@v4.2.4/dist/aframe-physics-system.js"></script>,

Maybe this can be solved by rebuilding the dist files?

EDIT: This is incorrect, importing from cdn does not throw error because it seems that the physic system is not even initializing.

This is very strange because the updates made to three-to-ammo should not cause something like this.

I will continue my investigation

[vincentfretin]

I didn't release 4.2.4 or made a tag yet, so the url gives a 404.
If you put back in package.json exactly the npm:@c-frame/three-to-ammo@1.0.3 version
npm install
npm run dev
and check for example http://locahost:8000/ammo/sweeper.html
it works, but with 1.0.4 it fails.

[Tpleme]

I found out that the changes on three-to-ammo is causing this error. Im working on a fix

[Tpleme]

This is the if statment that i will be takling from now on

export const iterateGeometries = (function() {
  const inverse = new THREE.Matrix4();
  return function(root, options, cb) {
    inverse.copy(root.matrixWorld).invert();
    root.traverse(mesh => {
      const transform = new THREE.Matrix4();
      if (
        mesh.isMesh &&
        mesh.name !== "Sky" &&
        (options.includeInvisible || (mesh.el && mesh.el.object3D.visible) || mesh.visible)
      ) {
        if (mesh === root) {
          transform.identity();
        } else {
          mesh.updateWorldMatrix(true);
          transform.multiplyMatrices(inverse, mesh.matrixWorld);
        }
        // todo: might want to return null xform if this is the root so that callers can avoid multiplying
        // things by the identity matrix

        let vertices;
        if (mesh.geometry.isBufferGeometry) {
          const verticesAttribute = mesh.geometry.attributes.position;
          if (verticesAttribute.isInterleavedBufferAttribute) {
            //
            // An interleaved buffer attribute shares the underlying
            // array with other attributes. We translate it to a
            // regular array here to not carry this logic around in
            // the shape api.
            //
            vertices = [];
            for (let i = 0; i < verticesAttribute.count; i += 3) {
              vertices.push(verticesAttribute.getX(i));
              vertices.push(verticesAttribute.getY(i));
              vertices.push(verticesAttribute.getZ(i));
            }
          } else {
            vertices = verticesAttribute.array;
          }
        } else {
          vertices = mesh.geometry.vertices;
        }

        cb(
          vertices,
          transform.elements,
          mesh.geometry.index ? mesh.geometry.index.array : null
        );
      }
    });
  };
})();

I will try to explain myself the best i can.
This error: RuntimeError: memory access out of bounds is not caused by the most recent changes to three-to-ammo being defective, but rather because they are working as intended and now exposing a pre-existing, underlying issue in the physics system's handling of skipped meshes.

This error occurs because something crucial must happen insisde this if statment block for every mesh that the physics system tracks. When a mesh is skipped (when the condition returns false = mesh invisible), the error is thrown.

I suspect that this was a old error, because the || mesh.visible addition on InfiniteLee/three-to-ammo@91597be, seems to intentionally mitigate this bug.
I kown that @diarmidmackenzie did some investigation on this repo commit history, maybe he/she can elaborate a bit more.

As identified in the comments of issue #66, this adition is a bad fix. The mesh.visible property is always true even when the parent A-Frame entity's visible component is false. By including || mesh.visible, the code was likely forcing all meshes (even the logically invisible ones) to pass the condition and execute the processing block, thus inadvertently preventing the error.

Keep in mind that my WebXR knowledge is very limited as i am starting to learn, but i think i solution to this is to find what peice of code of the if statment block is cruscial to run even when a mesh is invisible.

I will continue my investigation but i think its better to revert this commit until there is a definitive fix.

[diarmidmackenzie]

Yes I saw this out of memory error when I was originally experimenting with the issue you reported.

Will add some notes later on what might be causing this (when I have had a chance to refresh my memory)

[diarmidmackenzie]

Ok, this was my note from before...

"When I use this, it's also necessary to set "includeInvisible: true" on the invisible child (else you hit an infinite loop in the ammo wasm, I guess because you have an ammo shape with no geometries?)"

Based on this observation, I will look a bit more carefully at what might actually be going on here...

[diarmidmackenzie]

I have been looking at the compund example (it's a fairly simple one).

I can fix it by replacing this:

<a-box width="75" height="0.1" depth="75" ammo-body="type: static" ammo-shape visible="false"></a-box>

with:

<a-box width="75" height="0.1" depth="75" ammo-body="type: static" ammo-shape="includeInvisible: true" visible="false"></a-box>

Most other examples have the same invisible floor, so I think this is probably the issue for all of them.

Stack trace is this:

ammo.wasm.wasm:0x1fa79 Uncaught RuntimeError: memory access out of bounds
    at ammo.wasm.wasm:0x1fa79
    at ammo.wasm.wasm:0xbee0
    at ammo.wasm.wasm:0xb0a12
    at ammo.wasm.wasm:0xb2e6a
    at ammo.wasm.wasm:0xb3806
    at ammo.wasm.wasm:0x5d9db
    at ammo.wasm.wasm:0x5da3c
    at ammo.wasm.wasm:0x5e658
    at ammo.wasm.wasm:0x5db7e
    at ammo.wasm.wasm:0x5c12b

Interestingly, if I remove the controllers completely ("left-hand"" and "right-hand"), without the change above, that also avoids the "memory access out of bounds" error.

However the floor doesn't function as expected, because we don't get an ammo shape for it because it's invisible.

Question I have is whether an invisible floor, without ammo-shape="includeInvisible: true" should act as a floor or not. I suspect we need to make it so this does work, else we have a major back-compatibility issue.

So let's look at how to define the behaviour of the includeInvisible flag so that it allows the invisible floor pattern we use in all the examples, but also enables @Tpleme's use case...

[diarmidmackenzie]

So @Tpleme's use case involved an entity like this...

<a-box id="box" height="0.5" width="0.5" depth="0.5" material="color: red;" ammo-body="type: dynamic; mass: 1;" ammo-shape="type: hull;">
        <a-box id="inner-box" visible="false" ammo-body="type: kinematic; emitCollisionEvents: true; disableCollision: true;" ammo-shape></a-box>
</a-box>

The intention was that the child entity should not be included in shape calculations because includeInvisible was "false", and the child was invisible.

But the following pattern is common in examples:

<a-box width="75" height="0.1" depth="75" ammo-body="type: static" ammo-shape visible="false"></a-box>

Here, the box is also invisible, includeInvisible is "false", but we expect the mesh to be included in the shape calculation - presumably because it is the mesh on this entity, not a child.

So maybe the logic we want is that includeInvisible controls only whether meshes on child entities are included, based on their visibility?

There's no documentation on the "includeInvisible" option in the three-to-ammo repo. The only documentation of this option is in aframe-physics-system, where it says:

"Should invisible meshes be included when using fit: all"

That's not very clear. Based on the above, and established usage patterns, maybe the best update would be like this:

"Should invisible meshes on child entities be included when using fit: all"

If we updated code to match that, we might get behaviour we want in all cases.

There is also a separate question as to why we are seeing the out-of-memory error. But it's not clear that problem will remain if we adjust the usage of includeInvisible to match the suggestion above.

[Tpleme]

yes, i agree with you. This property is not very clear at all, but if we do as you suggest thing become more clear, it should affect invisible children of the object, and this property should be on the parent itself.

In the if statment, instead of checking if includeInvisible is true, should we check if the parent has this property as true? Yet i don't know if this in going to mitigate the nemory error.

[vincentfretin]

Interestingly, if I remove the controllers completely ("left-hand"" and "right-hand"), without the change above, that also avoids the "memory access out of bounds" error.

That would mean the issue is in the wasm collision algorithm between the kinematic body and an empty static body.

[vincentfretin]

I also agree with your proposal, checking visibility of el.children that are different than el.getObject3D('mesh').

[diarmidmackenzie]

Looking some more at this - I have expressed this in terms of A-Frame concepts (entities), but we will be implementing in three-to-ammo, which is a generic three js library, and should not depend on A-Frame.

It's not obvious to me how to distinguish between the parts of a multi-mesh GLTF attached to an A-Frame entity (where we'd want to include all parts) and a mesh that is a child of an entity that's a child of the A-Frame entity.

@vincentfretin 's suggestion of looking for meshes that are not el.getObject3D('mesh') won't work in the case of a multi-mesh GLTF, as the component meshes of the GLTF won't match `getObject3D('mesh'). In any case getObject3D is an A-Frame method, so it's inelegant to depend on this in three-to-ammo.

The best option I can see is to vary behaviour depending on where the visibility is set to "false" in the object hierarchy.

If the mesh is invisible because the root (i.e. the A-Frame Entity) is invisible, then we include it no matter what. But if it's invisible because of a visibility setting on some child of the root, then we excude it (unless includeInvisible is true).

So code becomes...

const isObjectVisibleUpToRoot = (object, root) => {
  if (!object.visible) return false
  
  if (object.parent && object.parent !== root) {
    return isObjectVisibleUpToRoot(object.parent)
  }
  
  return true
}

      if (
        mesh.isMesh &&
        mesh.name !== "Sky" &&
        (options.includeInvisible || isObjectVisibleUpToRoot(mesh, root))
      )

That seems to fix all the examples.

It should also fix @Tpleme 's use case - though I haven't tested that yet.

[Tpleme]

I've tested your solution with my use case and can confirme that it does solve the problem.

I have also checked the examples and none returns the memory error.

[diarmidmackenzie]

That's great - can you fix up the PR and resubmit? Then one of us will merge.

[Tpleme]

A new PR was created to address this issue: c-frame/three-to-ammo#7

[diarmidmackenzie]

I have published 1.0.5 just now, so you should be able to fix up this PR by including 1.0.5 and rebuilding.

[Tpleme]

Done
#68