Reduce code duplication between Transform and GlobalTransform

[olivia-fl]

What problem does this solve or what need does it fill?

Currently, Transform and GlobalTransform have effectively the same interface, just as different components. They are implemented as completely different types. Code is duplicated between the two and there is no simple way to write functions that are generic over both.

What solution would you like?

Make the definition of GlobalTransform:

#[repr(transparent)]
struct GlobalTransform(pub Transform);

What alternative(s) have you considered?

Creating a trait implemented by both transform types. I don't think there's any real reason for this because the actual implementation details are identical, as far as I know. A trait would make sense if the implementation differed, but since they don't I believe composition makes more sense.

Another thing we could add is a Deref<Target = Transform> impl for GlobalTransform. This would get rid of some global_transform.0 calls, but I think it's not worth it. Also, even though this is a common pattern in the rust ecosystem, it goes directly against the standard library's documentation for the intended usage of the Deref trait:

Implementing Deref for smart pointers makes accessing the data behind them convenient, which is why they implement Deref. On the other hand, the rules regarding Deref and DerefMut were designed specifically to accommodate smart pointers. Because of this, Deref should only be implemented for smart pointers to avoid confusion.

Additional context

This would be a breaking change, and would require a small modification to any code that uses global transforms.

The #[repr(transparent)] isn't strictly necessary, but there's no real reason not to add it.

I'm happy to write a PR for this if people agree that it's a good idea.

[cart]

The code duplication was an explicit decision to make GlobalTransform more ergonomic to work with. GlobalTransforms are a core enough component that it was worth it to me to duplicate the code: #374 (comment).

I honestly hate the "rust newtype" pattern as a means of reusing implementations in public apis. The following api is actively user-hostile imo:

global_transform.0.translation.x += 1.0;

Rust should give us better tools for this case. Something like the "type alias" syntax, but for defining actual new types instead of new names for the same type. Until that happens, I won't make compromises for high-usage apis when we can eat the complexity internally.

That being said, I am willing to consider alternatives:

Traits

Removes code duplication. Allows users to write code on both transform types. Transform method docs are less clear + discoverable.

struct Transform { translation: Vec3, rotation: Quat, scale: Vec3 }
struct GlobalTransform { translation: Vec3, rotation: Quat, scale: Vec3 }

// Similarity is the name for a [translation, rotation, scale] transform representation
trait Similarity {
  fn rotation(&self) -> &Quat;
  fn rotation_mut(&mut self) -> &mut Vec3;
  /* other accessors here */
}

trait TransformOperations {
  fn rotate(&mut self, rotation: Quat);
  /* other methods here */
}

impl<T: Similarity> TransformOperations for T  {
  fn rotate(&mut self, rotation: Quat) {
    *self.rotation_mut() *= rotation;
  }
  /* other impls here */
} 

Macros

Cuts down on code duplication at the cost of "macro complexity". Doesn't solve the "using transform types interchangeably" problem.

struct Transform { translation: Vec3, rotation: Quat, scale: Vec3 }
struct GlobalTransform { translation: Vec3, rotation: Quat, scale: Vec3 }

impl_transform_methods!(Transform);
impl_transform_methods!(GlobalTransform);

[olivia-fl]

I'm much more concerned with the user code duplication through not being able to use transform type interchangeably than I am with internal code duplication, so we should focus on finding an acceptable way to handle that.

Rust should give us better tools for this case. Something like the "type alias" syntax, but for defining actual new types instead of new names for the same type. Until that happens, I won't make compromises for high-usage apis when we can eat the complexity internally.

(Ab)using Deref is pretty much the standard way to do this right now. I think I have opposite preferences to you about this area of API design. Deref-coercion makes me pretty uncomfortable, particularly when used for things that aren't pointers. It's worth noting that any system of automatically converting between GlobalTransform and Transform is probably a mistake, because these types have very different semantics in the context of the ECS. I could see it being easy to introduce very difficult to find bugs where a user thinks they are operating on the global transform component, but have actually coerced into the transform component. It needs to be possible to write code that is deliberately generic over Transform and GlobalTransform, but impossible to write code that is accidentally generic.

With that in mind, I think there are only two remaining options: traits and impl From<GlobalTransform> for Transform and vice versa. Out of these I think I would prefer a TransformOperations trait, for clarity, but I think either solution works.

[cart]

Yeah I wouldn't want to use Deref in this context either (however I would do that over global_transform.0.translation).

It seems like traits are the way to go here.

[cart]

There are a few open questions like "how do we handle transform multiplication properly" and other things in that vein. We will probably want manual impls for each type to ensure the context is preserved.

Ex: GlobalTransform * Transform = GlobalTransform

[olivia-fl]

It's not actually clear to me what the best behavior for multiplication would be. I think a reasonable choice would be for the output type to match the LHS, for symmetry with MulAssign. Expressing this via a bound on the TransformOperation trait isn't possible with rust's type system (maybe HKT would help, I'm not actually sure off the top of my head), so I think it makes sense to implement it separately for each concrete type. We can still have TransformOperation: Mul<Self, Output = Self> though.

[engiwengi]

With #![feature(const_generics)] you could do:

enum TransformType {
    Global,
    Local,
}

struct Transform<const TYPE: TransformType> {
    translation: Vec3,
    rotation: Quat,
    scale: Vec3,
}

impl<const TYPE: TransformType> Transform<TYPE> {
    /* implement methods for both types of transforms */
}

Which I think is quite nice but might be a long way off.

[mockersf]

With #![feature(const_generics)] you could do:

Would this be closed to it with current Rust?

trait TransformType {}

struct Global;
impl TransformType for Global {}

struct Local;
impl TransformType for Local {}

struct Transform<T: TransformType> {
    translation: Vec3,
    rotation: Quat,
    scale: Vec3,
    ty: std::marker::PhantomData<T>,
}

[engiwengi]

With #![feature(const_generics)] you could do:

Would this be closed to it with current Rust?

Sure but this way is open ended, i.e consumers could make more 'types of transforms'.

[olivia-fl]

Users could still define their own types that impl TransformType with mockersf's proposal. I actually really like this solution! We can even have type aliases to keep backwards compatibility:

type GlobalTransform = Transform<Global>;

struct Transform<T: TransformType = Local> {
    ...
}

[bjorn3]

Generic defaults only work in limited cases: Rust-GCC/gccrs#307 (comment)

struct Box<T, A: Default = ()>(T, A);

impl<T, A: Default> Box<T, A> {
    fn new(x: T) -> Self {
        Box(x, Default::default())
    }
}

fn main() {
    Box::new(1);
}

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
4  |     fn new(x: T) -> Self {
   |     -------------------- required by `Box::<T, A>::new`
...
10 |     Box::new(1);
   |     ^^^^^^^^ cannot infer type for type parameter `A`
   |
   = note: cannot satisfy `_: Default`

error: aborting due to previous error

[olivia-fl]

Oh, that's unfortunate. An alternative would be to rename Transform<T> to something else, and make type Transform = TransformGeneric<Local>;. I don't have any good ideas for what to call it though.

[engiwengi]

Not sure if this is actually a good idea but this also popped into my head from the coordinate system discussion.

Struct Local;

Trait TransformType {}

Trait RightHanded {}

Trait YUp {}

Impl TransformType for Local {}

Impl RightHanded for Local {}

Impl YUp for Local {}

Impl<T> Transform<T>
where 
    T: TransformType + RightHanded + YUp 
{
    fn left(&self) -> f32 {
        -self.translation.x
    }
    /* other right handed, y up functions */
}

This is probably just complicating the traits approach though 🤣

[olivia-fl]

I'm gonna start working on an implementation using the type parameter approach for now. I think having fine-grained traits like this is not necessary at this point.

[cart]

Cool cool. I'm curious to see if in practice we hit the "default weirdness" @bjorn3 called out. Thats my only major concern.

[olivia-fl]

I was planning to just make the generic one GenericTransform and have type Transform = GenericTransform<Local> to avoid the issue entirely. We could also rename Transform to LocalTransform, but I don't really like that.

[cart]

Yeah I'd prefer to leave Transform naming as it is. Type aliases seem like a reasonable fix, although they might complicate the doc situation a bit. Lets roll with the alias and see what the docs look like. Its probably fine.

[james7132]

With #4379, the internal representation for both types is now very different. Is this issue still relevant?