Refactor with_ and add new set_ method for vector types

[ganhaque]

Objective:

The current behavior of the with_ methods for vector types is using mut self type, which can be confusing since it relies on the Copy trait for implicit copy.

There are also no convenient set_ method for method chaining pattern.

Solution:

• Refactor the with_ method of vector types to use self and create an explicit copy instead.
• Add a new set_ method to allow for method chaining when mutating vector types.

[aloucks]

Usually I've seen and expect with_ methods to take the object by value and return itself. Where as set_ methods take a &mut self receiver. The convention in this proposed change does not make sense to me.

These are all copy types anyway. The existing with_ methods copy the item and return a modified copy. I'm not sure there is any value in adding the set_ variants.

[ganhaque]

I don't think the current with_ method copy the item and return a modified copy?

Current:

/// Creates a 3D vector from `self` with the given value of `x`.
#[inline]
#[must_use]
pub fn with_x(mut self, x: f32) -> Self {
    self.x = x;
    self
}

New:

/// Returns a new version of this 3D vector with the given `x` value.
#[inline]
#[must_use]
pub fn with_x(&self, x: f32) -> Self {
    Self { x, ..*self }
}

---

I think both methods are pretty useful. The new set_ (the old with_ renamed) method provide a better way to initialize a vector type without the overhead of creating a new instance. The new with_ method is for cloning without mutating the original.

// set_ method is better for initializing
let foo = Vec3::ZERO
    .set_x(1.0);

// with_ method is better for cloning
let bar = foo.with_y(1.0);
let x = foo.with_z(1.0);

---

&mut self for the set_ method does make more sense than mut self. I didn't catch that when renaming the old with_ method. I fixed that in the latest commit.

[aloucks]

I don't think the current with_ method copy the item and return a modified copy?

Yes, it creates a copy. That's what mut self means as the receiver. It consumes the old one. If the type were not Copy the old one would no longer be available. A copy is NOT created when you use &mut self (a reference type).

#[test]
fn vec3_copy() {
    let a = Vec3::splat(1.0);
    let b = a.with_x(2.0);
    assert_eq!(1.0, a.x);
    assert_eq!(2.0, b.x);
}

[ganhaque]

Ah, sorry for the misunderstanding, I though you meant create a copy as in creating a new instance with the same value (with the modified field x, y, z, etc.).

A copy is NOT created when you use &mut self (a reference type).

Yeah, that is for the set_ method which modify the original in place without consuming it. The new with_ takes an immutable reference &self and returns a new instance with the modified value, while ensuring the original instance remains unchanged and available.

[aloucks]

Your desired behavior for the way it works is how it already works. Look at the example unit test that I posted. The current with_ methods do not modify the original.

[ganhaque]

Ah, I didn't understand how it work originally, thank you for explaining it. If both methods work, I think it would be better to explicitly create a copy instead of implicitly via the Copy trait? It would also be consistent with the current swizzle with_ methods.

[aloucks]

I don't understand why you want to create set_ methods for public fields and the with_ methods already work as expected. What problem are you trying to solve?

[ganhaque]

The set_ methods can be used for like

let mut foo = Vec3::ZERO;

// which allows for method chaining pattern
let modified_foo = foo
    .set_x(2.0)
    .set_y(3.0);
    
// instead of multiple statements
foo.x = 2.0;
foo.y = 3.0;
let modified_foo = foo;

It can also be used along with its swizzle variant (set_xy, etc.) if/when those are added. I just think it's a nice method to have for the method chaining coding style. I don't see the harm in adding it?

[aloucks]

From your opening statement:

Objective:

The current behavior of the with_* methods for vector types is confusing since the name suggests creating and returning a modified copy of the vector without modifying the original.

The suggested behavior is the actual behavior. It returns a modified copy without modifying the original.

However, these methods currently mutate the original vector in place.

This is false. It does not modify the original vector in place. That is the cause of your confusion.

When the receiver is mut self, it takes a copy of the receiver. The original item is not modified. This is idiomatic Rust code.

pub fn with_x(mut self, x: f32) -> Self {
    self.x = x;
    self
}

Using your example above:

foo does not need to be mutable because it's not being mutated.

#[test]
fn vec3_copy() {
    let foo = glam::Vec3::ZERO;

    let modified_foo = foo
        .with_x(2.0)
        .with_y(3.0);

    // foo has not changed
    assert_eq!(0.0, foo.x);
    assert_eq!(0.0, foo.y);
    assert_eq!(0.0, foo.z);

    // modified_foo has the new values specified by the `with_x` and `with_y` methods and the
    // original value for z
    assert_eq!(2.0, modified_foo.x);
    assert_eq!(3.0, modified_foo.y);
    assert_eq!(0.0, modified_foo.z);
}

[ganhaque]

Sorry, I didn't update the original description. The with_ method did work as intended. The change was just to make it create an explicit copy instead of implicitly via the Copy trait.

I was talking about the set_ method instead, which is for mutating the vector. The modified_foo assignment was just to showcase mutating and assignment in a single statement.

// modify `foo` with new values
foo
    .set_x(2.0)
    .set_y(3.0);
   
// create a copy of `foo` with new values
foo
    .with_x(2.0)
    .with_y(3.0);

[aloucks]

The fields are public. You can just do this:

foo.x = 2.0;
foo.y = 3.0;

[ganhaque]

#597 (comment)