bug? removeComponent 'dynamic-body' doesn't clean dependent component 'velocity' well

[ooatom]

Version: 4.0.1

Description:

<a-scene>
    <a-entity id="container"></a-entity>
    <a-entity id="target" dynamic-body></a-entity>
</a-scene>

run

const target = document.getElementById('target');
target.removeAttribute('dynamic-body');
target.parentElement.removeChild(target);
container.appendChild(target);

would raise an exception:

core:a-node:error Failure loading node:   TypeError: Cannot read property 'constructor' of undefined
    at extendProperties (aframe.js:77578)
    at NewComponent.callUpdateHandler (aframe.js:77258)
    at NewComponent.updateProperties (aframe.js:77142)
    at HTMLElement.updateComponent (aframe.js:75824)
    at HTMLElement.updateComponents (aframe.js:75797)
    at entityLoadCallback (aframe.js:75583)
    at emitLoaded (aframe.js:76511)

It looks like removing the component 'dynamic-body' doesn't remove the dependent component 'velocity' well.

Quick Fix

I have no time to dig out more information to fix it properly. Currently, just simply fix it by adding below in my code:

const target = document.getElementById('target');
target.removeAttribute('dynamic-body');

+ target.removeAttribute('velocity'); +

target.parentElement.removeChild(target);
container.appendChild(target);

[Galadirith]

@OORaiser Thanks so much for raising this issue. I am looking into this at the moment. Some preliminary findings and thoughts

1. This issue doesn't just manifest when you remove the dynamic-body component
   This means that it's not specifically to do with not cleaning up the dependent component velocity. Adapting your example, the following also invokes the same error

   const target = document.getElementById('target');
   target.parentElement.removeChild(target);
   container.appendChild(target);

2. The safest option is to leave the velocity component when dynamic-body is removed
   As far as I know, there is not automatic mechanism in place in core A-Frame that will clean up dependent components. In theory a user could add a velocity component to an entity independently of dynamic-body. In such a scenario it would not be appropriate to remove it when dynamic-body is removed. We could add an attribute to keep track of a velocity component that is added by a dynamic-body, but even then it is possible that the user takes control of that component and again it would not be appropriate to remove it when dynamic-body is removed.

   I would be very glad to hear your thoughts, but my inclination would be to leave the code as it is, and then have the user explicitly remove the velocity component if they want to along with removing a dynamic-body component. Because of (1) I don't see this as being an issue any more.

3. The error does not appear when a-entity is replaced with a geometry like a-box
   Specifically if there is an empty target.object3D at the time dynamic-body is initialised, and then the entity is recycled as in your example, the error with be thrown. I would be inclined to say that this is a bug, if no error is thrown with an empty object3D when it is initially loaded, then there shouldn't be an error when the entity is recycled, and definitely worth investigating. However I cannot think of a situation where you would want to apply dynamic-body to an empty entity without any associated geometry. Would you be able to provide an example for context.

   You can see the specific fork for cases when there is an empty object3D

   https://github.com/n5ro/aframe-physics-system/blob/d6806f81a6b772c1838ddffa2ff0e71dcfecb649/src/components/body/body.js#L69-L72

I'm looking it to this now and I will update when I have more details or a solution to provide consistent behaviour between an initial or recycled load with an empty object3D.

@MignonBelongie @Micah1 could I please ask for one of you to assign me this issue, thanks 😊

[Galadirith]

Further testing shows it is not to do with object3D, I was mistyping a-box 😅 I apologise for the misleading info.

However what I have identified to be important is Component.attrValue property, specifically

document.getElementById('target').components['velocity'].attrValue

When velocity is a component on it's own, this attribute attrValue gets populated with an object of the same structure as it's the components schema, by default {x: 0, y: 0, z: 0}. However when it is a dependency of another component, attrValue is never set and is left undefined. This causes issues later on in the the updateProperties call, where for some reason attrValue is taken as the true value, and overwrites components['velocity'].data. But as attrValue is undefined, .data is no longer an object with a constructor property and we get this error.

Here is an completely self-contained example that is completely independent of aframe-physics-system

<!DOCTYPE html>
<html lang="en">
<head>
<script src="https://aframe.io/releases/1.0.4/aframe.min.js"></script>
<script type="text/javascript">
AFRAME.registerComponent('test-dependency', {schema: {type: 'vec3'}});
AFRAME.registerComponent('test-component', {dependencies: ['test-dependency']});
</script>
</head>  
<body>
<a-scene>
  <a-entity id="target" test-component></a-entity>
</a-scene>
<script type="text/javascript">
setTimeout( () => {
  var target = document.getElementById('target');
  var parent = target.parentElement
  parent.removeChild(target);
  parent.appendChild(target);
}, 3000)
</script>
</body>
</html>

Note that if the schema for test-dependency in this example is something simple like a number or a list an error not thrown, and as such I believe this is conditional on the isObjectBased attribute of a component. Specifically it appears at src/core/component.js#L416 and then src/core/component.js#L736, where in the second instance source.constructor is never queried for isObjectBased == false because of the lazy condition evaluation of JavaScript. I'm guessing the isObjectBased is an internal A-Frame attribute, because everything is an object in JavaScript 😊

At this point I would conclude that this is not an aframe-physics-system issue. However I would very much like hear your thoughts @OORaiser before this issue is closed to make sure you're happy with this conclusion. I would recommend raising this as an issue in the main A-Frame repo, please do reference this issue, and I can also try to contribute to a fix over there 😊

[Galadirith]

Just one final note, if you do explicitly specify the dependent component, for example

<a-entity id="target" test-component test-dependency="0 0 0"></a-entity>

then you also do not get an error. It is only in the case where the dependent component has to use it's default values where this becomes an issue.

[ooatom]

@Galadirith Thanks for your analysis. I think it's the A-Frame issue too. I would submit a new issue for this.

[ooatom]

@Galadirith It seems that the way I was using A-Frame is not recommended. HTML tags are best used only to build the basic skeleton.

Sorry to have wasted your time.

[Galadirith]

@OORaiser thanks so much for the update, and definitely not a waste of my time :blush:

I hope that this issue happens rare enough in practice that it doesn't affect you, but if you do run into this issue and can't figure out a way to refactor your code using the suggested solution, please feel free to let me know and I'm more than happy to help in whatever way I can :blush: