Flat_map a map, the outer closure prohibits the inner closure from moving, how to fix?

help
1

The title may be a bit confusing, so please check the code:

pub fn generate_random_map(mut commands: Commands) {
    let mut rng = XorShiftRng::seed_from_u64(1u64);
    commands.spawn_batch(
        (-64i32..64i32).flat_map(|x| {
            (-64i32..64i32).map(move |y| SpriteBundle {
                sprite: Sprite {
                    color: if rng.gen::<bool>() {      //The rng here
                        Color::rgb(0.0, 1.0, 0.0)
                    } else {
                        Color::rgb(0.5, 0.5, 0.0)
                    },
                    custom_size: Some(const_vec2!([50.0, 50.0])),
                    ..default()
                },
                transform: Transform::from_xyz(x as f32 * 50.0, y as f32 * 50.0, 0.0),
                ..default()
            })
        })
    );
}

The compiler complains:

cannot move out of `rng`, a captured variable in an `FnMut` closure

I also tried collecting the Sprites into a vector beforehand, which didn't work either:

let pre_generated_sprites = (-64i32..64i32)
    .flat_map(|x| {
        (-64i32..64i32).map(move |y| SpriteBundle {
            sprite: Sprite {
                color: if rng.gen::<bool>() {      //the rng here
                    Color::rgb(0.0, 1.0, 0.0)
                } else {
                    Color::rgb(0.5, 0.5, 0.0)
                },
                custom_size: Some(const_vec2!([50.0, 50.0])),
                ..default()
            },
            transform: Transform::from_xyz(x as f32 * 50.0, y as f32 * 50.0, 0.0),
            ..default()
        })
    })
    .collect::<Vec<_>>();

I'm new to Rust, and this code seems legit in languages like Lisp. Do I have to use a nested for?
I made it work like this:

let mut rng = XorShiftRng::seed_from_u64(1u64);
let mut v = Vec::new();
for x in -64..64 {
    for y in -64..64 {
        v.push(SpriteBundle {
            sprite: Sprite {
                color: if rng.gen::<bool>() {
                    Color::rgb(0.0, 1.0, 0.0)
                } else {
                    Color::rgb(0.5, 0.5, 0.0)
                },
                custom_size: Some(const_vec2!([50.0, 50.0])),
                ..default()
            },
            transform: Transform::from_xyz(x as f32 * 50.0, y as f32 * 50.0, 0.0),
            ..default()
        })
    }
}
commands.spawn_batch(v.into_iter());

But that way flat_map is kind of pointless, and using a Vec introduces copies, so I'm guessing I did something wrong. How should I structure the code to make it work, elegantly?

2

The issue might be the move |y| closure. Recall that a move closure captures its variables by value, and not by reference. So in every iteration of the outer closure, the inner closure attempts to take ownership of rng by moving it. This causes the compiler error: since XorShiftRng is not Copy, the inner closure cannot take ownership of it from the outer closure every iteration.

Do you really need the move on the inner closure? What errors occur if you remove it?

3

Try if adding

let rng = &mut rng;

before the call to map helps.

Edit: Actually, this might run into problems with multiple conflicting mutable references.

4

The move wasn't there initially, but added because of a compiler hint. If I remove it, the compiler says (sorry about the long errors but I really don't understand the messages and therefore can't abstract the important parts):

error: captured variable cannot escape `FnMut` closure body
  --> src/map.rs:30:17
   |
27 |           let mut rng = XorShiftRng::seed_from_u64(1u64);
   |               ------- variable defined here
28 |           commands.spawn_batch(
29 |               (-64i32..64i32).flat_map(|x| {
   |                                          - inferred to be a `FnMut` closure
30 | /                 (-64i32..64i32).map(|y| SpriteBundle {
31 | |                     sprite: Sprite {
32 | |                         color: if rng.gen::<bool>() {      //The rng here
   | |                                   --- variable captured here
33 | |                             Color::rgb(0.0, 1.0, 0.0)
...  |
41 | |                     ..default()
42 | |                 })
   | |__________________^ returns a reference to a captured variable which escapes the closure body
   |
   = note: `FnMut` closures only have access to their captured variables while they are executing...
   = note: ...therefore, they cannot allow references to captured variables to escape

error[E0373]: closure may outlive the current function, but it borrows `x`, which is owned by the current function
  --> src/map.rs:30:37
   |
30 |                 (-64i32..64i32).map(|y| SpriteBundle {
   |                                     ^^^ may outlive borrowed value `x`
...
40 |                     transform: Transform::from_xyz(x as f32 * 50.0, y as f32 * 50.0, 0.0),
   |                                                    - `x` is borrowed here
   |
note: closure is returned here
  --> src/map.rs:30:17
   |
30 | /                 (-64i32..64i32).map(|y| SpriteBundle {
31 | |                     sprite: Sprite {
32 | |                         color: if rng.gen::<bool>() {      //The rng here
33 | |                             Color::rgb(0.0, 1.0, 0.0)
...  |
41 | |                     ..default()
42 | |                 })
   | |__________________^
help: to force the closure to take ownership of `x` (and any other referenced variables), use the `move` keyword

The "help" about

   |
30 |                 (-64i32..64i32).map(move |y| SpriteBundle {
   |                                     ++++

error[E0373]: closure may outlive the current function, but it borrows `rng`, which is owned by the current function
  --> src/map.rs:29:38
   |
29 |             (-64i32..64i32).flat_map(|x| {
   |                                      ^^^ may outlive borrowed value `rng`
...
32 |                         color: if rng.gen::<bool>() {      //The rng here
   |                                   --- `rng` is borrowed here
   |
note: function requires argument type to outlive `'static`
  --> src/map.rs:29:13
   |
29 | /             (-64i32..64i32).flat_map(|x| {
30 | |                 (-64i32..64i32).map(|y| SpriteBundle {
31 | |                     sprite: Sprite {
32 | |                         color: if rng.gen::<bool>() {      //The rng here
...  |
42 | |                 })
43 | |             })
   | |______________^
help: to force the closure to take ownership of `rng` (and any other referenced variables), use the `move` keyword
   |
29 |             (-64i32..64i32).flat_map(move |x| {
   |                                      ++++

For more information about this error, try `rustc --explain E0373`.
5

The compiler still gives errors about captured variable cannot escape FnMut closure body, and rng doesn't live long enough (perhaps due to spawn_batch uses it in another thread. Collecting the iterator before spawn_batch it solve this error but captured variable cannot escape FnMut closure body persists.)

6

This is certainly a somewhat nontrivial case you're running into here, the existing API seems somewhat limiting, perhaps one day with a good solution on "lending iterators" this all becomes a non-problem.

In the meantime, provided you want to avoid the creation of a Vec, you could either use an approach of splitting the Rng, so that each sub-iterator produced in the flat_map can get its own owned random number generator, or you could start wrapping the Rng into Arc<Mutex<..>> to share it.

7

Thanks for the solutions! After trying them I think I might stick to for for the time being, since splitting the Rng can introduce too much complexity when using Monte Carlo method, and copying Vec outperforms Arc<Mutex<..>> even when it's larger then the L3 cache on my system. Still, thanks for your kind help and trouble.

8

Another approach, you could fill a [bool; 128] with random data in each step of the outer loop and pass that instead of the rng into the inner call to map.

(Well, instead of "passing" it, you'll probably just want to zip it with the -64i32..64i32)

Could also save some space by using something like [u64; 2] (or a single u128, or [u8; 16], or [u32; 4]…) instead and turning it into an iterator of bools with something like .into_iter().flat_map(...) where ... is some way of turning a u64 into an iterator of 64 bools. Arguably that's a bit less ergonomic, and you might need to write such an iterator, but IMO it's an interesting candidate in case you're interested to compare different approaches for performance.

9

Thanks for the enlightment! I actually tried a similar route earlier and wrote an u8 to bool iterator because RngCore::fill_bytes doesn't work with [bool].

My earlier attempt failed because I created the [u8;2048] before the maps and errors like the ones above occured. I tried your method and it worked.

In terms of performance, after a bit of profiling the bottleneck seems to be the spawn operations (it's an ECS system, where inserting potentially millions of map tiles in the future can be relatively slow, and that's why I use spawn_batch instead of many spawn's in nested loops in the first place).

I might just give up putting them in the ECS so as to avoid the insertions and copies, in addition to access of the tiles directly by O(1) indexing.

In a tilemap project bevy_ecs_tilemap chunks of tiles are used. Having tried and failed reading its source code, which TBH isn't very well documented, I think I still need to learn some Rust before really using it to make a game.

10

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.