More idiomatic texture atlas builder

[s-puig]

Objective

• TextureAtlasBuilder has some non-idiomatic methods.

Solution

• Refactor non-idiomatic methods

---

Changelog

• Renamed TextureAtlasBuilder.finish() to build()
• TextureAtlasBuilder.with_texture() now consumes self and returns Self

Migration Guide

- texture_atlas_builder.add_texture(Some(id), texture);
+ texture_atlas_builder = texture_atlas_builder.add_texture(Some(id), texture);

- let (texture_atlas_layout, texture) = texture_atlas_builder.finish().unwrap();
+ let (texture_atlas_layout, texture) = texture_atlas_builder.build().unwrap();

[jgayfer]

👍

[brianreavis]

To be idiomatic and not awkward in cases like these, this should return &mut Self:

pub fn add_texture(&mut self, image_id: Option<AssetId<Image>>, texture: &'a Image) -> &mut Self

[s-puig]

I'm not so sure about that. It breaks the usual builder pattern (incidentally, this is why i noticed in the first place) i.e.

texture_atlas_builder = texture_atlas_builder.add_texture(Some(id), texture);

[brianreavis]

It is often more ergonomic (and more efficient) to take and return the builder as a mutable reference. The borrow checker makes this work naturally. This approach has the advantage that one can write code like

let mut fb = FooBuilder::new();
fb.a();
fb.b();
let f = fb.build();

as well as the FooBuilder::new().a().b().build() style.

From https://rust-unofficial.github.io/patterns/patterns/creational/builder.html#discussion

Imo, the only way that the builder = builder.method(...); pattern makes sense is if the method were named something less imperative like with_texture(...), but as @IceSentry mentioned, that style of use just makes things easier to mess up with no gain.

Is there a practical case this PR is attempting to address / improve?

[s-puig]

Is there a practical case this PR is attempting to address / improve?

I noticed when i was working on a project and wanted to quickly make an atlas like so:

let atlas = TextureAtlasBuilder::default().padding(padding.unwrap_or_default()).add_texture(None, &image).add_texture(None, &image2).finish();

Above doesn't work because add_texture returns ().

From https://rust-unofficial.github.io/patterns/patterns/creational/builder.html#discussion [..]

Mutable references builders are great if they are usually one-liners. They require every method to take/return a mutable reference or they become awkward to chain. The downside is that you now have to instantiate the owned type separately or the reference will outlive it:

// Allowed
let mut fb = FooBuilder::new();
fb.a();
fb.build();
// Compiler error
let mut fb = FooBuilder::new().a();
fb.build();

Anyways, i only care about it being consistent, so whatever you guys decide.

[IceSentry]

This doesn't seem like a win and just makes it much easier to mess up if you forget to do the assignment and we can't really warn users easily if they were using the previous code so it will break and be hard to debug.

[IceSentry]

I don't think there's anything wrong with using mutation here

[s-puig]

I think it is hard to mess up. It's a warn by default and they already have to use build() which will make it a compiler error that properly explains it:

error[E0382]: use of moved value: `texture_atlas_builder`
   --> examples/2d/texture_atlas.rs:225:43
    |
210 |     let mut texture_atlas_builder =
    |         ------------------------- move occurs because `texture_atlas_builder` has type `bevy::prelude::TextureAtlasBuilder<'_>`, which does not implement the `Copy` trait
211 |         TextureAtlasBuilder::default().padding(padding.unwrap_or_default());
212 |     for handle in folder.handles.iter() {
    |     ----------------------------------- inside of this loop
...
222 |         texture_atlas_builder.add_texture(Some(id), texture);
    |                               ------------------------------ `texture_atlas_builder` moved due to this method call, in previous iteration of loop
...
225 |     let (texture_atlas_layout, texture) = texture_atlas_builder.build().unwrap();
    |                                           ^^^^^^^^^^^^^^^^^^^^^ value used here after move
    |
note: `bevy::prelude::TextureAtlasBuilder::<'a>::add_texture` takes ownership of the receiver `self`, which moves `texture_atlas_builder`
   --> /home/foo/Programming/bevy/crates/bevy_sprite/src/texture_atlas_builder.rs:89:28
    |
89  |     pub fn add_texture(mut self, image_id: Option<AssetId<Image>>, texture: &'a Image) -> Self {

I don't mind changing it back though.

[pablo-lua]

Probably better to have this method take a mutable reference and return a mutable reference, this is a Pattern quite used for builders IIRC