Proposal: eliminate the optional target

There are about 30 methods that have an optionalTarget argument.

getWorldPosition: function ( optionalTarget ) {

	var result = optionalTarget || new Vector3();

	this.updateMatrixWorld( true );

	return result.setFromMatrixPosition( this.matrixWorld );

},

I suggest optionalTarget be made non-optional — the policy being that three.js methods do not instantiate objects except in constructors and in methods such as clone().

getWorldPosition: function ( result ) {

	this.updateMatrixWorld( true );

	return result.setFromMatrixPosition( this.matrixWorld );

},

On a related note are the getPoint() methods in the Curve classes. They, too, instantiate an instance and return it. But that is for another thread.

Author: Fantashit

5 thoughts on “Proposal: eliminate the optional target

  1. Just to see what that means in numbers I created a jsperf-test for the three possible scenarios (i.e. using es6 default-values, old-school default-values and without them alltogehter). See and run it here: https://jsperf.com/default-parameters-performance-cost – the difference isn’t that big in most cases and the versions with and without default-value will both turn out as winner from time to time (at least in chrome 60 and FF55 on osx).

    However, I agree that a policy like “three.js methods do not instantiate objects except in constructors and in methods such as clone()” would have its benefits in terms of clarity.

    As for deprecating, it might be nice not to write every single instance of such a call to the console, as that would render most applications unusable until fixed. Alternative could be a helper that just outputs the first instance for each deprecation-warning. Something like

    var printedWarnings = {};
    function deprecationWarning(id, message) {
    
      if ( !printedWarnings[id] ) {
        console.warn(message);
        printedWarnings[id] = true;
      }
    
    }
  2. I think the following are bad form and it would make the library worse to make these changes:

    Camera.getWorldDirection( result ) -> Vector3.getWorldDirectionFromCamera( camera )
    Object3D.getWorldPosition( result ) -> Vector3.getWorldPositionFromObject( object )
    Object3D.getWorldQuaternion( result ) -> Quaternion.getWorldQuaternionFromObject( object )
    Object3D.getWorldRotation( result ) Euler.getWorldRotationFromObject( object )
    Object3D.getWorldScale( result ) -> Vector3.getWorldScaleFromObject( object )
    Object3D.getWorldDirection( result ) -> Vector3.getWorldDirectionFromObject( object ) // this would have to be merged with the camera method
    Box2.getCenter( result ) -> Vector2.getCenterFromBox2( box2 )
    Box2.getSize( result ) -> Vector2.getSizeFromBox2( box2 )
    Box2.getParameter( point, result ) -> Vector2.getParameterFromBox2( box2, point )
    Box2.clampPoint( point, result ) -> Vector2.clampPointInsideBox2( box2, point )
    Box3.getCenter( result ) -> Vector3.getCenterFromBox3( box3 )
    Box3.getSize( result ) -> Vector3.getSizeFromBox3( box3 )
    Box3.getParameter( point, result ) -> Vector3.getParameterFromBox3( box3, point )
    Box3.clampPoint( point, result ) -> Vector3.clampPointInsideBox3( box3, point )
    Plane.projectPoint( point, result ) -> Vector3.projectPointOnToPlane( point, plane )
    Plane.intersectLine( line, result ) -> Vector3.intersectLineWithPlane( line, plane )
    Plane.coplanarPoint( result ) -> Vector3.getCoplanarPointFromPlane( plane )
    Line3.getCenter( result ) -> Vector3.getCenterFromLine3( line3 )
    Line3.delta( result ) -> Vector3.getDeltaFromLine3( line3 )
    Line3.at( t, result ) -> Vector3.getPointOnLine3( line3, t )
    Line3.closestPointToPoint( point, clampToLine, result ) -> Vector3.getClosestPointOnLineToPoint( line3, point, clampToLine )
    Ray.at( t, result ) -> Vector3.getPointOnRay( ray, t )
    Ray.closestPointToPoint( point, clampToLine, result ) -> Vector3.getClosestPointOnRayToPoint( ray, point, clampToLine )
    Ray.intersectPlane( plane, result ) -> Vector3.getRayIntersectionWithPlane( ray, plane )
    Ray.intersectBox( box, result ) -> Vector3.getRayIntersectionWithBox( ray, box ) // what if it does not
    

    If this was being seriously considered I would likely want to speak up forcefully against it. I think it pollutes Vector3 with all these other classes. It encourages Vector3 to know about every other possible class that could create a Vector3. This is really the opposite of well designed code — well designed code should discourage coupling as much as possible. The only coupling one should allow is complex classes (Camera, Object3D, etc.) can depend on simple classes (Vector3, Box3, etc.), ,but not the other way around. This proposed change would make Vector3 dependent on nearly everything, which sort of the opposite of what you want.

    To put it in another way, you want a software library’s dependencies organized like a series of layers.
    The first layer is very basic classes,like the Vector3, Box3, Ray classes in the /math directory. These have no dependences except for themselves. The next set of classes is those that are moderately more complex like Camera, Mesh, AttributeBuffer, Geometry, and Object3D. These are dependent on each other and the simple classes. Then you have more complex types of WebGLRenderer, etc. These are dependent on the other layers.

    The whole idea is that you reduce cyclic dependences that simple types have on complex types as much as possible to decouple the system — to untangle the knot of everything dependent on everything.

    This is also going to make Vector3 huge and less focused. It will have algorithms related to cameras, meshes, etc in it.

    It will not be possible to just import the ThreeJS math library into other projeccts, because Vector3 will be dependent on everything else in ThreeJS because Vector3 will reference nearly everything.

    So I do feel this is not a good way to go. It is also very untypical in 3D libraries.

  3. This has largely been completed — except for the ParametricGeometries and the Curve-based classes.

    The curve classes are a mixture of 2D-specific, 3D-specific, and dimension-agnostic classes. I think a refactoring or redesign is required.

    The goal is to remove optionalTarget from getPoint(). However, getPoints() will still have to allocate an array.

    Closing.

Comments are closed.