Three.js: Overloading of functions

[cnspaha]

Is there any downside of passing a Vector3 instead of three single values into functions like Matrix4,makeTranslation(x, y, z) ?

If not I would make a few PRs in the future where Matrix4.makeTranslation(x, y, z) and other functions would also accept a Vector3. Of course I would also at it in the documentation.

So I would suggest to allow "overriding" of functions, I know JavaScript can't do this but it is possible to do it by code.

Example:

// three.js/src/math/Matrix4.js

// Before
makeTranslation: function ( x, y, z ) {

  this.set(
    1, 0, 0, x,
    0, 1, 0, y,
    0, 0, 1, z,
    0, 0, 0, 1
  );
  return this;
},...

// After (with "overriding")
makeTranslation: function ( x, y, z ) {

  if (x instanceof THREE.Vector3) { // haven't tested this line, but should work
    z = x.z;
    y = x.y;
    x = x.x;
  }

  this.set(
    1, 0, 0, x,
    0, 1, 0, y,
    0, 0, 1, z,
    0, 0, 0, 1
  );
  return this;
},...

Three.js version

• [x ] Dev

Browser

• [x] All of them

OS

• [x] All of them

[Mugen87]

From my point of view, this is an unusual pattern for the library. three.js normally creates separate methods in these cases like Box3.setFromArray(), Box3.setFromBufferAttribute() or Box3.setFromPoints(). I prefer parameters which types are unambiguously.

BTW: I think the correct term is overloading and not overriding. The latter is actually no problem in JavaScript. To realize overloading, the developer has to ensure that the respective method has a different behavior for different sets of parameters.

https://stackoverflow.com/a/34337046

[cnspaha]

Ah yeah, I meant "overloading" ;)

So it would be rather Matrix4.makeTranslationByVector(v) ? Would be also fine with me, even though I like overloading, but I don't want to break coding rules ;)

[Mugen87]

Sry, i don't think we should go this way. The project would need to add lot of new methods for consistency reasons which makes the API and the maintenance unnecessarily complicated.

But this is just my personal opinion. Let's see what others developers say.

[Mugen87]

BTW: It's easy for users to extend the default prototypes of three.js with custom logic. If you have special requirements to the API, you can always do something like this:

https://jsfiddle.net/eouq64nk/

But you should generally be careful with extending prototypes that you don't own. It's actually bad practice 😇 .

[cnspaha]

npnp, I understand your thoughts.
But yeah, lets wait for others to give their opinion.

Thanks for your example.
But is there a benefit compared to just doing the following?

THREE.Matrix4.prototype.makeTranslationByVector = function (vector) {
    // doing stuff
};

Okay, your looks more awesome, but don't they do the same thing?

[Mugen87]

Okay, your looks more awesome, but don't they do the same thing?

It's basically the same, yes :wink:

[WestLangley]

We support multiple types in certain cases. For example,

Color.set( value ) // value can be of multiple types
Mesh( geometry, material ) // material can be an array

This method is particularly problematic for users:

camera.lookAt( new THREE.Vector3( x, y, z ) );

Users often do this, instead, and are confused when it doesn't work.

camera.lookAt( x, y, z );

Furthermore, because a Vector3 is required, I have seen a Vector3 instantiated in the render loop

camera.lookAt( new THREE.Vector3( x, y, z ) );

I think supporting alternate types is fine in certain cases -- lookAt() may be a good candidate.

[RemusMar]

I think supporting alternate types is fine in certain cases -- lookAt() may be a good candidate.

+1

[Mugen87]

think supporting alternate types is fine in certain cases -- lookAt() may be a good candidate.

I admit that makes sense. But i still think we should carefully use this pattern.

[pailhead]

What about:

camera.lookAtVector( new THREE.Vector3( x, y, z ) );
camera.lookAt( x, y, z );

?

[mrdoob]

I think supporting alternate types is fine in certain cases -- lookAt() may be a good candidate.

Supporting camera.lookAt( x, y, z ) sounds good to me 👍

[WestLangley]

Can you think of a less-hacky way of implementing it than this?

lookAt: function () {

    var m1 = new Matrix4();
    var vector = new Vector3();

    return function lookAt( x, y, z ) {

        if ( x.isVector3 ) {

            vector.copy( x );

        } else {

            vector.set( x, y, z );

        }

        if ( this.isCamera ) {

            m1.lookAt( this.position, vector, this.up );

        } else {

            m1.lookAt( vector, this.position, this.up );

        }

        this.quaternion.setFromRotationMatrix( m1 );

    };

}(),

[mrdoob]

Nope. I think that should do the trick 😊

[cnspaha]

@mrdoob

Supporting camera.lookAt( x, y, z ) sounds good to me 👍

so overloading matrix4.makeTranslation(v) doesn't?
don't know how often you use this function but most of the time I use it I have a vector instead of three single values.
But if @mrdoob or @WestLangley says no, I will accept it ;)

[mrdoob]

I have to say that I've found myself needing the same for makeTranslation() too... 😅