bitshifter / glam-rs

A simple and fast linear algebra library for games and graphics
Apache License 2.0
1.55k stars 159 forks source link

Improve glam source readablility #177

Closed bitshifter closed 2 years ago

bitshifter commented 3 years ago

Internally glam got a lot more complex when support for other primitive types were added, e.g. f64, i32 & u32.

There are a couple of sources of complication:

The purpose of both of these is to avoid a lot of code duplication between different types. Avoiding duplicating comments is almost as significant a motivator as avoiding code duplication. The old simple way that glam was written felt like it would not scale, at least not while the library is still evolving.

The macros are generally what users run into when they want to read glam source for some reason although it's likely the next hurdle will be the core crate traits.

One thought on how to make the source more readable is to use a templating language to generate the glam source instead of using Rust macros. The generated source would then be committed and that is what users would compile and view. Contributors would ideally update the template files, however PRs could be made to the generated source and then later ported back into the template generation.

The core crate feels more necessary still, I don't see an easy way to avoid that, but at least without the macros it might be easier to follow.

A few people have suggested https://github.com/dtolnay/cargo-expand for code gen. It's meant as a debugging tool but maybe it will do the job, or could be modified to support this use case.

Another suggestion was to look at rust-analyzer and cargo xtask generate.

mickare commented 3 years ago

Hey @bitshifter ,

that are some good thoughts. I think that the core the library is well designed for the current stable state of rust. I like the direct access to the storage values.

As you said, the macros are one big part that could be improved. But for me improving the macros is not all about the code readability. I'm missing the generic vectors that "magically" work. I would like a u8 vector, or custom type vectors.

One way, to achieve this, would be to move and separate each vector method (e.g. abs(), clamp(), is_nan(), etc.) into traits. This could reduce the heavy macro usage by using generic implementations for known scalar types (SIMD as well).

I'm not sure about Rust best-practices, but I would group traits into definition and impl files depending on the functionality.


Here some examples, how it could look... (Click to expand) ##### Storage Traits ```rust // Empty trait that marks a storage of a type T; could be extended with a Item type... trait Storage; impl Storage for Vec3 {} trait Map: Storage { type Output: Storage; fn map R>(self, func: F) -> Self::Output; } trait Join : Storage { type Output: Storage; fn join R>(self, other: Rhs, func: F) -> Self::Output; } ... impl Map for Vec3 { type Output = Vec3; #[inline] fn map R>(self, func: F) -> Self::Output { Self::Output::new(func(self.x), func(self.y), func(self.z)) } } impl Join> for Vec3 { type Output = Vec3; #[inline] fn join R>(self, other: Vec3, func: F) -> Self::Output { Self::Output::new(func(self.x, other.x), func(self.y, other.y), func(self.z, other.z)) } } ``` ----------------- ##### Math Traits ```rust impl Add for Vec3 { type Output = Self; #[inline] fn add(self, other: Self) -> Self::Output { self.join(other, ::add) } } impl Product for Vec3 { type Output = Self; #[inline] fn product(self, other: Self) -> Self::Output { self.join(other, ::mul) } } impl Dot for Vec3 { type Output = T; #[inline] fn dot(self, other: Self) -> Self::Output { self.product(other).sum() } } ``` ------------------------- ##### Scalar Traits ```rust use num_traits::sign::Signed; // ... definition of traits Abs, Clamp, etc.. with docstrings impl Abs for Vec3 { #[inline] fn abs(self) -> Self { self.map(::abs) } } impl Clamp for Vec3 { #[inline] fn clamp(self, min_value: T, max_value: T) -> Self { let func = |v| v.max(min_value).min(max_value); self.map(func) } } ```
bitshifter commented 3 years ago

glam has intentionally avoiding being a generic library. Instead it has focused on the most common use case (f32) and has expanded a bit from there due to demand to support some other primitive types such as f64, i32 and u32.

It's not clear from you example how this approach would work using a storage type of __m128 with a public scalar type of f32 as the traits only have one generic type T but surely two would be required?

In short, I think it is difficult to achieve both using explicit SIMD usage in combination with a generic interface and I think it would complicate the public interface greatly so it's not a direction I've ever pursued.

Internally I do use a lot of traits to implement functionality and to get some code reuse, however it is much more complicated and you tend to run into confusing compile errors. It has always been a goal of glam to provide a simple interface which doesn't impose this kind of complexity on users, so all of this complexity is internal only for now except for the case where users want to view the source.

Another problem of implementing everything via traits is the documentation. You can no longer view the docs for Vec3 and see all the methods it implements. Documentation is on the trait, not the struct so it's much harder for users to find all the methods a struct implements. You need to find the trait that implements the method you want and then find if it's implemented for the type you are interested in. cgmath is a good example of this issue, try and find the documentation for dot for https://docs.rs/cgmath/0.18.0/cgmath/struct.Vector3.html. You need to work out what trait has dot (there are multiple) and then work out which trait is implemented for Vector3.

mickare commented 3 years ago

After two weeks of experiments to implement a generic ndarray lib that has both const & dynamic structs (with the new const-generics) and views on part of the data, I must confess that generic trait implementations are the worst abominations that exists in rust (both in std and libs) and should be avoided at all costs. :goberserk: I learned the hard way.

And using macro impl for many type specifications of a generic struct can result in a docs monster. Just take a look what I found today: https://rust-lang.github.io/packed_simd/packed_simd_2/struct.Simd.html Have fun. xD

~~ Brainstorming mode ON ~~ I get your point on the complexity and documentation issues of the "mini traits" approach. Maybe having a big generic trait with most methods could also reduce the duplicated docs between each Vec struct? It could keep most common methods on a single docs page? E.g.: Expose the Vector / Matrix trait?


Response to the previous example questions.... It was just an idea, and not a solution. ;) > It's not clear from you example how this approach would work using a storage type of `__m128` with a public scalar type of `f32` as the traits only have one generic type `T` but surely two would be required? Maybe it can be solved via a trait's associated type on T. And to supoort SIMD, the non-SIMD arrays must be in block shape. ```rust pub trait Block { ... // whatever is needed } pub struct ArrayBlock([T; N); impl Block for ArrayBlock {}; impl Block for u8x2 {} impl Block for u8x4 {} ... impl Block for i32x2 {} impl Block for i32x4 {} ... impl Block for f32x4 {} /// The type defines the storage type. /// `const R` for the row size pub trait StorageType { type Item; type Block: Block; } macro_rules! impl_simple_storage_r { ($t:ty, $r:expr) => { impl StorageType<$r> for $t { type Item = $t; type Block = ArrayBlock<$t, $r>; } }; } macro_rules! impl_simple_storage { ($t:ty) => { impl_simple_storage_r!($t, 1); impl_simple_storage_r!($t, 3); impl_simple_storage_r!($t, 5); impl_simple_storage_r!($t, 6); impl_simple_storage_r!($t, 7); ... }; } impl_simple_storage!(u8); impl_simple_storage!(u16); impl_simple_storage!(u32); ... impl_simple_storage!(i64); macro_rules! impl_simd_storage_r { ($t:ty, $r:expr) => { impl StorageType<$r> for $t { type Item = $t; type Block = Simd<[$t; $r]>; } }; } impl_simd_storage_r!(u8, 2); impl_simd_storage_r!(u8, 4); impl_simd_storage_r!(u8, 8); ... impl_simd_storage_r!(i128, 2); impl_simd_storage_r!(i128, 4); pub struct Vec3>(T::Block); pub struct Vec3>(T::Block); pub struct Vec4>(T::Block); ... pub struct Mat2>([Vec2; 2]); pub struct Mat3>([Vec3; 3]); pub struct Mat4>([Vec4; 4]); ... // impl Constructors impl> Add> for Vec4 where T::Block: Add { type Output = Self; fn add(self, rhs: Vec4) -> Self::Output { Vec4(self.0.add(rhs.0)) } } fn example() { let foo: Vec3 = Vec3([0,1,2]); // no simd let bar: Vec4 = Vec4::new(0,1,2,3); // simd } ``` This is without the nice direct value access (like `vec.x`). > In short, I think it is difficult to achieve both using explicit SIMD usage in combination with a generic interface and I think it would complicate the public interface greatly so it's not a direction I've ever pursued. Yes, it looks ugly and the code is less readable, because the generic type bound has to be dragged along in all implementations.
aloucks commented 2 years ago

I think the best compromise is to write a code generator, similar to what ash does to generate bindings from vk.xml. The resulting code may have duplication, but the source data to generate that code will not.

klangner commented 2 years ago

Out of curiosity. What about approach similar to num_complex crate. where we have generic struct but implementation is generic or specific to type:

pub struct Complex<T> {
    /// Real portion of the complex number
    pub re: T,
    /// Imaginary portion of the complex number
    pub im: T,
}

impl<T> Complex<T>  {
}

impl<T: Clone + Num> Complex<T> {
}

impl<T: Float> Complex<T> {
}

I agree that finding the source code now is a little bit difficult. VSC jumps to the macro for example. Could this work here?

bitshifter commented 2 years ago

@klangner glam could not support simd with that approach as it's a different type to T.

bitshifter commented 2 years ago

I took the templating approach using Tera. Merged to main in #294.