Skip to content
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

Vector math API should make mul() less confusing #344

Closed
jmiskovic opened this issue Dec 6, 2020 · 4 comments
Closed

Vector math API should make mul() less confusing #344

jmiskovic opened this issue Dec 6, 2020 · 4 comments
Labels

Comments

@jmiskovic
Copy link
Contributor

The mul function in vector math API has inconsistencies which make it hard to remember and use correctly. I find it hardest part of LOVR API to remember and it results in most of my bugs due to unintended mutation.

The "mul" as function name is more about implementation (multiplying things together) and doesn't convey semantic meaning of vector operations. There are few different operations that all use the same name:

  • vec3:mul(n) is vector scaling, semantically same as vec3:div(1/n) and similar to non-mutating vec3 * n variant
  • vec3:mul(vec3) is component-wise multiplication (Hadamard product), I don't know if it's useful for small-dimensional vectors?
  • mat4:mul(mat4) and quat:mul(quat) stacks (combines) two matrices on top of each other
  • mat4:mul(vec3) and quat:mul(vec3) applies transformation to vector, without mutating mat4 / quat

The last usage is especially confusing. The rest of API (add, scale, rotate...) follows the convention that only object on which the operation is called gets mutated. The mat4:mul(vec3) will return transformed vector, but it will also mutate passed-in vector which is pretty big exception from rest of API.

I'm thinking how it could be improved at this point. Few suggestions:

  1. Add function transform() for vec3 and vec4, which would take mat4 / quat and it would mutate only object on which it's called (it could also be called apply() for brevity)
  2. Describe quat:mul(vec3) and mat4:mul(vec3) variants as deprecated and redirect readers to vec3:transform()
  3. Add function scale(n) for vec2 / vec3 / vec4 and deprecate mul() and div() for those vector types

This would make vector operations more consistent and memorable, so that users could focus on what they are trying to accomplish. For me personally even implementing just first suggestion would be useful as I could ignore mul operations that don't fit the mental model. Other suggestions would IMO benefit new users making sense of API and to avoid frustrating bugs.

Does this make sense? Has the ship already sailed for substantially changing vector math?

@bjornbytes
Copy link
Owner

The ship hasn't sailed! There's also #169 which I think about pretty regularly.

I can explain the reasoning behind the current API: it matches GLSL. That was the main inspiration behind the feature set, overloads, and naming of the current API.

  • In GLSL, * with two vectors is component-wise, and * with a vector and a number is scaling.
  • For matrices, * with two matrices is composition, but * with a vector is transformation.
  • quaternions in LOVR just followed matrices

That doesn't necessarily mean that the current API is good though, and I have totally experienced the brain pain when remembering how the mutation works with Mat4:mul(Vec3).

I like the idea of Vec3:transform(Mat4|Quat), or even separate functions for mat4/quat like transform and rotate. Should the __mul metamethod for Mat4 and Quat still work with vectors? Some people that are more math-oriented may be expecting that to exist, although then it would be a bit weird to have an inconsistency between __mul and mul.

At a higher level, I think this suggestion could also be stated as making the vector API less math-y and more semantic-y. Instead of thinking of operations as mathematical operations between the objects, think about them as high level operations like composition, scaling, rotations. There are other related changes that could be made as well, like treating Mat4 as a transform object rather than a matrix (e.g. identity -> origin). Deciding to expose an API that is higher level like this would probably be more appropriate for LOVR which aims to be cute and somewhat beginner-friendly.

@jmiskovic
Copy link
Contributor Author

I gave this some more thought.

Mutating vs non-mutating is a pretty clean cut. Using methods on objects always changes those objects only. Using meta-method operators always produces temporary object from one or two objects that remain unchanged. This is easy to remember and both those styles produce readable code.

The Mat4:mul(Vec3) is only current deviation from above rule, so it's a small change to move implementation into Vec3:transform(Mat4). The Vec3:rotate(Quat) also sounds useful.

I've already come to treat Mat4 and Quat as transform objects. That's why I suggested Vec3:apply(Mat4). Not sure in what ways would that affect the API? Note that they are still being used as plain data matrices in context of pose and perspective. Vec3 could double as operator for translation but it doesn't bring any clarity into vector math.

As for higher-level operations I don't know if 3D transformations can be simplified. The current approach makes sense: expose atomic operations that can be combined in various ways. It's also nice that operations use common math and computer graphics naming conventions, so transition into GLSL is less painful. One thing missing right now is a cookbook that would show some examples of how more complex operations are composed. I can start with some use cases I previously ran into, and submit PU on lovr-docs.

@bjornbytes
Copy link
Owner

Deciding between transform/rotate and apply:

  • Using apply makes a lot of sense to me, I like the word. It would be nice to have one function that can be used to apply another object onto another, supporting whichever variants make sense.
  • You could also take an approach where each object has "verb" functions that modify it in an easy to understand way. Mat4 has translate, rotate, scale, Vec has scale, rotate, and transform. Quat has rotate. The extreme version of this would be to have Mat4:transform and to rename Vec:add to Vec:translate, but those might be going too far.

@bjornbytes bjornbytes added the design hmm label Feb 21, 2021
@jmiskovic
Copy link
Contributor Author

The second option would result in more readable code. Imagining math-heavy function written against first option, it would be full of :apply() calls and we'd have to track down types to know the intention of each operation. The code written against second option would be more self documenting.

As for Vec:add vs Vec:translate I think I prefer add. With translate it is more clear that op is mutating Vec, but add maps better to + metamethod and it's shorter to write.

Those are my preferences at least.

jmiskovic added a commit to jmiskovic/lovr that referenced this issue Sep 8, 2021
As discussed in bjornbytes#344, the `Mat4:mul(Vec3)` and `Quat:mul(Vec3)` don't
follow the convention of vector library. Those forms mutate the passed
in vector and not the Mat4/Quat object on which method was called.

The `Mat4:mul(Vec3/4)` is moved to `Vec3/4:transform(Mat4)` and the
`Quat:mul(Vec3)` is moved to `Vec3:rotate(Quat)`.

The `Mat4:mul(x,y,z)` seemed like useful form, and it returns number
components without mutation. That form is now expanded to more useful
`Mat4:mul(x,y,z,w)` to be able to treat input vector as directional.
If not supplied, w defaults to 1 so that vector is treated as point. The
function now returns also the w component.

Because Mat4 has `mul(x,y,z,w)`, the similar form is added to Quat for
consistency: `Quat:mul(x,y,z)`.

This change breaks backward compatibility by removing three forms
`Mat4:mul(Vec3)`, `Mat4:mul(Vec4)` and `Quat:mul(Vec3)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants