Conway's Game of Life not updating correctly

Question

I wrote a quick implementation of Conway's Game of Life, but it ran awfully slow mostly because my method of checking for neighbouring cells involved looping through the entire grid of cells again, now I've changed my method of checking for neighbouring cells but unfortunately it's not updating correctly anymore, it seems to work fine except that it doesn't create nearly as many new cells as it should.

Now I have spent a good few hours manually debugging the code, going through it with breakpoints and trying to compare the values and calls, but it SEEMS as if my GetNeighbours() method is working, so I concede to you, I can't figure out what's wrong on my own. I've submitted the code below for help.

EDIT: Some of you have pointed out that I can't copy my Grid.cells array the way I am doing it. I've changed it to use Array.Copy() instead but unfortunately it still doesn't work completely. I can't figure it out but it still doesn't seem to create new cells in all cases where it should.

MainForm.cs

public partial class MainFom : Form
{
    Grid formGrid;
    CancellationTokenSource tokenSrc = new CancellationTokenSource();

    public MainFom()
    {
        InitializeComponent();
    }

    private void MainFom_Load(object sender, EventArgs e)
    {
        formGrid = new Grid();

        for (int i = 0; i < 50; i++)
        {
            int xCoord = 10 * i + 12;      

            Controls.Add(new Label() 
            { 
                AutoSize = true, 
                Text = i.ToString(), 
                Location = new Point(xCoord, 0),
                Font = new Font(Font.FontFamily, 6)
            });

            for (int s = 0; s < 50; s++)
            {
                int yCoord = 10 * s + 12;

                Controls.Add(new Label() 
                { 
                    AutoSize = true, 
                    Text = s.ToString(), 
                    Location = new Point(0, yCoord),
                    Font = new Font(Font.FontFamily, 6)
                });
            }
        }
    }

    private void MainFom_Paint(object sender, PaintEventArgs e)
    {
        e.Graphics.DrawImage(formGrid.toBitmap(), 0, 0);
        e.Graphics.Dispose();
    }

    private void startBtn_Click(object sender, EventArgs e)
    {
        Task tempTask = Task.Factory.StartNew(
            (x) =>
            {
                while (!tokenSrc.IsCancellationRequested)
                {
                    formGrid.UpdateGrid();
                    Graphics graphics = this.CreateGraphics();
                    graphics.Clear(this.BackColor);
                    graphics.DrawImage(formGrid.toBitmap(), 0, 0);
                    graphics.Dispose();
                }
            }, tokenSrc);

        startBtn.Hide();
        Button stopBtn = new Button() { Text = "Stop", Location = startBtn.Location, Size = startBtn.Size };
        this.Controls.Add(stopBtn);
        stopBtn.Click += new EventHandler(
            (x, y) => 
            { 
                tokenSrc.Cancel(); 
                stopBtn.Hide();
                startBtn.Show();
                tempTask.Wait();
                tokenSrc = new CancellationTokenSource();
            });

    }
}

Grid.cs

class Grid
{
    #region Properties/Fields

    const int MAX_CELLS = 50;
    Random RNG = new Random();
    Cell[,] cells;
    int generations = new int();

    #endregion

    public Grid()
    {
        cells = new Cell[MAX_CELLS, MAX_CELLS];

        for (int x = 0; x < MAX_CELLS; x++)
        {
            int xCoord = 10 * x + 12; 

            for (int y = 0; y < MAX_CELLS; y++)
            {
                int yCoord = 10 * y + 12;
                Point point = new Point(xCoord, yCoord);

                if (RNG.Next(100) < 20) { 
                    cells[x, y] = new Cell(point, true); } 
                else {
                    cells[x, y] = new Cell(point, false);
                }
            }
        }
    }

    public void UpdateGrid()
    {
        Cell[,] copy = cells;

        for (int x = 0; x < MAX_CELLS; x++)
        {
            for (int y = 0; y < MAX_CELLS; y++)
            {
                int neighboursCtr = GetNeighbours(x, y);

                //Rule 1: Any live cell with fewer than two live neighbours dies, as if caused by under-population.
                if (cells[x, y].IsAlive && neighboursCtr < 2)
                {
                    copy[x, y].Kill();
                }
                //Rule 2: Any live cell with more than three live neighbours dies, as if by overcrowding.
                if (cells[x, y].IsAlive && neighboursCtr > 3)
                {
                    copy[x, y].Kill();
                }
                //Rule 3: Any dead cell with exactly three live neighbours becomes a live cell, as if by reproduction.
                if (!cells[x, y].IsAlive && neighboursCtr == 3)
                {
                    copy[x, y].Alive();
                }
            }
        }

        cells = copy;

        generations++;
    }

    public Bitmap toBitmap()
    {
        Bitmap gridBmp = new Bitmap(1000, 1000); // TODO: Find optimal size for bmp
        Size cellSize = new Size(10, 10);

        using (Graphics gfxObj = Graphics.FromImage(gridBmp))
        {
            // Draw grid here and Dispose() on Pen, gfxObj is implicitly disposed
            Pen myPen = new Pen(Color.LightGray);
            SolidBrush myBrush = new SolidBrush(Color.Black);

            for (int x = 0; x < MAX_CELLS; x++)
            {
                for (int y = 0; y < MAX_CELLS; y++)
                {
                    if (!cells[x, y].IsAlive)
                    {
                        gfxObj.DrawRectangle(myPen, new Rectangle(cells[x, y].point, cellSize));
                    } else
                    {
                        gfxObj.FillRectangle(myBrush, new Rectangle(cells[x, y].point, cellSize));
                    }
                }
            }
            myPen.Dispose();
            myBrush.Dispose();
        }

        return gridBmp;
    }

    private int GetNeighbours(int column, int row)
    {
        int neighbours = new int();
        int[] starts = new int[] { Math.Max(0 ,column - 1), Math.Max(0, row - 1) };
        int[] ends = new int[] { Math.Min(49, column + 1), Math.Min(49, row + 1) };
        double colAndRow = column + row/10;

        for (int x = starts[0]; x < ends[0]+1; x++)
        {
            for (int y = starts[1]; y < ends[1]+1; y++)
            {
                double xAndY = x + y/10;
                if (cells[x, y].IsAlive && xAndY != colAndRow)
                {
                    neighbours++;
                }
            }
        }

        return neighbours;
    }
}

Cell.cs

struct Cell
{
    public bool IsAlive { get; private set; }
    public readonly Point point;

    public Cell(Point point, bool isAlive) : this()
    {
        this.point = point;
        IsAlive = isAlive;
    }

    public void Alive()
    {
        IsAlive = true;
    }

    public void Kill()
    {
        IsAlive = false;
    }
}

Okay, I'll do that, it's not really the "source" of the error though. — Overly Excessive, Mar 17 '14 at 14:18
http://ericlippert.com/2014/03/05/how-to-debug-small-programs/ — Eric Lippert, Mar 17 '14 at 15:15
This question appears to be off-topic because StackOverflow is not a service for debugging your buggy code. "I can't figure out a program that I wrote" is not a *question*, it is a *fact*. Come on back when you have a *specific* question. — Eric Lippert, Mar 17 '14 at 15:16

score 1 · Answer 1 · answered Mar 17 '14 at 14:26

The problem is in your UpdateGrid() method. You're simply assigning the reference for your original array to a new variable:

Cell[,] copy = cells;

But this is still the same object; in particular, there's no difference between calling copy[x, y].Kill() and cells[x, y].Kill(). So you're modifying your state during calculations, this affects your code's logic.

Make a copy of the original using Array.Copy and it should work correctly (there doesn't seem to be anything else wrong with your algorithm).

treze · Answer 2 · 2014-03-17T15:08:42.053

1

Arrays are reference types, which means

Cell[,] copy = cells;

doesn't what you probably intent to do. It's not a copy of the source array, so it will manipulate this whilst analyzing the neighbors which will lead to wrong results.

Use Array.Copy.

edited Mar 17 '14 at 15:08

answered Mar 17 '14 at 14:27

treze

2,671
15
19

I don't believe `Array` has a `ToArray()` method (or extension method). – decPL Mar 17 '14 at 14:48
@decPL it's a LINQ extension method – treze Mar 17 '14 at 14:56
There is one on `IEnumerable` but multidimensional arrays do not implement that interface. You could obviously use `Cast()`, followed by `ToArray()`, but that'd give you a one dimensional array which is probably less than helpful here. – decPL Mar 17 '14 at 15:05

score 0 · Answer 3 · edited May 23 '17 at 12:05

There are a lot of improvements that can be done. Take a look at Optimizing Conway's 'Game of Life' and Hashlife

You can start by using LockBits to work faster with the bitmap.

You can use parallel programming to improve the loops: Save time with parallel FOR loop

You can also improve the algorithm avoiding the whole matrix scan each time, and instead maintain a list of the alive cells, and only step thru these cells and it's neighbors. I've implemented such algorithm in the following C# Game of Life Code.

Conway's Game of Life not updating correctly

3 Answers3