Trying to avoid for/while loop

code review
1

I am attempting to build a Chip-8 emulator in rust through this tutorial How to Create Your Very Own Chip-8 Emulator mainly as a practice. I am using rust-sfml for the graphical output here.

Supposedly I have a struct

struct Screen {
    cols: usize,
    rows: usize,
    scale: usize,
    pixels: Vec<bool>,
    window: RenderWindow,
}

and I am implementing a method, and this version works fine


    fn render(&mut self) {
        self.window.clear(Color::rgb(255, 255, 255));

        for (idx, &value) in self.pixels.iter().enumerate() {
            if value {
                let mut rect =
                    RectangleShape::with_size(Vector2f::new(self.scale as f32, self.scale as f32));
                rect.set_fill_color(Color::rgb(0, 0, 0));
                rect.set_position(Vector2f::new(
                    ((idx % self.cols) * self.scale) as f32,
                    (idx.div_euclid(self.cols) * self.scale) as f32,
                ));

                self.window.draw(&rect);
            }
        }
    }

however, when I turn it into the following form

    fn render(&mut self) {
        self.window.clear(Color::rgb(255, 255, 255));

        self.pixels
            .iter()
            .enumerate()
            .filter(|(_, &value)| value)
            .for_each(|(idx, _)| {
                let mut rect =
                    RectangleShape::with_size(Vector2f::new(self.scale as f32, self.scale as f32));
                rect.set_fill_color(Color::rgb(0, 0, 0));
                rect.set_position(Vector2f::new(
                    ((idx % self.cols) * self.scale) as f32,
                    (idx.div_euclid(self.cols) * self.scale) as f32,
                ));

                self.window.draw(&rect);
            });
    }

I get ownership problem (if I understand correctly, the error is about mutating self.pixels while referring and/or mutating other fields in self within the .for_each method call), a quick workaround is to add a .clone() to self.pixel and things would work properly, but it is probably not the best way to fix the problem. Then after some experiments, I came out with

    fn render(&mut self) {
        self.window.clear(Color::rgb(255, 255, 255));

        let scale = &self.scale;
        let cols = &self.cols;

        let window = &mut self.window;

        self.pixels
            .clone()
            .iter()
            .enumerate()
            .filter(|(_, &value)| value)
            .for_each(|(idx, _)| {
                let mut rect =
                    RectangleShape::with_size(Vector2f::new(*scale as f32, *scale as f32));
                rect.set_fill_color(Color::rgb(0, 0, 0));
                rect.set_position(Vector2f::new(
                    ((idx % cols) * scale) as f32,
                    (idx.div_euclid(*cols) * scale) as f32,
                ));

                window.draw(&rect);
            });
    }

This code above works, but still feels unnecessary with the extra let statements.

It currently looks like this.

    fn render(&mut self) {
        self.window.clear(Color::rgb(255, 255, 255));

        self.pixels
            .iter()
            .enumerate()
            .filter(|(_, &value)| value)
            .map(|(idx, _)| {
                let mut rect =
                    RectangleShape::with_size(Vector2f::new(self.scale as f32, self.scale as f32));
                rect.set_fill_color(Color::rgb(0, 0, 0));
                rect.set_position(Vector2f::new(
                    ((idx % self.cols) * self.scale) as f32,
                    (idx.div_euclid(self.cols) * self.scale) as f32,
                ));

                rect
            })
            .collect::<Vec<RectangleShape>>()
            .iter()
            .for_each(|rect| {
                self.window.draw(rect);
            });
    }

Just out of curiosity, if I want to avoid writing for .. in ../while loop, what is the better way to do this (assuming if there is)?

2

You can also write the let statements a bit like this:

let Screen { scale, cols, window, .. } = self;

There isn't really any better solution, unfortunately. The for loop is probably the cleanest, tbh.

4 Likes
3

Yes. I vote for the loop. Far simpler to comprehend.

4 Likes
4

Indeed. You should know that the plan is that for the incoming Rust edition (2021), the behavior of closure captures will change, so that your closure will not capture the whole self: &mut Self (which overlaps with self.pixels being borrowed outside the closure), and, instead, it will capture (&self.scale, &self.cols, &mut self.window), thus not overlapping :slightly_smiling_face:

4 Likes
5

Thanks for the replies, the reason I started the topic was because I hit similar kind of error in the past, and wanted to know if there's a better solution to this

6

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.