Strict type check on geometry object before accessing key like isGeometry

Describe the bug

Application throwing error Cannot read property ‘isGeometry’ of null .

To Reproduce

Send/pass geometry object as null.

Code snippet

https://raw.githubusercontent.com/mrdoob/three.js/dev/build/three.js

_proto.expandByObject = function expandByObject(object) {
			// Computes the world-axis-aligned bounding box of an object (including its children),
			// accounting for both the object's, and children's, world transforms
			object.updateWorldMatrix(false, false);
			var geometry = object.geometry;

			if (geometry !== undefined) {// There should be strict type check here.  Why you are not handling null condition.  
				if (geometry.boundingBox === null) {
					geometry.computeBoundingBox();
				}

				_box.copy(geometry.boundingBox);

				_box.applyMatrix4(object.matrixWorld);

				this.union(_box);
			}

			var children = object.children;

			for (var i = 0, l = children.length; i < l; i++) {
				this.expandByObject(children[i]);
			}

			return this;
		};

Expected behavior
There should be strict type check on geometry object before fetching any key inside object.
.

Screenshots

If applicable, add screenshots to help explain your problem (drag and drop the image).
image

Platform:

  • Device: [Desktop ]
  • OS: [ MacOS ]
  • Browser: [Chrome ]

3 thoughts on “Strict type check on geometry object before accessing key like isGeometry

  1. The argument to expandByObject is expected to be an Object3D or a subclass. The Mesh.geometry property is not nullable. We do not typically include validation of incorrect inputs to three.js methods, as this creates performance costs paid even by users who are using the APIs correctly.

  2. Checking for .isGeometry has a different effect — until r125, three.js had two different geometry implementations, Geometry and BufferGeometry. They are not subclasses of one another, and the .isGeometry check was there to detect the older type.

    With Geometry now removed, perhaps we should be checking these properties differently… I’m not sure on that.

  3. Cannot have similar condition everywhere ? How will it effect performance ?

    I would rather remove the check in SkinnedMesh than adding them elsewhere in the code.

    The checks for isGeometry were introduced so the engine can specifically log a warning if users still use Geometry in context of certain engine features like raycasting. After a while, we can delete them and always assume instances of BufferGeometry.