-
-
Notifications
You must be signed in to change notification settings - Fork 180
math: refactor Vec and Mat mixin structs #1420
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair disclaimer, these are just my opinions as someone who isn't an active contributor to this project, so take them with a grain of salt.
src/math/mat.zig
Outdated
/// The underlying Vec type, e.g. Mat3x3.Vec == Vec3 | ||
pub const Vec = vec.Vec4(Scalar); | ||
pub const Vec = vec.VecN(T, @min(rows, cols)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this type make sense for matrices without equal numbers of rows and columns?
If this definition of Vec
is kept, maybe it should be renamed to MinVec
to go alongside RowVec
and ColVec
, although this would create a breaking change since Vec
would no longer exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Mat type is not necessarily square anymore, maybe it just doesn't make sense to have this type at all? Those using Vec should replace that usage with RowVec or ColVec as appropriate.
I noticed a minor inconsistency: Vec takes a parameter named |
This change refactors the vector and matrix mixin structs to instead share a single generic struct. This allows for more generalized functions and less boilerplate. It also adds a new implementation of swizzle that is more flexible, more suitable for generalized vectors, and supports different axis names.
This change refactors the vector and matrix mixin structs to instead share a single generic struct. This allows for more generalized functions and less boilerplate. It also adds a new implementation of swizzle that is more flexible, more suitable for generalized vectors, and supports different axis names.
New mixin approach
This change moves all vector and matrix functions to their respective generic structs (e.g.
Vec2.dot
,Vec3.dot
,Vec4.dot
=>VecN.dot
). Functions that are specific to certain versions of the struct (e.g.Mat4x4.projection2D
) have also been moved to the generic struct. This means that the public API for these functions does not change at all; users can still callMat4x4.projection2D(.{ ... })
like normal.Somewhat confusingly, this means that users can also refer to this function as
Mat2x2.projection2D
, but if they attempt to use the result as aMat2x2
instead of aMat4x4
, there will of course be a compile error. I think this is a reasonable sacrifice compared to the benefits of this change.pub const x = Shared.x
boilerplatetranspose
,scale
,translate
, etc. can be merged into one implementationinit
New swizzle implementation
The current implementation is somewhat awkward to use and lacks the flexibility that swizzling in shaders feature. For example, using the current implementation, the input vector type must be the same as the output, e.g. you can't write
const v: Vec2 = myVec4.swizzle(.x, .y);
. You are also locked into using xyz to as your axis names. This is inconvenient when working with vectors of rgba values or texture coordinates. Finally, due to the current implementation of mixin structs, swizzle can't be used on higher dimensional vectors.My implementation solves all of these problems. Instead of an array of enum values, it takes a
comptime []const u8
which represents a list of the vector components to use. It accounts for returning vectors of a different size than the input, it allows you to use "xyzw", "rgba", and "stpq" depending on the application. It also has the bonus of being slightly faster to type (myVec.swizzle("yzw")
instead ofmyVec.swizzle(.y, .z, .w)
).P.S. I know it's probably bad to include this in the same PR, but making these changes separately would result in wasted work. If you would like me to move it into a new PR, I will do that.
Public API changes
Swizzle changes arguments from enum to string
VectorComponent enum removed
init
functions for vector and matrix structs take slices instead of the exact number of arguments (the reexports in main remain the same, however)Vec.fromInt
removed from generic struct (the reexports in main remain the same, however)Mat.Vec changed to Mat.MinVec
By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.