mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.75k stars 439 forks source link

overload operator * for vector component-wise multiplication is quite error prone in some cases #550

Closed bigbike closed 2 years ago

bigbike commented 2 years ago

The code can be found here.

I understand it was used quite often in the shading language but it is not the case at all for more general usage (e.g., physics simulation).

This is error-prone because it does not satisfy associative property when matrix is involved, namely, M * vec_a * vec_b != M * (vec_a * vec_b) where M is a 3x3 matrix, and vec_a and vec_b are 2 vectors.

I met a case where I actually needed the right hand side, but coded it up as the left hand side (a bug).

Suggest removing this operator overloading, as it causes ambiguity.

Maybe you can define a function, e.g., scale(vec_a, vec_b) instead.

(By the way, hope you are doing great @mosra! )

Squareys commented 2 years ago

I think this is actually quite nicely consistent with other languages like GLSL :)

Would be quite a pain to maintain backwards compat here ;)

mosra commented 2 years ago

Hi,

I'm afraid we'll have to disagree on this one. Vector component-wise multiplication using * is so widespread and such a common concept that I can't imagine providing a non-operator function for that. It's not just GLSL, but also HLSL, Metal's MSL, numpy arrays use it for this same thing, and even vector extensions in C have the same semantics.

I checked with Python's relatively new @ operator for matrix multiplication (so you could do M @ vec_a * vec_b to disambiguate matrix and component-wise multiplication), and even that has the exact same priority as *. Which means Python developers, even given the chance of introducing a completely new operator, didn't feel the need to distinguish between matrix and vector multiplication and left them in their implicit behavior for operators of the same precendence -- evaluated from left to right.

I would have a hard time convincing myself to replace a lot of *s with scale() and there would have to be a really convincing reason to force this change on all users of the library. Plus it could introduce a dangerous precedent, where next time * for matrices could be considered dangerous, replaced with matmul() .... and soon we'd end up with a very Java[Script]-like math API that would be a pain to use.

I have to admit I dropped various weird/dangerous aspects of GLSL like mat3(2.0) actually filling just the diagonal, or vec3() + 5.0 and similar being an allowed expression. But I don't consider this a dangerous API, at all, stuff like implicit conversions or division on integers is way more dangerous.

bigbike commented 2 years ago

Yeah, maybe a function/method call to do the element-wise multiplication is not a good idea but I made my point -- use a different operator to distinguish it from the general matrix multiplication. (My point is never to change the priority of the operator. I should clarify.) In that sense, you can even think of changing the operator of the matrix multiplication as well, like the py author did -- leaving the * operator to do the element-wise multiplication. (I know it is also a tough decision as well, and most likely would not happen. It is totally fine.)

Actually what they did for python was a quite smart move as when two different symbols (@ and *) appear in the same equation, people will think twice about the priority. That is their point, also mine.

mosra commented 2 years ago

If there would be a way to define custom C++ operators including their priority, and if the project was in its early stages, then yes, I would definitely spend time thinking how to improve the situation if such operations would show to be problematic. But there isn't (there's https://github.com/klmr/named-operator, but the operator precedence issues make it rather impractical and error-prone), and the project is already well-established with a sheer amount of projects relying on its continuity with no surprising changes to the core APIs. And I'd argue the way it is is the best possible given the tradeoffs.

Early on with the project I invested my time into introducing strongly typed angles, as a lot of accidental bugs were from deg/rad mismatches, and in retrospect those proved to be a really good decision. But * "abused" for matrix multiplication but also used for vector component-wise multiplication wasn't recognized as a problem. Column-major indexing and right-to-left multiplication direction were regularly complained about but this not -- you're the first. On the contrary, I don't remember anyone ever asking for component-wise matrix multiplication, it's not that common operation apparently. Which means, if I used * to perform component-wise matrix multiplication instead (to be consistent with * for vector component-wise multiplication), I expect I would get a lot of concerned bug reports very early on, because it's different from how everything else is known to work.