bjornbytes / lovr

Lua Virtual Reality Framework
https://lovr.org
MIT License
2k stars 138 forks source link

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

Closed jmiskovic closed 1 year ago

jmiskovic commented 3 years ago

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:

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 commented 3 years ago

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.

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 commented 3 years ago

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 commented 3 years ago

Deciding between transform/rotate and apply:

jmiskovic commented 3 years ago

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.