Three.js: Proposal: eliminate the optional target

[WestLangley]

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.

[Mugen87]

How can we change this without breaking code (in cases users rely on object creation)? Besides, do we still need a return statement?

It's just a code style issue but using ES6 default parameters makes the code also nicer:

getWorldPosition ( result = new Vector3() ) {

    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.

I'm glad you are mentioning this! But yeah, let's tackle this separately.

[WestLangley]

How can we change this without breaking code

We will have to figure out something reasonable...

do we still need a return statement?

I think that is an unrelated issue. The return statement is for chaining.

[Usnul]

I agree wholeheartedly to this proposal; random object allocations are something to be avoided for real-time applications.
As such I see this change as both reducing complexity of the three.js own code as well as enforcing a good practice for its users.

[makc]

How can we change this without breaking code

Isn't the whole point to break the code here? Not breaking code would mean still allocating memory somewhere.

[mrdoob]

getWorldPosition ( result = new Vector3() ) {

  this.updateMatrixWorld( true );

  return result.setFromMatrixPosition( this.matrixWorld );

}

That sure is much nicer... 🤔

[WestLangley]

That sure is much nicer...

That coding pattern completely defeats the purpose of the proposal.

[Usnul]

using default values for parameters is really neat in principle, as a language feature, it makes writing some code a lot easier. Under the hood it results in 2 problems (not really novel, "arg = arg || default" is much the same):

• conditional check, at best argument count on the stack
• polymorphic method signature

both of these have performance costs. Tiny costs, but it is something to consider.

[pailhead]

This could be the most sound test for one's application. If it breaks, you're not doing it right anyways.

Agree with this:

That coding pattern completely defeats the purpose of the proposal.

[mrdoob]

That sure is much nicer...

That coding pattern completely defeats the purpose of the proposal.

I see I see. I wasn't sure if you were aiming for code clarity or otherwise.

The (obvious) problem I see is backwards compatibility.

[mrdoob]

May be the issue is that these functions should be in a different place.

If we moved the function to Vector3 it would look like this:

vector.setFromObjectWorldPosition( object );

setFromObjectWorldPosition: function ( object ) {

    object.updateMatrixWorld( true );
    return this.setFromMatrixPosition( object.matrixWorld );

}

And, at this point, the function is almost irrelevant...

[WestLangley]

@mrdoob You suggested that previously, but @bhouston had a convincing reply.

I am concerned this isn't a scalable because if we were to follow this paradigm religiously, any function that would return a Vector3, say the Curve class, now would have to have its function on Vector3, so Vector3.getCurvePointAt( curve, t ). But so many operations in 3D return a Vector3 which means that Vector3 will get polluted by various relatively unrelated functions (such as other curve types, the Triangle.barycoordinate function, scale and Euler angles from both Matrix3 and Matrix4,... the list is incredibly long.)

[mrdoob]

I guess is good that I'm consistent 😅

[mrdoob]

But yeah... the backwards compatibility problem still stands I think.

I guess we'll have to do this...?

getWorldPosition: function ( target ) {

    if ( target === undefined ) {
        console.warn( 'THREE.Matrix4: Blah' );
        target = new Vector3();
    }

    this.updateMatrixWorld( true );

    return target.setFromMatrixPosition( this.matrixWorld );

},

[mrdoob]

Out of curiosity... What prompted you to suggest this?

[usefulthink]

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;
  }

}

[bhouston]

@usefulthink array.push() is not fast. As it does allocations and is fairly complex itself. You should try a simpler function body to get a better result.

[WestLangley]

I guess is good that I'm consistent

@mrdoob Well, I decided to have a look at your suggestion anyway -- just to see...

// Camera

Camera.getWorldDirection( result ) -> Vector3.setFromWorldDirection( camera ) // or setWorldDirectionFromCamera() or setFromCameraWorldDirection()

// Object3D

Object3D.getWorldPosition( result ) -> Vector3.setFromWorldPosition( object ) // or setWorldPositionFromObject()

Object3D.getWorldQuaternion( result ) -> Quaternion.setFromWorldQuaternion( object )

Object3D.getWorldRotation( result ) -> Euler.setFromWorldRotation( object )

Object3D.getWorldScale( result ) -> Vector3.setFromWorldScale( object )

Object3D.getWorldDirection( result ) -> Vector3.setFromWorldDirection( object ) // this would have to be merged with the camera method

// Color

Color.getHSL( result ) -> this is a special case...

// Box2
Box2.getCenter( result ) -> Vector2.setFromBox2Center( box2 ); // or setCenterFromBox2()

Box2.getSize( result ) -> Vector2.setFromBox2Size( box2 ); // or setSizeFromBox2();

Box2.getParameter( point, result ) -> Vector2.setParameterFromBox2( point );

Box2.clampPoint( point, result ) -> Vector2.clampPointFromBox2( point );

I would prefer the "set-what-from-whom" pattern if we were to do this.

setWorldPositionFromObject( object )

[WestLangley]

Out of curiosity... What prompted you to suggest this?

Coding patterns from users on SO.

[usefulthink]

@bhouston thanks for pointing that out. Updated the test, and unsurprisingly the results are more stable now, still not that much of a difference.

[bhouston]

@usefulthink I have no clue how aggressive these optimizers are on simplistic test cases (does everything get inlined and presolved) or whether it can achieve this type of performance on the complexity in production code.

[Mugen87]

When we talk about backward compatibility, i always have to think at a certain lecturer :laughing:
https://www.youtube.com/watch?v=DQ8XB7SjfRQ&t=2104s

[makc]

just kids who are smart

[mrdoob]

@mrdoob Well, I decided to have a look at your suggestion anyway -- just to see...

These changes look good to me 👍

[WestLangley]

@mrdoob's suggestion is a significant API change... I think we should give this serious thought before heading down this path... (The alternate would be to do what I suggested in my original post above.)

// Camera

Camera.getWorldDirection( result ) -> Vector3.getWorldDirectionFromCamera( camera )

// Object3D

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

// Color

Color.getHSL( result ) -> this is a special case...

// Box2

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

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

Plane.projectPoint( point, result ) -> Vector3.projectPointOnToPlane( point, plane )

Plane.intersectLine( line, result ) -> Vector3.intersectLineWithPlane( line, plane )

Plane.coplanarPoint( result ) -> Vector3.getCoplanarPointFromPlane( plane )

// Line3

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

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 intersect ? <====


// Sphere

Sphere.clampPoint( point, result ) -> Vector3.clampPointInsideSphere( sphere, point )

Sphere.getBoundingBox( result ) -> Box3.setFromSphere( sphere )


// Triangle

todo...


// All the curves

todo...

[bhouston]

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.

[WestLangley]

@bhouston Well, there is my original proposal at the top of this thread. I prefer the original proposal, of course. What is your opinion?

[bhouston]

@WestLangley I do like your original proposal, I think it is the right way to go.

[WestLangley]

@mrdoob My proposal is to file a PR that makes the optional target non-optional, eliminating the use of the following pattern from the library:

var result = optionalTarget || new Vector3();

The policy would be that three.js methods do not instantiate objects, except in constructors and in methods such as clone().

Would that be something you would support?

[Itee]

I totally agreed with @bhouston about the polluting against Vector3, which will become unmaintainable with this way.

And

the policy being that three.js methods do not instantiate objects except in constructors

well designed code should discourage coupling as much as possible.

Should be a lead coding pattern in the lib !

About @Mugen87

How can we change this without breaking code (in cases users rely on object creation)?

getWorldPosition ( result = new Vector3() ) {

    this.updateMatrixWorld( true );

    return result.setFromMatrixPosition( this.matrixWorld );

}

It's look good but is in conflict with the first proposal, so to avoid breaking change and allow user to make their port we should use, like @mrdoob said:

getWorldPosition: function ( target ) {

    if ( target === undefined ) {
        console.warn( 'THREE.Matrix4: Blah' );
        target = new Vector3();
    }

    this.updateMatrixWorld( true );

    return target.setFromMatrixPosition( this.matrixWorld );

},

Finally, just a suggest about parameters usage in threejs. I never see ( or in rare case ) check against wrong parameters in the lib, that's make really difficult to debug in certain case ( I will make a proposal about this topic ).

For example this example "should"/could become after R89:

getWorldPosition: function ( target ) {

    if ( target === undefined ) {
        throw new Error('THREE.Matrix4: Invalid target argument !')
    }

    this.updateMatrixWorld( true );

    return target.setFromMatrixPosition( this.matrixWorld );

},

So... ok this pattern must not be apply in core or in class/methods called in render pipeline to avoid big performance regression. But this could help a lot developers to catch error as soon as possible in their code !

[Usnul]

I would even drop the checks for === undefined.

similarly, i would soon drop return parameter also, as long as it's the target that's being returned. It saves some cpu cycles and removes something that no longer serves a purpose.

[bhouston]

i would soon drop return parameter also

Are you sure the return parameter doesn't get optimized out if you do not use it? Also it you return it, it allows you to chain together functions very nicely, which leads to concise code:

Vector3.dot( Object3D.getWorldPosition( worldPosition ), c );

I always though the compiler was very good at optimizing unused return parameters.

[bhouston]

or example this example "should"/could become after R89:

Back in my C++ days we had ASSERTs that only existed in debug builds. Those debug builds ran slow but they found so many issues.

getWorldPosition: function ( target ) {
        Assert.notNull( target );
        Assert.instanceOf( target, THREE.Matrix4 );

    this.updateMatrixWorld( true );

    return target.setFromMatrixPosition( this.matrixWorld );

},

I haven't seen tooling in the JavaScript world that allows this easily. But I guess that is where FB's Flow and Microsoft's typescript come in. I understand that a lot of companies are having great success with typescript making JavaScript more robust.

[Usnul]

Vector3.dot( Object3D.getWorldPosition( worldPosition ), c );

Maybe it's a personal preference, but I would write it as

Object3D.getWorldPosition( worldPosition );
Vector3.dot(worldPosition , c );

with respect to how smart the compiler is - it is a little tricky. Most of the time - i would say yes, but in cases where de-optimization occurs (such as functions with catch clauses) - I am not so sure.

Assert.notNull( target );

actually there's a little bit exploitation that can be used in JS to achieve this. JIT performs opportunistic optimizations and constant folding, so if you write your assert like so:

function notNull(target){
  if(debugEnabled){
   if(target === null){ 
      throw new Error("We're all going to die, especially you");
   }
  }
}

then you might end up with zero overhead... or close to that in case of opportunistic optimization.

[Itee]

@bhouston

Back in my C++ days we had ASSERTs that only existed in debug builds.

Yes ! This is a real problem in javascript library like threejs, that why i refactor units test part ! And of course this type of check should be in dev only !

@Usnul

then you might end up with zero overhead... or close to that in case of opportunistic optimization.

Could you prove this assert ? Because if this is right it could be a very big deal i think to make safer library. See #12574 part 3.

[cecilemuller]

@bhouston If you do want to give Typescript a try but don't want to go all the way to pure typescript, there is the checkJs mode which uses JSDoc comments and works quite well (although of course it can do more with actual Typescript) and is a great complement to Eslint.

[Itee]

MrDoob isn't agreed to port three to Typescript, and dislike jsdoc comment "polluting"...
but yes a more strongly typed javascript could be cool.

[donmccurdy]

What is the goal of vector.setFromObjectWorldPosition( object ), since it's still not backwards-compatible? The benefit I see is that getFoo( target ) methods are relatively rare in JS libraries, so the naming may come more naturally to new developers.

If that's the goal, and we're OK with breaking backwards-compatibility anyway, another idea would be to keep the methods where they are but rename them in a way that makes their effect clearer. Example:

Object3D.prototype.worldPositionToVector = function ( result ) { ... };

// ... or ...

Object3D.prototype.applyWorldPosition = function ( result ) { ... };

But I'd be OK with @WestLangley's suggestion, or keeping things as they are with opt-in performance.

Checking for target === undefined to throw deprecation warnings seems fine, given that (1) checks could be removed in a few versions anyway, and (2) checks can be stripped out during minification if that's a concern. See: rollup-plugin-strip.

[Usnul]

@Itee

Could you prove this assert ? Because if this is right it could be a very big deal i think to make safer library.

sure, here are a couple of attempts:

https://jsfiddle.net/4xuuqg6v/3/
https://jsperf.com/assert

Interestingly it seems that using function replacement is faster than using flags.

[makc]

https://jsperf.com/assert

what were your results? I got this:

[makc]

chrome does not show any signs of "zero overhead" neither

[Itee]

@donmccurdy Wow ! i was not aware of that rollup-plugin-strip !!! This is excellent !!!
And i totaly agreed with you about the naming convention. For me the semantic IS the key of a good code.
A code that could be read like very hight level language is the best to do !
So yes a thing like this:

Object3D.prototype.applyWorldPositionTo = function ( vector3 ) { ... };

Is just perfect between the method concerne and the semantic !

@Usnul thank for bench ! This is very interesting.

@makc i got the same second result

So to recap, using this pattern or equivalent assert:

getWorldPosition: function ( target ) {

        if(debugEnabled){
        if ( target === undefined ) {
        throw new Error('THREE.Matrix4: Invalid target argument !')
        }
        }

    this.updateMatrixWorld( true );

    return target.setFromMatrixPosition( this.matrixWorld );

},

allow in development to check parameters in a very safe way, which is the right way IMO ! With not the best performance when testing it ( but this is not a problem i think ! ), and better performance when disabled.

And if threejs integrate the rollup-plugin-strip it will get better performance than currently ( due to all check removal ) when used in production ! This is AWESOME !

@mrdoob what do you think about that ?

[donmccurdy]

Probably the check can be reduced from this:

if(debugEnabled){
    if ( target === undefined ) {
    throw new Error('THREE.Matrix4: Invalid target argument !')
    }
}
````

to this:

```js
assert( target !== undefined, 'THREE.Matrix4: Missing required target.' );

[Itee]

@donmccurdy that why I specified

So to recap, using this pattern or equivalent assert:

😄

[Mugen87]

The policy would be that three.js methods do not instantiate objects, except in constructors and in methods such as clone().

I'm fine with this 👍

[usefulthink]

React and probably other libraries/frameworks already have this sort of developer-friendly / performant modes. In react for example they are using tests for process.env.NODE_ENV === 'production' throughout the code to enable or disable costly developer-features/warnings and things like that.

The way this works is by using a webpack-plugin that statically replaces all occurences of process.env.NODE_ENV with the string corresponding to the build-environment. The dead-code-elimination takes care of removing any trace of the developer-specific code when minifying.

For the build-process here, we could do that using the rollup-plugin-replace-plugin for rollup. For instance by using a global variable THREE_DEV with a boolean value. For example

if (THREE_DEV) {

    // ... optional developer-friendly checks here

}

After rollup, that would become if (false) { ... } for production-builds, which uglify will remove completely when minifying. For closure-compiler this would require running with ADVANVED_OPTIMIZATIONS, which is (probably for a good reason) currently not enabled. So we'd still need a solution here, but that shouldn't be too complicated.

What do you think? I would be happy to write a PR for this..

EDIT @Itee, sorry I posted this before fully reading all of the other replies. It's quite similar to what you're suggesting (wasn't aware of the existance of rollup-plugin-strip either), but the approach described here would allow for entire code-blocks to be removed when building, instead of only removing specific calls.

[Itee]

@usefulthink No prob !

I think it will be dangerous to use somethings else than assert ( or simply statement ) for check parameters in dev. Allow user to add stuff about development every where in the library would become quickly unmaintainable.

The problem with a plugin like replace is the regex to use for removal. Ok you will easily match if (THREE_DEV) but what about the inner stuff ?

To be sure to match things like that you will require a end tag like:

if (THREE_DEV) {

    // ... optional developer-friendly checks here

} // END_THREE_DEV_TAG

But my purpose was only for ctor, setter and any methods that user can call, a.k.a not private stuff in fact, so where user can make mistake about args.

getWorldPosition: function ( target ) {

        assert( target !== undefined, 'THREE.Matrix4: Missing required target.' );

    this.updateMatrixWorld( true );

    return target.setFromMatrixPosition( this.matrixWorld );

},

Look very good to me, with rollup-plugin-strip.

Anyway ! I think the topic is derivating from the first purpose... which was eliminate the optional target

So for people that are interesting by the parameters check, come to #12578

[Usnul]

@makc

what were your results?

pretty much the same, up to about 3-4x difference. Hence the "attempt" word used :)
It's quite a disappointment to me, since doing an opportunistic optimization here would be very sensible, replacing the whole if clause with nothing and having a trap in the flag's memory location - but that's too much to wish for, apparently.
More interestingly - i'm surprised to see that calls to empty functions result in much the same overhead. I guess I learned something :)

[WestLangley]

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.