New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: eliminate the optional target #12231
Comments
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 );
}
I'm glad you are mentioning this! But yeah, let's tackle this separately. |
We will have to figure out something reasonable...
I think that is an unrelated issue. The return statement is for chaining. |
I agree wholeheartedly to this proposal; random object allocations are something to be avoided for real-time applications. |
Isn't the whole point to break the code here? Not breaking code would mean still allocating memory somewhere. |
That sure is much nicer... 🤔 |
That coding pattern completely defeats the purpose of the proposal. |
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):
both of these have performance costs. Tiny costs, but it is something to consider. |
This could be the most sound test for one's application. If it breaks, you're not doing it right anyways. Agree with this:
|
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. |
May be the issue is that these functions should be in a different place. If we moved the function to vector.setFromObjectWorldPosition( object ); setFromObjectWorldPosition: function ( object ) {
object.updateMatrixWorld( true );
return this.setFromMatrixPosition( object.matrixWorld );
} And, at this point, the function is almost irrelevant... |
@mrdoob You suggested that previously, but @bhouston had a convincing reply.
|
I guess is good that I'm consistent 😅 |
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 );
}, |
Out of curiosity... What prompted you to suggest this? |
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;
}
} |
@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. |
@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 ) |
Coding patterns from users on SO. |
@bhouston thanks for pointing that out. Updated the test, and unsurprisingly the results are more stable now, still not that much of a difference. |
@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. |
When we talk about backward compatibility, i always have to think at a certain lecturer 😆 |
These changes look good to me 👍 |
@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... |
I think the following are bad form and it would make the library worse to make these changes:
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 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. |
@bhouston Well, there is my original proposal at the top of this thread. I prefer the original proposal, of course. What is your opinion? |
@WestLangley I do like your original proposal, I think it is the right way to go. |
@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? |
I totally agreed with @bhouston about the polluting against Vector3, which will become unmaintainable with this way. And
Should be a lead coding pattern in the lib ! About @Mugen87
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 ! |
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.
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. |
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 !
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. |
@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. |
MrDoob isn't agreed to port three to Typescript, and dislike jsdoc comment "polluting"... |
What is the goal of 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 |
sure, here are a couple of attempts: https://jsfiddle.net/4xuuqg6v/3/ Interestingly it seems that using function replacement is faster than using flags. |
|
@donmccurdy Wow ! i was not aware of that rollup-plugin-strip !!! This is excellent !!! 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 ? |
Probably the check can be reduced from this: if(debugEnabled){
if ( target === undefined ) {
throw new Error('THREE.Matrix4: Invalid target argument !')
}
} to this: assert( target !== undefined, 'THREE.Matrix4: Missing required target.' ); |
@donmccurdy that why I specified
😄 |
I'm fine with this 👍 |
React and probably other libraries/frameworks already have this sort of developer-friendly / performant modes. In react for example they are using tests for The way this works is by using a webpack-plugin that statically replaces all occurences of For the build-process here, we could do that using the rollup-plugin-replace-plugin for rollup. For instance by using a global variable if (THREE_DEV) {
// ... optional developer-friendly checks here
} After rollup, that would become 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 |
@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 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 |
pretty much the same, up to about 3-4x difference. Hence the "attempt" word used :) |
This has largely been completed -- except for the 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 Closing. |
There are about 30 methods that have an
optionalTarget
argument.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 asclone()
.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.The text was updated successfully, but these errors were encountered: