aframevr / aframe-inspector

:mag: Visual inspector tool for A-Frame. Hit *<ctrl> + <alt> + i* on any A-Frame scene.
https://aframe.io/aframe-inspector/examples/
MIT License
654 stars 201 forks source link

Remove Box3.prototype.expandByObject patch #686

Closed vincentfretin closed 8 months ago

vincentfretin commented 1 year ago

Remove Box3.prototype.expandByObject patch, this function is an old version of what now exists in latest threejs: https://github.com/mrdoob/three.js/blob/25859b6bdf64a13301dffdad6369b0c675584aa4/src/math/Box3.js#L155-L220

I mentioned it in my comment https://github.com/aframevr/aframe-inspector/issues/649#issuecomment-1364654680 It's been months since I commented that code in my project to not patch Box3 with an old version of expandByObject. I didn't see any issue.

I verified for example with the blue cube with a sphere as a child

<a-entity id="blueBox" mixin="blueBox" position="0 8 0">
  <a-entity id="yellowSphere" geometry="primitive: sphere" material="color:#ff0; metalness:0.0; roughness:1.0" position="-2 2 -2"></a-entity>
</a-entity>

This works properly: Capture d’écran du 2023-04-17 15-47-53

vincentfretin commented 9 months ago

I saw recently how I can have a NaN for position. There a small difference between having the patch or not. If you type inadvertently some characters for position that are not a digit, you will get a NaN in the field. With the patch, no error in the console and you have the previous blue bounding box still there but not the model. Without the patch, the model and blue bounding box disappear and you can the following error in the console: THREE.BufferGeometry.computeBoundingSphere(): Computed radius is NaN. The "position" attribute is likely to have NaN values.

I still believe the patch should be removed. Another fix could be done to avoid the field to have NaN and put back the previous value or 0.

vincentfretin commented 8 months ago

We definitely need to remove this old expandByObject patch, this fixed several issues that 3DStreet was having with some of their models https://github.com/3DStreet/3dstreet-editor/pull/383

I rebased from master and fixed the issue of having a NaN in a number input like position if you typed a character inadvertently, putting the previous value given in props.