Add methods to return the inner value for direction types

[Multirious]

Objective

Currently in order to retrieve the inner values from direction types is that you need to use the Deref trait or From/Into. Deref that is currently implemented is an anti-pattern that I believe should be less relied upon.
This pull-request add getters for retrieving the inner values for direction types.

Advantages of getters:

• Let rust-analyzer to list out available methods for users to understand better to on how to get the inner value. (This happens to me. I really don't know how to get the value until I look through the source code.)
• They are simple.
• Generally won't be ambiguous in most context. Traits such as From/Into will require fully qualified syntax from time to time.
• Unsurprising result.

Disadvantages of getters:

• More verbose

Advantages of deref polymorphism:

• You reduce boilerplate for getting the value and call inner methods by:

  let dir = Dir3::new(Vec3::UP).unwrap();
  // getting value
  let value = *dir;
  // instead of using getters
  let value = dir.vec3();
  
  // calling methods for the inner vector
  dir.xy();
  // instead of using getters
  dir.vec3().xy();

Disadvantages of deref polymorphism:

• When under more level of indirection, it will requires more dereferencing which will get ugly in some part:

  // getting value
  let value = **dir;
  // instead of using getters
  let value = dir.vec3();
  
  // calling methods for the inner vector
  dir.xy();
  // instead of using getters
  dir.vec3().xy();

More detail here.

Edit: Update information for From/Into trait.
Edit: Advantages and disadvantages.

Solution

Add vec2 method for Dir2.
Add vec3 method for Dir3.
Add vec3a method for Dir3A.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[Multirious]

I just realized that these methods is probably inaccessible unless specifying the type name because of the auto deref.
Edit: After testing in rust playground, it's probably fine. This magic is still a bit confusing lol.

[afonsolage]

I'm not sure this is the best approach. There is already From implemented to cast from/into (I just noticed there is no From<Dir2> for Vec2), also it's possible to Deref, like you said.

Maybe the documentation can be improved to describe how to get the inner value?

[NthTensor]

It does seem like From + Deref is more idiomatic. From<Dir2> should definitely be implemented on Vec2. I suppose I wouldn't hate it if we had both From and access methods, but I'd like to hear more from the author as to why the proposed solution is also necessary.

Can you elaborate on your rust-analyzer comment?

[Multirious]

Can you elaborate on your rust-analyzer comment?

Without the getter:

With the getter:

It's clear right away on how to access the Vec2.

why the proposed solution is also necessary.

If we look it the other way around, implementing deref-polymorphism is not necessary but just to reduce boilerplate and so does From/Into.
My take is Creating methods should be the first preferred solution. Traits should be second. Then deref-polymorphism as the last thing, like, the last.

From and Into is a trait. It's only useful when something expects these traits or used in the right context. Sometimes users may need to use fully qualify syntax, especially when doing operations after .into().
deref-polymorphism in its self has its own issues.
Getters resolves the above and avoid trait and deref problems that may came later on in development.

Basically, getters is just a lot more simpler that avoid issues. The code is easily parsed in mind when written, dir.vec3() for example instead of dir.into() or especially *dir that it looks just like you're dereferencing something.

[Multirious]

Maybe the documentation can be improved to describe how to get the inner value?

Absolutely! As it stands, deref polymorphism is useful, but as detailed in here, this is completely hidden from user. It should be clear for everyone that we're abusing deref to access the inner value.

I'm not very familiar with Bevy. From my outside perspective, deref polymorphism is known as an anti-pattern and I wouldn't implement it in my library, but I'm not against it if it your decision that you guys have made. I just hope that when this is implemented, it will be documented.

Also another example on when deref is less useful.

[afonsolage]

deref polymorphism is known as an anti-pattern

There is no polymorphism here, it's a new-type pattern. DirX isn't a subtype of VecX it is an extension/specialization. The Deref here is so that you can use any method in DirX as you would in VecX. The official docs of Deref even mention this kind of usage:

1. a value of the type transparently behaves like a value of the target type;
2. the implementation of the deref function is cheap; and
3. users of the type will not be surprised by any deref coercion behaviour.

In our case, DirX is just a VecX guaranteed to be of unit length.

let d = Dir3::X;
let xz = d.xz(); // d acting like a `Vec3`
let vec3 = *d; // If a Vec3 is needed, just deref copy it

I've opened a follow-up issue to add the missing From<Dir2> for Vec2

[Multirious]

There is no polymorphism here, it's a new-type pattern.

It's a new-type pattern that implements deref polymorphism pattern via the Deref trait.

let xz = d.xz(); // d acting like a `Vec3`

This is exactly deref polymorphism.

Deref polymorphism description:

Misuse the Deref trait to emulate inheritance between structs, and thus reuse methods.

The only use for implementing Deref for Dir* types is to reuse methods and so fits the anti-pattern description.

Let's looks in the deref polymorphism example:

use std::ops::Deref;

struct Foo {}

impl Foo {
    fn m(&self) {
        //..
    }
}

struct Bar {
    f: Foo,
}

impl Deref for Bar {
    type Target = Foo;
    fn deref(&self) -> &Foo {
        &self.f
    }
}

fn main() {
    let b = Bar { f: Foo {} };
    b.m();
}

Here's the simplified implementation of Vec3 and Dir3:

use std::ops::Deref;

struct Vec3 {
    x: f32,
    y: f32,
    z: f32,
}

impl Vec3 {
    fn xz(&self) {
    }
}

struct Dir3(Vec3);

impl Deref for Dir3 {
    type Target = Vec3;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

fn main() {
    let d = Dir3(Vec3 { x: 1., y: 0., z: 0. });
    let xz = d.xz();
}

This is deref polymorphism according to the example.

---

It's true that the official docs of Deref mention this kind of usage. But its usage outlined by the community is that it only should be implemented for types that "intuitively can be dereferenced" such as Box or Rc etc or have "valid meaning" to be dereferenced.
IMO, Dir3 doesn't really fits the community outlined usages and not even fits for its intended implementation from my understanding.

Intended uses from the unofficial docs:

The Deref trait is designed for the implementation of custom pointer types. The intention is that it will take a pointer-to-T to a T, not convert between different types.

---

In our case, DirX is just a VecX guaranteed to be of unit length.

If we described it like this, this might fits for the Deref trait. But this DirX description basically disallowed us to add more methods for DirX in the future which might be undesirable, such as methods that guaranteed to returned unit-length vectors without additional conversion from VecX to DirX.

---

DirX isn't a subtype of VecX

The Deref here is so that you can use any method in DirX as you would in VecX

These statements contradict themselves.
Trying to reuse method within VecX from DirX fits the description of subtyping from below.

Here below is the subtyping definition from a google search:

Subtyping is a method for substitution and code reuse

[NthTensor]

Interesting discussion here. Removing our use of Deref would be controversial, so let's call that out of scope for this PR. I have been swayed a bit, and would be interested in seeing follow up. This "anti-pattern" is certainly common, but that doesn't mean it's good use here.

Regarding the proposed changes, I think discovery is worth some redundancy. I support having access methods in addition to Deref+From. Personally I would just call all of these into_vec.

[alice-i-cecile]

I'm lukewarm in favor of this if it's called into_vec and the methods are const.

[Jondolf]

I don't have strong opinions on the topic, but for some prior art:

Nalgebra's Unit types implement Deref. They also have into_inner, which is equivalent to the proposed vec2/vec3/vec3a, but under a unified name.

Personally I consider our direction types to be Bevy's equivalents of Nalgebra's unit vectors and think of them primarily as vectors with extra restrictions. The direction types are newtypes of the vector types, and implementing Deref for newtypes is a very common pattern in Bevy (although perhaps for slightly different reasons in general), even though I guess in this case it could qualify under the mentioned deref polymorphism. Either way, imo the Deref impl makes at least some sense, but I'm fine with adding separate methods too.

For naming, Glam has methods like as_vec2 to convert a DVec2 to a Vec2, for example. But a unified name like into_vec or into_inner could be fine too (except into_vec implies turning it into a Vec<f32>, so I wouldn't go for that). The names vec2/vec3/vec3a don't seem to be consisent with any existing math types though.

[Multirious]

I like to follow the naming guidelines generally.

I decided to use these names for the initial commit because:

• It's the most straightforward.
• Type is in the name. It's easily parsed without an LSP (e.g., reading a commit on the web).

I agree that using only vec could be confusing.

I will make the method const.
According to the naming guidelines, into_ prefix should be use when the methods consumes self. Should I change to that?

[NthTensor]

Whoops, I missed that this takes borrowed self and does an implicit copy. to_vec2/3/3a seems like the way to go then, considering it would be best to avoid ambiguity with Vec. Do I have that naming convention right?

[Jondolf]

I guess ownership-wise to_ would be the correct prefix according to the guidelines, but they also say this:

Conversions prefixed to_, on the other hand, typically stay at the same level of abstraction but do some work to change from one representation to another.

and to_ is listed as "expensive". I mainly associate to_ with methods that do some actual computations or transformations to change the underlying data to a different format, like how to_radians converts degrees to radians or how to_lowercase iterates through a str to construct a lowercase String. to_vec2/3/3a would be basically free and does not modify the underlying data in any way, and instead just gets the inner value stored in the direction type. So I'd personally prefer as_vec2/3/3a, if not for following guidelines then at least for consistency with Glam.

Edit: The guidelines also don't have our "borrowed -> owned" case for a Copy type, so don't think that necessary applies for to_.

[Jondolf]

This comment format is quite strange. The verb should generally be in the present tense, not present continuous.

Suggested change

	/// Returning the inner [`Vec2`]
	/// Returns the inner [`Vec2`]

[Multirious]

My bad. English is not my first language.

[Multirious]

https://stackoverflow.com/a/73861738

A quicker way to think about it:

• if it consumes the data, use into_*
• if it returns another "view" of the data, use as_*
• otherwise use to_*

These rules are pretty much followed by the standard library and most ecosystem crates. As always though, these are more guidelines than actual rules. Conventions are helpful but don't need to have strict adherence.

I'd use to_

[Jondolf]

I'll suggest once more that if we want a something_vec2/3/3a prefix, then I think we really should go for as_* for the sake of consistency with Glam regardless of what may or may not be technically more correct according to the guidance. It's confusing to have as_dvec2 but to also have to_vec2. With Deref implemented, these are both exposed, and even if they weren't, I'd expect the naming to be consistent between Vec2 and Dir2.

[NthTensor]

I can get onboard with as. The name standard is a little ambiguous between as and to here.

[NthTensor]

Sorry this ended up with so much bike shedding.

[Multirious]

My bad if I may have been a bit unclear on the explanation of things. This is my first actual PR attempt after all 😃. Just finnally gets to contribute to something I like.

The summary for adding getters is that it would've been more ergonomic long-term because of its simple usage.