mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.6k stars 35.36k forks source link

Matrix3 invalid translation implementation #24731

Open eXponenta opened 2 years ago

eXponenta commented 2 years ago

Describe the bug

Matrix3 has invalid translate implementation.

https://github.com/mrdoob/three.js/blob/master/src/math/Matrix3.js#L267

tx, ty not should affect on scale elements, and must be applied only on translation elements.

Take a look at glMatrix:

https://glmatrix.net/docs/mat3.js.html#line345

WestLangley commented 2 years ago

For context, please see the PR where this method was introduced.

Please read the entire thread.

Please also see the source code of https://threejs.org/examples/webgl_materials_texture_rotation.html.

//

We decided early-on in the development of three.js, that we rotate and translate objects -- not matrices.

So, to be honest, I am not a fan of these Matrix3 methods. I think they are confusing, and I doubt they are needed.

I do not like removing methods, but I'm thinking this may be an exception...

eXponenta commented 2 years ago

I not sure that method should be removed.

For texture matrices should be introduced new class as TextureMatrix, where this method is specified for texture usage if needed.

There are a convention of matrices operations. Scale is scale, translation is translation, rotation is rotation.

This translation is not translation for matrices, because cannot be presened as

R = M * T, where R is translated result, T - translation matrix ( 1 0 tx, 0 1 ty, 0 0 1)

What is represented? What it means mathematicaly ?

WestLangley commented 2 years ago

If I recall correctly, the methods were added to replicate the equivalent CSS API, as suggested by @mrdoob here.

See the three.js example referenced above. It requires modifying the example and setting texture.matrixAutoUpdate = false; -- something I doubt users typically do.

//

Something may not be correct, but as I said, I think the methods are confusing, and deviate from the three.js API conventions.