Error "Cannot read property 'isEmpty' of undefined" when cloning a Geometry with a Box3Helper

[jscastro76]

Description

I have found an issue that seems to be a bug, when cloning a geometry that has as children a Box3Helper.
This simple scene is producing an error message:

Uncaught TypeError: Cannot read property 'isEmpty' of undefined
    at pe.updateMatrixWorld (three.min.js:961)
    at Z.updateMatrixWorld (three.min.js:451)
    at xd.updateMatrixWorld (three.min.js:451)
    at Kd.render (three.min.js:223)
    at animate (threebug.html:98)

Repro steps

The issue is easy to reproduce:

• Create any geometry
• Create its Box3Helper
• Add the Box3Helper as its children
• Add the geometry to the scene
• Clone the object and add it to the scene.

I have tried many combinations including adding both to a Group to be siblings instead of parent/children, but it happens the same. I have tried to debug it my myself and the issue seems to be at this checkpoint Box3Helper, as the cloned instance has box as undefined.

Demo

Here is the repro of the issue: ( you only have to uncomment line 62: //scene.add(dupe); )

• jsfiddle (repro with the latest release branch)

Code

Here is the full js code:

import * as THREE from "https://threejs.org/build/three.module.js";
import { OrbitControls } from "https://threejs.org/examples/jsm/controls/OrbitControls.js";

var mesh, renderer, scene, camera, controls;

init();
animate();

function init() {

    // renderer
    renderer = new THREE.WebGLRenderer();
    renderer.setSize( window.innerWidth, window.innerHeight );
    renderer.setPixelRatio( window.devicePixelRatio );
    document.body.appendChild( renderer.domElement );

    // scene
    scene = new THREE.Scene();
    
    // camera
    camera = new THREE.PerspectiveCamera( 40, window.innerWidth / window.innerHeight, 1, 10000 );
    camera.position.set( 20, 20, 20 );

    // controls
    controls = new OrbitControls( camera, renderer.domElement );
    
    // ambient
    scene.add( new THREE.AmbientLight( 0x222222 ) );
    
    // light
    var light = new THREE.DirectionalLight( 0xffffff, 1 );
    light.position.set( 20,20, 0 );
    scene.add( light );
    
    // axes
    scene.add( new THREE.AxesHelper( 20 ) );

    // geometry
    var geometry = new THREE.SphereGeometry( 5, 12, 8 );
    
    // material
    var material = new THREE.MeshPhongMaterial( {
        color: 0x00ffff, 
        flatShading: true,
        transparent: true,
        opacity: 0.7,
    } );
    
    // mesh
    mesh = new THREE.Mesh( geometry, material );
    scene.add( mesh );
    
    let bb = new THREE.Box3().setFromObject(mesh);
	  let boxModel = new THREE.Box3Helper(bb, new THREE.Color(0xffff00));
	  boxModel.name = "BoxModel";
	  mesh.add(boxModel);
    
    let dupe = mesh.clone(true);
    //UNCOMMENT THIS LINE TO REPRO THE ISSUE
    //scene.add(dupe);
    
}

function animate() {

    requestAnimationFrame( animate );
    
	  mesh.rotation.x += 0.01;
	  mesh.rotation.y += 0.02;
    
    controls.update();    

    renderer.render( scene, camera );

}

Possible solution

Adding a copy method to Box3Helper.prototype will solve this issue as box will be set cloning the source one

	Box3Helper.prototype = Object.assign(Object.create(LineSegments.prototype), {

		constructor: Box3Helper,

		copy: function (source) {

			LineSegments.prototype.copy.call(this, source);

			this.box = source.box.clone();

			return this;

		}

	});

Three.js version

• [x ] r118

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• macOS
• Linux
• Android
• iOS

[Mugen87]

So the root cause of the issue is the missing Box3Helper.copy() method which breaks the invocation of Box3Helper.clone(). Notice that most helpers do not support cloning so far. This was already reported earlier e.g. in #13565.

The problem is that these classes derive the copy() implementation from their upper classes like LineSegments which are of course not sufficient to properly perform a copy.

Helpers often have a reference to other objects in the scene and graphically represent respective properties. So Box3Helper visualizes the dimensions of an instance of Box3 whereas CameraHelper visualizes the projection matrix of a camera.

I'm convinced that it's wrong if helpers clone these "parent" objects. I also think it makes no sense to just assign the object like so:

this.box = source.box;

It's probably best for now if the engine would avoid calling clone() for helpers.

[jscastro76]

@Mugen87 I understand the reasons you expose, but then what should it be done when you clone a 3D object that contains a helper?? traverse the object to remove the helper, then clone and recreate the helper again?

[Mugen87]

I'm sorry, I don't have a proper solution for this at hand. At first glance, it seems to me that performing no clone is better than performing a buggy one.

A better solution would be to find a common way how helpers are attached to objects. The engine could then detect objects that have a helper and properly process them during a clone.

[jscastro76]

It could be a by design decision not to implement a clone method for Box3Helper if performance is affected (despite we are only guessing because I understand there are no performance tests that can probe it yet).
What I honestly wouldn't understand is not to resolve a breaking error that can be corrected just checking first if box is undefined instead of calling its isEmpty function

if ( !box || box.isEmpty() ) return;

[Mugen87]

The thing is that box is a mandatory parameter when creating Box3Helper right now. So an undefined check is somewhat inconsistent in this context since the helper is not created correctly in the first place.

Let's see if it's possible to find a common policy for attaching helpers in three.js. This would also help in context of serialization/deserialization them (see #15714).

[jscastro76]

I totally trust on your criteria @Mugen87.
Thanks for your interest and support

[jscastro76]

This issue has been resolved as a consequence of the .copy() changes @Mugen87 implemented around here #19921 and here #19471.
Thanks @Mugen87!