Game of Life in Rust-lang

Question

I decided to implement Conways Game of Life for my first project using Rust. I also haven't had much code review ever, and have been looking for a good critique of my code so I can improve my skills. Here is the code:

extern crate glutin_window;
extern crate graphics;
extern crate piston;
extern crate piston_window;

use glutin_window::GlutinWindow as Window;

use graphics::*;
use opengl_graphics::{GlGraphics, OpenGL};
use piston::{
    event_loop::*, RenderArgs, RenderEvent, UpdateArgs, UpdateEvent,
};
use piston::window::WindowSettings;
use rand::prelude::*; //Generate random life and death

const WINDOW_WIDTH: f64 = 400.0;
const WINDOW_HEIGHT: f64 = 400.0;

const BACKGROUND_COLOR: [f32; 4] = [0.5, 0.5, 0.5, 1.0];
const CELL_ALIVE: [f32; 4] = [0.0, 1.0, 0.0, 1.0];
const CELL_DEAD: [f32; 4] = [0.25, 0.25, 0.25, 1.0];

const CELL_SIZE: i32 = 1;

#[derive(Clone)]
struct Cell {
    alive: bool,
}

impl Cell {
    fn set(&mut self, alive: bool) {
        self.alive = alive;
    }
}

struct CellWorld {
    rows: i32,
    columns: i32,
    map: Box<Vec<Vec<Cell>>>,
}

impl CellWorld {
    fn new(window_height: f64, window_width: f64) -> CellWorld {
        let n_rows = (window_height as i32) / CELL_SIZE;
        let n_cols = (window_width as i32) / CELL_SIZE;

        CellWorld {
            rows: n_rows,
            columns: n_cols,
            map: Box::new(Vec::new()),
        }
    }
}

struct PistonApp {
    gl_context: GlGraphics,
    world: CellWorld,
}

impl PistonApp {

    fn init(&mut self) {
        let mut rand_life = thread_rng();

        for row in 0..self.world.rows {
            let rand_num: i32 = rand_life.gen();
            let mut cell_row = vec![Cell { alive: row / rand_num == rand_life.gen() }; self.world.columns as usize];

            for cell in 0..self.world.columns {
                cell_row[cell as usize].set(cell % 2 == 0);
            }

            self.world.map.push(cell_row);
        }
    }

    fn render(&mut self, render_args: &RenderArgs) {
        let cell_map = &self.world.map;
        let n_rows = self.world.rows;
        let n_columns = self.world.columns;

        self.gl_context.draw(render_args.viewport(), |c, gl| {
            clear(BACKGROUND_COLOR, gl);

            for row in 0..n_rows {
                let cell_row = &cell_map[row as usize];
                let row_offset = row * CELL_SIZE;

                for col in 0..n_columns {
                    let cell = &cell_row[col as usize];
                    let col_offset = col * CELL_SIZE;

                    let cell_repr = rectangle::square(0.0, 0.0, CELL_SIZE as f64);
                    let trnsfrm = c.transform.trans(col_offset as f64, row_offset as f64);

                    let cell_color = match cell.alive {
                        true => CELL_ALIVE,
                        false => CELL_DEAD
                    };

                    rectangle(cell_color, cell_repr, trnsfrm, gl);
                }
            }
        });
    }

    fn update(&mut self, _update_args: &UpdateArgs) {
        self.simulate();
    }

    fn simulate(&mut self) {
        let n_rows = self.world.rows;
        let n_cols = self.world.columns;

        for row in 0..n_rows {
            for col in 0..n_cols {
                self.update_cell(row as usize, col as usize);
            }
        }
    }

    fn update_cell(&mut self, row: usize, col: usize) {
        let world = &mut self.world;
        let world_map = &mut world.map;
        let mut alive_neighbors = 0;

        let has_left_neighbors = if col == 0 { false } else { col - 1 > 0 };
        let left_col = if has_left_neighbors { col - 1 } else { 0 };
        let has_right_neighbors = col + 1 < world.columns as usize;
        let right_col = if has_right_neighbors { col + 1 } else { 0 };
        let has_top_neighbors = if row == 0 { false } else { row - 1 > 0 };
        let top_row = if has_top_neighbors { row - 1 } else { 0 };
        let has_btm_neighbors = row + 1 < world.rows as usize;
        let btm_row = if has_btm_neighbors { row + 1 } else { 0 };

        if has_left_neighbors {
            if world_map[row][left_col].alive {
                alive_neighbors += 1;
            }

            if has_top_neighbors && world_map[top_row][left_col].alive {
                alive_neighbors += 1;
            }

            if has_btm_neighbors && world_map[btm_row][left_col].alive {
                alive_neighbors += 1;
            }
        }

        if has_right_neighbors {
            if world_map[row][right_col].alive {
                alive_neighbors += 1;
            }

            if has_top_neighbors && world_map[top_row][right_col].alive {
                alive_neighbors += 1;
            }

            if has_btm_neighbors && world_map[btm_row][right_col].alive {
                alive_neighbors += 1;
            }
        }

        if has_top_neighbors {
            if world_map[top_row][col].alive {
                alive_neighbors += 1;
            }
        }

        if has_btm_neighbors {
            if world_map[btm_row][col].alive {
                alive_neighbors += 1;
            }
        }

        if alive_neighbors < 2 || alive_neighbors > 3 {
            world_map[row][col].set(false);
        }

        if alive_neighbors == 2 || alive_neighbors == 3 || world_map[row][col].alive == false && alive_neighbors == 3 {
            world_map[row][col].set(true);
        }
    }
}

fn main() {
    let opengl_version = OpenGL::V3_2;

    let mut window: Window = WindowSettings::new(
        "Cellular Automata!",
        piston_window::Size::from([WINDOW_WIDTH, WINDOW_HEIGHT]),
    ).graphics_api(opengl_version).exit_on_esc(true)
        .resizable(false).fullscreen(true).build().unwrap();

    let mut cellular_automata_app = PistonApp {
        gl_context: GlGraphics::new(opengl_version),
        world: CellWorld::new(WINDOW_HEIGHT, WINDOW_WIDTH),
    };

    cellular_automata_app.init();

    let mut window_events = Events::new(EventSettings::new());

    while let Some(event) = window_events.next(&mut window) {

        if let Some(render_args) = event.render_args() {
            cellular_automata_app.render(&render_args);
        }

        if let Some(update_args) = event.update_args() {
            cellular_automata_app.update(&update_args);
        }

        std::thread::sleep(std::time::Duration::from_millis(10));
    }
}

Let me know what you think about my code!

Answer 1

extern crate glutin_window;
extern crate graphics;
extern crate piston;
extern crate piston_window;

The new edition of rust, 2018, does not require extern crate statements. Furthermore, you don't have extern crate statements for rand or opengl_graphics suggesting that you are already using Rust 2018, and you just have these extra extern crates for no reason.

const BACKGROUND_COLOR: [f32; 4] = [0.5, 0.5, 0.5, 1.0];
const CELL_ALIVE: [f32; 4] = [0.0, 1.0, 0.0, 1.0];
const CELL_DEAD: [f32; 4] = [0.25, 0.25, 0.25, 1.0];

I suggest that you define a type alias for colors:

type Color = [f32; 4];

const BACKGROUND_COLOR: Color = [0.5, 0.5, 0.5, 1.0];
const CELL_ALIVE: Color = [0.0, 1.0, 0.0, 1.0];
const CELL_DEAD: Color = [0.25, 0.25, 0.25, 1.0];

It doesn't make a big difference, but it makes it a bit clearer when reading this code what these values are.

const CELL_SIZE: i32 = 1;

I think this should be an f64. Its the size of a cell in the OpenGL drawing context which is in f64, not i32.

#[derive(Clone)]
struct Cell {
    alive: bool,
}

impl Cell {
    fn set(&mut self, alive: bool) {
        self.alive = alive;
    }
}

Instead of a Cell containing a lone bool, it would make more sense to have an enum.

enum Cell {
   Living,
   Dead
}

In Rust, you don't want to think in OO terms where you call on methods on objects to manipulate them. Instead, you want data structures that represent state. That's why it makes more sense to have a Living/Dead enum rather than a struct which you can call.

struct CellWorld {
    rows: i32,
    columns: i32,
    map: Box<Vec<Vec<Cell>>>,
}

I might use usize instead of i32 since the sizes correspond to the length of the Vecs. Also there doesn't seem to be any reason to put your Vec into a Box.

impl CellWorld {
    fn new(window_height: f64, window_width: f64) -> CellWorld {
        let n_rows = (window_height as i32) / CELL_SIZE;
        let n_cols = (window_width as i32) / CELL_SIZE;

        CellWorld {
            rows: n_rows,
            columns: n_cols,
            map: Box::new(Vec::new()),
        }
    }
}

It is odd that you don't fill in the map here. Typically, we'd have the new function CellWorld produce a fully ready CellWorld. Right now that returned CellWorld is in an inconsistent state, its width/height and actually cells aren't consistent.

impl PistonApp {

    fn init(&mut self) {
        let mut rand_life = thread_rng();

I wouldn't put life in the name, just because its a general random number generator, its not really life specific.

        for row in 0..self.world.rows {
            let rand_num: i32 = rand_life.gen();
            let mut cell_row = vec![Cell { alive: row / rand_num == rand_life.gen() }; self.world.columns as usize];

I don't what this is trying to be... but I'm pretty sure this isn't it.

vec![] doesn't re-evaluate the expression for each element created, it clone()s the one element repeatedly. So this won't produce different values for the different cells.

I'm not sure what you are trying to with generating the two different random values and then taking a division. As it works out, a random i32 is almost certainly much bigger than your row index so row / rand_num is almost always zero. And its is highly unlikely that the second call to gen() will also produce zero. So this seems like it will just always produce false.

            for cell in 0..self.world.columns {
                cell_row[cell as usize].set(cell % 2 == 0);
            }

One way or the other, you then throw away everything you did above to set every other cell to true.

            self.world.map.push(cell_row);
        }
    }

I'd use iterators to actually create this, something like:

self.world.map = (0..self.world.rows).map(|_| 
   (0..self.world.columns).map(|_| 
       rand_life.rng()
   ).collect()
).collect();

That will create a randomly filled up grid.

    fn render(&mut self, render_args: &RenderArgs) {
        let cell_map = &self.world.map;
        let n_rows = self.world.rows;
        let n_columns = self.world.columns;

        self.gl_context.draw(render_args.viewport(), |c, gl| {
            clear(BACKGROUND_COLOR, gl);

            for row in 0..n_rows {
                let cell_row = &cell_map[row as usize];

                let row_offset = row * CELL_SIZE;

                for col in 0..n_columns {
                    let cell = &cell_row[col as usize];

Use iterators

for (row_index, row) in cell_map.iter().enumerate() {
    for (col_index, col) in row.iter().enumerate() {

That simplifies access to the index alongside the values.

    fn simulate(&mut self) {
        let n_rows = self.world.rows;
        let n_cols = self.world.columns;

        for row in 0..n_rows {
            for col in 0..n_cols {
                self.update_cell(row as usize, col as usize);
            }
        }
    }

Your implementation seems subtlety incorrect to me. In the GoL, all cells update at once. That is, the state of each cell at time T + 1 depends on the state of all cells at time T. But since you update cell by cell, some cells will already have been updated when you are updating a cell. In order to GoL right, you need to two copies of the map, the previous state and the new state being computed.

    fn update_cell(&mut self, row: usize, col: usize) {
        let world = &mut self.world;
        let world_map = &mut world.map;
        let mut alive_neighbors = 0;

        let has_left_neighbors = if col == 0 { false } else { col - 1 > 0 };
        let left_col = if has_left_neighbors { col - 1 } else { 0 };
        let has_right_neighbors = col + 1 < world.columns as usize;
        let right_col = if has_right_neighbors { col + 1 } else { 0 };
        let has_top_neighbors = if row == 0 { false } else { row - 1 > 0 };
        let top_row = if has_top_neighbors { row - 1 } else { 0 };
        let has_btm_neighbors = row + 1 < world.rows as usize;
        let btm_row = if has_btm_neighbors { row + 1 } else { 0 };

        if has_left_neighbors {
            if world_map[row][left_col].alive {
                alive_neighbors += 1;
            }

            if has_top_neighbors && world_map[top_row][left_col].alive {
                alive_neighbors += 1;
            }

            if has_btm_neighbors && world_map[btm_row][left_col].alive {
                alive_neighbors += 1;
            }
        }

        if has_right_neighbors {
            if world_map[row][right_col].alive {
                alive_neighbors += 1;
            }

            if has_top_neighbors && world_map[top_row][right_col].alive {
                alive_neighbors += 1;
            }

            if has_btm_neighbors && world_map[btm_row][right_col].alive {
                alive_neighbors += 1;
            }
        }

        if has_top_neighbors {
            if world_map[top_row][col].alive {
                alive_neighbors += 1;
            }
        }

        if has_btm_neighbors {
            if world_map[btm_row][col].alive {
                alive_neighbors += 1;
            }
        }

So... that was complicated, here's what I'd do:

   let mut alive_neighbors = 0;
   // by taking a slice of the vec we get rust to take care of the 
   // bounds checking.
   for row in world_map[row-1:row+2] {
          for col in row[col - 1:col + 2] {
             if col.alive {
                 alive_neighbors += 1;
             }
          }
       }
   }
   // the above will could the cell itself, so balance that.
   if world_map[row][col].alive {
      alive_neighbors -= 1;
   }

That, I think, is easier to follow.

        if alive_neighbors < 2 || alive_neighbors > 3 {
            world_map[row][col].set(false);
        }

        if alive_neighbors == 2 || alive_neighbors == 3 || world_map[row][col].alive == false && alive_neighbors == 3 {
            world_map[row][col].set(true);
        }

I'd do something like this:

world_map[row][col].set(
   if world_map[row][col] {
      alive_neighbors >= 2 && alive_neighbors <= 3
   } else {
      alive_neighbors == 3
   });

Firstly, this always set the value. This just makes the logic a bit more straightforward, since you don't have to figure out whether or not it changes. Also, by handling the living and dead cell cases seperately, we get a simpler presentation of the rule.