Three.js: Change the behavior or name of Matrix.getInverse() to align with the rest of the API

[AndrewPrifer]

Is your feature request related to a problem? Please describe.

At any other place in the API I've seen so far, the semantics of methods are <source>.getSomething(<target>). Except for Matrix.getInverse where this becomes <target>.getInverse(<source>). I find this extremely confusing and it can expose developers to some nasty bugs. It is true that the docs do clarify this, however it goes against some natural assumptions you make as a programmer. You assume that the API is consistent, and you assume that a getter gets a property or derived value of the this object, and not the parameter.

Describe the solution you'd like

Either change the semantics of the getInverse function to apply the inverse of this to the parameter matrix, or change the name to setToInverseOf().

For reference, in Babylon this is much more clearly named: https://doc.babylonjs.com/api/classes/babylon.matrix#inverttoref

[Mugen87]

see https://github.com/mrdoob/three.js/pull/18026#issuecomment-559952681

Unfortunately, changing the way getInverse() works would break code. Introducing a new method would be the safest solution but I would love to see the existing getInverse() fixed. Simply because I'm annoyed every time I see this method because of its inconsistency 😅.

@mrdoob Assuming getInverse() is not used by the average three.js user, do you think we could take the risk and change it to <source>.getInverse(<target>)? To me, the current behavior is more a bug than anything else...

[AndrewPrifer]

@Mugen87 I figured as much, but I was so annoyed after spending half an hour debugging just to find _this_ to be the source of the problem, that I had to open an issue. 😅

[RemusMar]

At any other place in the API I've seen so far, the semantics of methods are <source>.getSomething(<target>).
Except for Matrix.getInverse where this becomes <target>.getInverse(<source>).

Honestly, both are wrong.
Logically speaking, these are the correct methods:

new = getInverse(old)
and
new = old.getInverse()

Don't you agree?

[AndrewPrifer]

@RemusMar it's this way for performance.

[RemusMar]

it's this way for performance.

If you think so, don't change anything.

[AndrewPrifer]

@RemusMar in your example, a new object is created, which can impact performance. My issue was about consistency. Tbh I also dislike mutation, but a 3D lib has different priorities. :)

[mrdoob]

I don't like either way to be honest.

I like this pattern better:

matrixInv.copy( matrix ).invert();

[Mugen87]

So instead of fixing the existing method you want to introduce invert() which would be similar to identity().

Should getInverse() be deprecated then?

[mrdoob]

@Mugen87 Yep!

[Mugen87]

Okay, I'll give it a go!