Skip to content

Conversation

@vincentfretin
Copy link
Contributor

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 #649 (comment)
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
Copy link
Contributor Author

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 vincentfretin force-pushed the remove-expandByObject-patch branch from ad0616c to b848e35 Compare February 4, 2024 16:48
@vincentfretin
Copy link
Contributor Author

We definitely need to remove this old expandByObject patch, this fixed several issues that 3DStreet was having with some of their models 3DStreet/3dstreet-editor#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.

@dmarcos dmarcos merged commit 0b45e2e into aframevr:master Feb 4, 2024
@vincentfretin vincentfretin deleted the remove-expandByObject-patch branch February 4, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants