29
\$\begingroup\$

I wrote this to learn JavaFX, and as an excuse to re-make the Game of Life. This is the most complex GUI I've ever written, so I'd like feedback mainly on it, but I'll welcome any criticism!

My version differs from the original in that cells inherit their neighbour's color when they're born, which leads to interesting "war" scenarios.

It's also technically "1-player", as the user can intervene by painting new live cells right onto the field to affect the evolution.

A sample:

GOLSample


What I'd like commented on mainly:

  • All of the settings that the controls affect are global; they're (private) instance members of the application class. I know that in general, this is bad practice, but I'm not sure how else to set it up. If I have an event handler that needs to change a setting, short of threading every setting through the handlers and the main-loop, I'm not sure how else it could be accomplished.

  • The general set-up of the controls in the code. Is there a standard way of creating/setting up controls? Here, I've segregated set up of controls into functions based on which bar they belong to. It works, but I can see it getting out of control with more controls added.

  • The GOL class (Environment) is actually faster than I expected, but it gets quite slow at around 500x500 cells. Is there anything I can do to help speed it up?

  • Anything regarding layout of the GUI.

Note that currently this isn't a "real" GOL clone in that it's not infinite. When a cell exits the screen, it's no longer "seen", and is erased the next generation. This leads to the corruption of classic patterns if they collide with the end of the world.

GameOfLife.java:

package gameOfLife;

import static utils.Utils.clearCanvas;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Random;

import javafx.application.Application;
import javafx.event.ActionEvent;
import javafx.event.Event;
import javafx.event.EventHandler;
import javafx.scene.Scene;
import javafx.scene.canvas.Canvas;
import javafx.scene.canvas.GraphicsContext;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.Slider;
import javafx.scene.control.TextField;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.scene.paint.CycleMethod;
import javafx.scene.paint.LinearGradient;
import javafx.scene.paint.Stop;
import javafx.scene.shape.Rectangle;
import javafx.scene.text.Font;
import javafx.scene.text.TextAlignment;
import javafx.stage.Stage;
import utils.MainLoop;
import utils.Position;

public class GameOfLife extends Application {

    private final int startCanvasWidth = 1500, startCanvasHeight = 800;
    private double gameSpeedSecs = 0.016667;

    //Global settings to be modified by the controls
    private boolean isPaused = false;
    private boolean clearScreen = true;
    private int birthRadiusOnClick = 5;
    private Color drawColor = Color.BLACK;
    private int cellsWide = 300;
    private int cellsHigh = 300;

    //Settings that can be looked up directly when needed
    private final Slider lifeChanceSlider = new Slider(0, 1, 0.5);
    private final Slider cellsWideSlider = new Slider(10, 800, 200);
    private final Slider cellsHighSlider = new Slider(10, 800, 200);
    private final Label statusLabel = new Label("");

    private Stage mainStage;
    private Scene mainScene;

    private Canvas canvas = new Canvas(startCanvasWidth, startCanvasHeight);
    private GraphicsContext gc = canvas.getGraphicsContext2D();

    private final Random randGen = new Random(993061001);

    private final List<Color> colors = Arrays.asList(Color.BLUE, Color.DARKGOLDENROD, Color.RED, Color.GREEN,
            Color.DARKCYAN, Color.PURPLE, Color.BLACK, Color.CHOCOLATE, Color.AQUA,
            Color.FUCHSIA, Color.HOTPINK, Color.TURQUOISE, Color.AQUAMARINE, Color.CRIMSON,
            Color.BLANCHEDALMOND, Color.THISTLE, Color.DARKORCHID);

    private final Environment<Color> env = new Environment<Color>();

    public static void main(String[] args) {
        launch(args);
    }

    private double getCanvasWidth() {
        return canvas.getWidth();
    }

    private double getCanvasHeight() {
        return canvas.getHeight();
    }

    /**
     * Gets the average (between height and width) ratio between the chosen screen size and the area of the environment
     * @return The average ratio
     */
    private double getAverageScalingFactor() {
        return (getCanvasWidth() + getCanvasHeight()) / (cellsWide + cellsHigh);
    }

    /**
     * Gets the chosen dimensions of the environment as decided by the sliders.
     * @return a Duple representing the chosen (width, height) of the area
     */
    private Position<Integer> getCellAreaBySliders() {
        return new Position<Integer>(
            (int)cellsWideSlider.getValue(),
            (int)cellsHighSlider.getValue() );
    }

    /**
     * Gets the chance that a cell should live during a repopulation as decided by the slider.
     * @return The value of the slider
     */
    private double getLifeChanceBySlider() {
        double lifeChance = lifeChanceSlider.getValue();

        assert (lifeChance >= 0 && lifeChance <= 1) : ("Life Chance out of Range: " + lifeChance);

        return lifeChance;
    }

    /**
     * Finds the chosen area as decided by the sliders, and sets it.
     */
    private void setCellAreaBySliders() {
        Position<Integer> cellArea = getCellAreaBySliders();

        cellsWide = cellArea.getX();
        cellsHigh = cellArea.getY();
    }

    /**
     * Resets the population according to the controls
     */
    private void rePopulateAccordingToControls() {
        Position<Integer> popArea = getCellAreaBySliders();

        env.randomizePopulation(0, popArea.getX(), 0, popArea.getY(), getLifeChanceBySlider(), colors, randGen);
    }

    @Override
    public void start(Stage stage) {
        mainStage = stage;
        mainStage.setMaximized(true);

        mainStage.centerOnScreen();

        BorderPane rootNode = new BorderPane();

        mainScene = new Scene(rootNode, mainStage.getWidth(), mainStage.getHeight());

        //To "draw" cells
        canvas.setOnMousePressed(mouseHandler);
        canvas.setOnMouseDragged(mouseHandler);

        mainStage.setScene(mainScene);

        HBox toolBar = new HBox(5);
        setupTopBar(toolBar);

        VBox rePopBar = new VBox(5);
        setupRePopBar(rePopBar);
        setupRePopSliders();

        HBox colorBar = new HBox(5);
        setupColorBar(colorBar, 50);

        rootNode.setTop(toolBar);
        rootNode.setCenter(canvas);
        rootNode.setRight(rePopBar);
        rootNode.setBottom(colorBar);

        env.randomizePopulation(0, cellsWide, 0, cellsHigh, 0.6, colors, randGen);

        MainLoop mainLoop = new MainLoop((long)(gameSpeedSecs * 1000000000L),
            elapsedNS -> {

                Color backgroundColor = Color.WHITE;

                if (clearScreen) clearCanvas(gc);

                if (isPaused) {
                    gc.setTextAlign(TextAlignment.CENTER);
                    gc.setFont(Font.font(70));
                    gc.fillText("PAUSED", mainStage.getWidth() / 2,
                                            mainStage.getHeight() / 2);

                } else {
                    //To decide how big each cell should be
                    double scalingFactor = getAverageScalingFactor();

                    gc.setFill(backgroundColor);
                    gc.fillRect(0, 0, canvas.getWidth(), canvas.getHeight());

                    //Loop over the environment, drawing a cell if it's alive
                    Environment.forAllCells(0, cellsWide, 0, cellsHigh, (x, y) -> {
                        Color cellColor = env.getCellSpecies(x, y);
                        if (cellColor != null) {

                            double sX = x * scalingFactor, sY = y * scalingFactor,
                                    sSize = scalingFactor;

                            gc.setFill(cellColor);
                            gc.fillRect(sX, sY, sSize, sSize);
                        }


                    });     

                env.simGeneration(0, cellsWide, 0, cellsHigh);

            }
        });

        mainLoop.start();

        mainStage.show();
    }

    public void setupTopBar(HBox toolBar) {
        Button pauseButton = new Button("Pause");
        pauseButton.setOnMouseClicked(pauseHandler);
        toolBar.getChildren().add(pauseButton);

        Button clearButton = new Button("Clear");
        clearButton.setOnMouseClicked(clearHandler);
        toolBar.getChildren().add(clearButton);

        Button toggleClearButton = new Button("Toggle ScreenClear");
        toggleClearButton.setOnMouseClicked(toggleClearHandler);
        toolBar.getChildren().add(toggleClearButton);

        Label dropLabel = new Label("Drop n*n square:");
        toolBar.getChildren().add(dropLabel);

        TextField birthRadText = new TextField();
        birthRadText.setOnAction(birthRadHandler);
        toolBar.getChildren().add(birthRadText);
    }

    private void updateStatusLabel() {
        Position<Integer> area = getCellAreaBySliders();

        statusLabel.setText(
            area.getX() + " x " + area.getY() + "\n" + (int)(getLifeChanceBySlider() * 100) + "% chance"
        );
    }

    private void setupRePopBar(VBox rePopBar) {
        rePopBar.getChildren().add(new Label("Cell Life Chance: "));
        rePopBar.getChildren().add(lifeChanceSlider);

        rePopBar.getChildren().add(new Label("Cells Wide: "));
        rePopBar.getChildren().add(cellsWideSlider);

        rePopBar.getChildren().add(new Label("Cells High: "));
        rePopBar.getChildren().add(cellsHighSlider);

        Button submitRePopButton = new Button("Repopulate");
        rePopBar.getChildren().add(submitRePopButton);
        submitRePopButton.setOnMouseClicked( e -> {
            setCellAreaBySliders();
            rePopulateAccordingToControls();
        });

        rePopBar.getChildren().add(statusLabel);

        rePopBar.setPrefWidth(400);
    }

    /**
     * Creates a colored box that sets the main drawing color to its color when clicked
     * 
     * @param color The color to paint the box and to set the drawing color to when pressed
     * @param sideLength The side length to make the square box
     * @return The created box
     */
    public Rectangle newColorBox(Color color, double sideLength) {
        Rectangle box = new Rectangle(sideLength, sideLength);

        box.setFill(color);

        box.setOnMouseClicked( event -> {
            drawColor = color;
        });

        return box;
    }

    /**
     * Generates a list of stops between each color to create a gradient over each color
     * 
     * @param colors The list of colors to create a gradient for
     * @return A list of stops for the given colors
     */
    private static List<Stop> evenStops(List<Color> colors) {
        List<Stop> stops = new ArrayList<Stop>(colors.size());

        for (int i = 0; i < colors.size(); i++) {
            Stop s = new Stop((double)i / colors.size(), colors.get(i));

            stops.add(s);
        }

        return stops;
    }

    private void setupColorBar(HBox colorBar, double boxSideLength) {
        for (Color c : colors) {
            colorBar.getChildren().add(newColorBox(c, boxSideLength));
        }

        Rectangle randBox = new Rectangle(boxSideLength, boxSideLength);

        randBox.setFill(new LinearGradient(0, 0, 1, 1, true, CycleMethod.REFLECT, evenStops(colors)));
        randBox.setOnMouseClicked( e -> { drawColor = colors.get(randGen.nextInt(colors.size())); });

        colorBar.getChildren().add( randBox );

    }

    private void setupLabelSliderHandlers(Slider slider) {
        slider.setOnScroll( e -> updateStatusLabel()); 
        slider.setOnKeyReleased( e -> updateStatusLabel()); 
        updateStatusLabel();
    }

    private void setupRePopSliders() {
        lifeChanceSlider.setShowTickMarks(true);
        lifeChanceSlider.setMajorTickUnit(0.2);
        lifeChanceSlider.setMinorTickCount(1);
        lifeChanceSlider.setSnapToTicks(true);
        lifeChanceSlider.setBlockIncrement(0.1);
        setupLabelSliderHandlers(lifeChanceSlider);

        cellsWideSlider.setShowTickMarks(true);
        cellsWideSlider.setMajorTickUnit(50);
        cellsWideSlider.setMinorTickCount(1);
        cellsWideSlider.setSnapToTicks(true);
        cellsWideSlider.setBlockIncrement(10);
        setupLabelSliderHandlers(cellsWideSlider);

        cellsHighSlider.setShowTickMarks(true);
        cellsHighSlider.setMajorTickUnit(50);
        cellsHighSlider.setMinorTickCount(1);
        cellsHighSlider.setSnapToTicks(true);
        cellsHighSlider.setBlockIncrement(10);
        setupLabelSliderHandlers(cellsHighSlider);
    }

    //Event Handlers

    EventHandler<Event> pauseHandler = event -> {
        isPaused = !isPaused;
    };

    EventHandler<MouseEvent> mouseHandler = event -> {
        double scale = getAverageScalingFactor();
        int sX = (int)(event.getX() / scale),
            sY = (int)(event.getY() / scale);

        Environment.forAllCells(sX, sX, sY, sY, birthRadiusOnClick, (x,y) -> {
            env.setCell(x, y, drawColor);
        });
    };

    EventHandler<Event> clearHandler = event -> {
        clearCanvas(gc);
    };

    EventHandler<Event> toggleClearHandler = event -> {
        clearScreen = !clearScreen;
    };

    /**
     * Attempts to parse the given text as a integer and set the draw radius.<p>
     * 
     * Outputs an error to the System.out in the event of a parsing error.
     */
    EventHandler<ActionEvent> birthRadHandler = event -> {
        TextField textField = (TextField)event.getSource();

        String enteredText = textField.getCharacters().toString();

        try {
            birthRadiusOnClick = Integer.parseInt(enteredText);

        } catch(NumberFormatException e) {
            System.out.println("Invalid birth radius entered: " + enteredText);
        }
    };

}

Environment.java:

package gameOfLife;

import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.function.BiConsumer;

import utils.Position;

/**
 * An object representing a Game of Life environment.
 * 
 * @author Brendon
 *
 * @param <S> The species representation of the cells
 */
public class Environment<S> {

    /**
     * The currently living cells. Any cell not explicitly alive is considered dead.
     */
    private Map<Position<Integer>, S> liveCells = new HashMap<Position<Integer>, S>();

    //Cached as instance member to avoid repeated constant construction/destruction
    private final SpeciesFreqs speciesFreqs = new SpeciesFreqs();

    /**
     * Finds what species a cell at a certain cell is, or null if the cell is dead
     * 
     * @param x The x-coordinate
     * @param y The y-coordinate
     * @return The species of the cell at (x,y) if alive, or null if dead
     */
    public S getCellSpecies(int x, int y) {
        return liveCells.get(new Position<Integer>(x, y));
    }

    /**
     * Generates a {@link SpeciesFreqs} of the cells surrounding (x,y).<p>
     * A range of 1 scans the 8 immediate cells surrounding (x,y) (default behavior)
     * 
     * @param x The x-coordinate
     * @param y The y-coordinate
     * @param range The depth to check from the centre coordinate.
     * @return A reference to the instance speciesFreqs recorder.
     */
    private SpeciesFreqs getNeighborsOf(int x, int y, int range) {
        speciesFreqs.reset();

        for (int checkY = y - range; checkY <= y + range; checkY++) {
            for (int checkX = x - range; checkX <= x + range; checkX++) {
                Position<Integer> pos = new Position<Integer>(checkX, checkY);
                S cellSpecies = liveCells.get(pos);

                if (cellSpecies != null && !(checkX == x && checkY == y)) {

                    speciesFreqs.add(cellSpecies);
                }
            }
        }

        return speciesFreqs;
    }

    /**
     * Figures out whether or not the cell at (x,y) is alive or dead, and what
     *  species it should adopt.
     *  
     * @param x The x-coordinate to check
     * @param y The y-coordinate to check
     * @return The species the cell should become, or null if it's dead.
     */
    private S findStateOfCell(int x, int y) {
        SpeciesFreqs neighborSpecies = getNeighborsOf(x, y, 1);
        int neighbors = neighborSpecies.getNNeighbors();

        S domSpecies = neighborSpecies.getDomSpecies();
        boolean isCurrentlyAlive = getCellSpecies(x,y) != null;

        if (isCurrentlyAlive) {
            if (neighbors == 2 || neighbors == 3)
                return domSpecies;
            else return null;

        } else {
            if (neighbors == 3)
                return domSpecies;
            else return null;
        }

    }

    /**
     * A compacted 2D for-loop
     * @param xMin The starting x-value for the loop
     * @param xMax The inclusive max x-value
     * @param yMin The starting y-value for the loop
     * @param yMax The inclusive max y-value
     * @param f The body of the loop
     */
    public static void forAllCells(int xMin, int xMax, int yMin, int yMax, BiConsumer<Integer, Integer> f) {
        for (int checkY = yMin; checkY <= yMax; checkY++) {
            for (int checkX = xMin; checkX <= xMax; checkX++) {
                f.accept(checkX, checkY);
            }
        }
    }

    /**
     * A compacted 2D for-loop<p>
     * An convenience version that automatically subtracts the range from the minimum values, and adds it to the maximum values.<p>
     * 
     * In the x-dimension the loop starts at xMin - range, and extends to (inclusive) xMax + range
     * 
     * @param xMin The starting x-value for the loop
     * @param xMax The inclusive max x-value
     * @param yMin The starting y-value for the loop
     * @param yMax The inclusive max y-value
     * @param range The amount to increase the loop by in each "direction"
     * @param f The body of the loop
     */
    public static void forAllCells(int xMin, int xMax, int yMin, int yMax, int range, BiConsumer<Integer, Integer> f) {
        forAllCells(xMin - range, xMax + range, yMin - range, yMin + range, f);
    }

    /**
     * Allows a cell to be forcibly made alive. <p>
     * Replaces old cells at the target location
     * 
     * @param x The x-coordinate of the cell to set
     * @param y The y-coordinate of the cell to set
     * @param species The species of the cell to make it alive, or null to kill it
     */
    public void setCell(int x, int y, S species) {
        liveCells.put(new Position<Integer>(x,y), species);
    }

    /**
     * Advances the environment by 1 "generation".<p>
     * After calling, the population will have been replaced by the new generation as determined by the rules.<p>
     * Only "scans" and updates the area indicated by the ranges
     * 
     * 
     * @param xMin The minimum value in the x-dimension to affect
     * @param xMax The maximum value (inclusive) in the x-dimension to affect
     * @param yMin The minimum value in the y-direction to affect
     * @param yMax The maximum value (inclusive) in the y-direction to affect
     */
    public void simGeneration(int xMin, int xMax, int yMin, int yMax) {
        Map<Position<Integer>, S> newLiveCells = new HashMap<Position<Integer>, S>();

        forAllCells(xMin, xMax, yMin, yMax, (x, y) -> {
            S cellState = findStateOfCell(x, y);

            if (cellState != null) {
                newLiveCells.put(new Position<Integer>(x, y), cellState);
            }

        });

        liveCells = newLiveCells;
    }

    /**
     * Clears and replaces the current population with a randomized population with the indicated traits within the indicated bounds.
     * 
     * @param xMin The minimum value in the x-dimension to populate
     * @param xMax The maximum value (inclusive) in the x-dimension to populate
     * @param yMin The minimum value in the y-direction to populate
     * @param yMax The maximum value (inclusive) in the y-direction to populate
     * @param chanceOfLife The decimal chance that any cell with become alive. (<= 0) results in an empty population, (>= 1) results in a full population
     * @param possibleSpecies The range of possible species 
     * @param randGen A random generator
     */
    public void randomizePopulation(int xMin, int xMax, int yMin, int yMax,
            double chanceOfLife, List<S> possibleSpecies, Random randGen) {

        liveCells.clear();

        forAllCells(xMin, xMax, yMin, yMax, (x, y) -> {
            if (randGen.nextDouble() <= chanceOfLife) {
                liveCells.put(new Position<Integer>(x, y), possibleSpecies
                        .get(randGen.nextInt(possibleSpecies.size())));
            }
        });
    }

    @Override
    public String toString() {
        StringBuilder strB = new StringBuilder();

        for (int checkY = 0; checkY <= 10; checkY++) {
            for (int checkX = 0; checkX <= 10; checkX++) {
                S cellSpecies = getCellSpecies(checkX, checkY);
                strB.append((cellSpecies != null ? cellSpecies : ' ') + " ");
            }

            strB.append('\n');
        }

        return strB.toString();
    }

    public static void main(String[] args) {
        Random randGen = new Random(993061003);

        int xB = 9, yB = 9;

        Environment<Character> env = new Environment<>();
        // env.randomizePopulation(0, xB, 0, yB, 0.1, randGen);

        env.liveCells.put(new Position<Integer>(2, 1), 'A');
        env.liveCells.put(new Position<Integer>(2, 2), 'B');
        env.liveCells.put(new Position<Integer>(2, 3), 'C');

        for (int i = 0; i < 3; i++) {
            System.out.println("____________________\n" + env);

            env.simGeneration(0, xB, 0, yB);
        }
    }

    /**
     * A simple mutable integer class to assist in efficient species counting.
     * 
     * @author Brendon
     *
     */
    static class MutableInt {
        private int val;

        /**
         * Defaults to a value of 0
         */
        public MutableInt() {
            this(0);
        }

        public MutableInt(int v) {
            val = v;
        }

        public void inc() {
            val++;
        }

        public int getVal() {
            return val;
        }

        @Override
        public String toString() {
            return String.valueOf(val);
        }
    }

    /**
     * A class to assist in counting the number of each species surrounding a cell.
     * @author Brendon
     *
     */
    class SpeciesFreqs {
        private final Map<S, MutableInt> speciesFreqs = new HashMap<>();
        private int totalNeighbors = 0;

        /**
         * Adds an instance of a species to the count
         * @param species
         */
        public void add(S species) {
            MutableInt count = speciesFreqs.get(species);

            if (count == null) {
                speciesFreqs.put(species, new MutableInt(1));

            } else {
                count.inc();
            }

            totalNeighbors++;
        }

        /**
         * Clears the recorded neighbors 
         */
        public void reset() {
            speciesFreqs.clear();
            totalNeighbors = 0;
        }

        /**
         * Returns the total number of recorded neighbors
         * @return The total number of neighbors recorded since the last reset 
         */
        public int getNNeighbors() {
            return totalNeighbors;
        }

        /**
         * Figures out the dominant counted color. Considers the last species added to be dominant in the event of a tie. 
         * @return The most-occurring species counted, or the last species in the event of a tie.
         */
        public S getDomSpecies() {
            if (speciesFreqs.isEmpty()) return null;

            S domSpecies = null;
            int highFreq = 0;

            for (Map.Entry<S, MutableInt> entry : speciesFreqs.entrySet()) {
                S species = entry.getKey();
                int freq = entry.getValue().getVal();

                if (freq >= highFreq) {
                    highFreq = freq;
                    domSpecies = species;
                }
            }

            return domSpecies;
        }

    }
}

MainLoop.java:

package utils;

import java.util.function.Consumer;

import javafx.animation.AnimationTimer;

/**
 * An extension of an {@link AnimationTimer} that allows the user to select their framerate.<p>
 * 
 * Also allows for a more convenient syntax to declare the main loop routine
 * 
 * @author Brendon
 *
 */
public class MainLoop extends AnimationTimer {

    private final long updateGraphicsEvery;

    private final Consumer<Long> doEveryUpdate;

    private long lastTime = IDEALFRAMERATENS;

    /**
     * @param updateEveryNS How often to run the loop.
     * @param f The main-loop body. Its parameter is the number of nanoseconds since the last update.
     */
    public MainLoop(long updateEveryNS, Consumer<Long> f) {
        this.updateGraphicsEvery = updateEveryNS;
        doEveryUpdate = f;
    }

    @Override
    public void handle(long currentTime) {

        long nanosElapsed = currentTime - lastTime;

        if (nanosElapsed < updateGraphicsEvery) {
            return;

        } else {
            lastTime = currentTime;
            doEveryUpdate.accept(nanosElapsed);

        }

    }

    public final static long NANOSPERSECOND = 1000000000;
    public final static long IDEALFRAMERATENS = (long)(1 / 60.0 * NANOSPERSECOND);

}

Utils.java:

package utils;

import java.util.function.Consumer;

import javafx.animation.AnimationTimer;
import javafx.scene.canvas.Canvas;
import javafx.scene.canvas.GraphicsContext;

public class Utils {
    private Utils() {

    }

    /**
     * Clears the canvas associated with the given {@link GraphicsContext}
     * 
     * @param gc The associated GraphicsConetext to clear
     */
    public static void clearCanvas(GraphicsContext gc) {
        Canvas c = gc.getCanvas();
        gc.clearRect(0, 0, c.getWidth(), c.getHeight());
    }

}

Position.java:

package utils;

import java.util.function.BinaryOperator;
import java.util.function.UnaryOperator;

/**
 * A simple immutable Duple class where both members are the same type
 * 
 * @author Brendon
 *
 * @param <T> The type of its members
 */
public class Position<T> {

    private final T xPos;
    private final T yPos;

    public Position(T x, T y) {
        xPos = x;
        yPos = y;
    }

    public T getX() {
        return xPos;
    }

    public T getY() {
        return yPos;
    }

    /**
     * Applies the function to each member of the Duple
     * @param f The function to apply to each member
     * @return The resulting Position
     */
    public Position<T> map(UnaryOperator<T> f) {
        return new Position<T>(f.apply(xPos), f.apply(yPos)); 
    }

    /**
     * Applies the function to this and the other Position
     * @param otherPos The other Position to use
     * @param f A function taking 2 Positions, where the first parameter is a member of this, and the second is a member of otherPos
     * @return The resulting Position
     */
    public Position<T> map(Position<T> otherPos, BinaryOperator<T> f) {
        return new Position<T>(f.apply(xPos, otherPos.xPos), f.apply(yPos, otherPos.yPos)); 
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((xPos == null) ? 0 : xPos.hashCode());
        result = prime * result + ((yPos == null) ? 0 : yPos.hashCode());
        return result;
    }


    @SuppressWarnings("rawtypes")
    @Override
    public boolean equals(Object obj) {
        if (this == obj) return true;
        if (obj == null) return false;
        if (getClass() != obj.getClass()) return false;
        Position other = (Position) obj;
        if (xPos == null) {
            if (other.xPos != null) return false;
        } else if (!xPos.equals(other.xPos)) return false;
        if (yPos == null) {
            if (other.yPos != null) return false;
        } else if (!yPos.equals(other.yPos)) return false;
        return true;
    }

    @Override
    public String toString() {
        return "(" + xPos + "," + yPos + ")";
    }

}
\$\endgroup\$
  • \$\begingroup\$ Just a side note, the "Toggle Screen Clear" and "Clear" buttons effect screen clearing between generations. They have nothing to do with the game of life, but creates an interesting effect that looks like a painting painting itself. They'll likely be taken out in future versions, so they can be ignored. \$\endgroup\$ – Carcigenicate Oct 17 '15 at 1:03
  • \$\begingroup\$ @Antot Added. 2 things to note: 1. I've since renamed Position to Duple (although that's not reflected in this code), 2. MainLoop, Utils and Position are part of the utils package, so be sure to either modify that or set up the packages accordingly. \$\endgroup\$ – Carcigenicate Oct 18 '15 at 13:29
  • \$\begingroup\$ Duple? Did you mean to name it Pair? What does Duple mean? \$\endgroup\$ – Justin Oct 23 '15 at 4:50
  • \$\begingroup\$ @Justin Awhile ago, I heard Duple used as a specific case of a 2-element tuple, but I'm hard pressed to find a similar usage now that I've looked. You're right, it should be something like Pair. \$\endgroup\$ – Carcigenicate Oct 23 '15 at 12:53
  • \$\begingroup\$ Could you update this with your latest code before I start reviewing it (i.e. with the new class names)? Otherwise I'm gonna give advice that either doesn't apply or you've already put in. \$\endgroup\$ – Nic Hartley Dec 6 '15 at 23:43
8
\$\begingroup\$

Naming

Manifest constants typically use _ for space in Java. So NANOSPERSECOND for example should be NANOS_PER_SECOND.

Also I believe that the name MainLoop is quite misleading as a name because the class isn't actually your main loop but rather a timing helper.

Avoid unnecessary conversions

You don't need the double precision here:

public final static long IDEALFRAMERATENS = (long)(1 / 60.0 * NANOSPERSECOND);

just swap the operations:

public final static long IDEALFRAMERATENS = NANOSPERSECOND / 60;

The result is the same because you're truncating anyway.

Model View Controller (non-)separation

Your class GameOfLife mixes all three of model, view and controller. It is very desirable to keep these separate. What I would do is to separate everything that has to do with the game simulation: isPaused, cellsWide/High, the mainloop etc and put into a new class GameOfLifeSimulator. Next I would separate the UI design from the class and put it into an FXML document that describes the UI. Then I would have a separate controller class for the FXML document and leave the application class just as a pretty basic launcher.

If what I just said sounds like gibberish, have a look here: Model View Controller.

Using FXML would allow you to create your event handlers as member functions in the controller instead. So you would have:

@FXML
public void startStopSimulation(){
    isPaused = !isPaused;
}

and in the fxml you would have an attribute saying onAction="startStopSimulation". To link the button action to the method in your controller.

This here will get you started with FXML.

Use properties

JavaFX controls are built on the powerful concept of observables, properties and bindings. An observable is a class that wraps an object that allows you to listen to changes in the object. A property is a value that is observable. A binding is a way to create an expression that is bound to the values of properties, using the fact that they are observable. So if a property changes, so does all bindings that depend on it. And finally, you can bind the value of a property to a binding so you can effectively link properties to each other and create arbitrary expressions of your properties and bind those expressions to other properties. More info here.

For example you could do the following:

NumberBinding averageScaleFactor = Bindings.divide(
    canvas.widthProperty().add(canvas.heightProperty()),
    cellsWideSlider.valueProperty().add(cellsHighSlider.valueProperty()));

To create a binding, a kind of numerical expression that can be evaluated.

And whenever you need the scale factor simply call: averageScaleFactor.get(). The result is cached and automatically updated as soon as either the canvas or the sliders change. So the above snippet will eliminate the getAverageScalingFactor method.

Going with the Model-View-Controller idea, your "model", the GameOfLifeSimulator would have: IntegerProperty cellsWide/High; that you would bind to the value of the sliders of your UI ("view") in your "controller". And they would automatically be kept in sync.

Avoid allocating new objects in your game loop.

I see that you frequently allocate new Position elements in your game code. While memory allocation is cheap in Java, frequently allocating objects and throwing them away like you're doing will cause a significant strain on the GC and can cause your game to appear "jittery". I would advice to allocate the position objects once and re-use them where it makes sense.

For example here:

private SpeciesFreqs getNeighborsOf(int x, int y, int range) {
    speciesFreqs.reset();

    for (int checkY = y - range; checkY <= y + range; checkY++) {
        for (int checkX = x - range; checkX <= x + range; checkX++) {
            Position<Integer> pos = new Position<Integer>(checkX, checkY);
            S cellSpecies = liveCells.get(pos);

            if (cellSpecies != null && !(checkX == x && checkY == y)) {

                speciesFreqs.add(cellSpecies);
            }
        }
    }

you allocate a new position for each of the $$4\cdot range^2$$ elements checked when you could get away with simply allocating pos once at the top and re-using it.

Or here:

public S getCellSpecies(int x, int y) {
    return liveCells.get(new Position<Integer>(x, y));
} 

or here:

public void setCell(int x, int y, S species) {
    liveCells.put(new Position<Integer>(x,y), species);
}

This whole allocation and disposal of Position objects seem to be centered around the fact that you're using a Map<Position,S> to represent your live-state of the game. This may also be a cause for your performance hit, every access you make to the map requires that you must calculate the hash-code of these positions over and over again.

It would probably be faster if you just used Integer as key and used a custom hash function like so x + MAX_WIDTH*y. The function is collision free and faster to calculate than the hashCode of your Position class. Also you avoid straining the GC.

Another thing that might be even faster is to simply have a large array List<S> cells = new ArrayList<>(cellsWide * cellsHigh); and let dead cells be null. Access is done by cells.get(x + cellsWide*y) which avoids the lookup of the map and the computation of the hash code. It also will allow you to ditch allocating Position objects all over the place. However it does mean that the resize behavior of the game changes and you will have to reallocate and copy the field when that happens. However that is a rare event compared to how often the simulation is ran so I believe you will gain performance on this.

Summary

This is as far as I feel it is useful to go in this review. The best thing you can do is to separate your model, view and controller and reduce the load on the GC in your game code. The frequent allocation and disposal of Position objects and the expensive hashCode function is a likely suspect for your performance issues.

\$\endgroup\$
  • \$\begingroup\$ Awesome, thank you. I'm not going to accept this quite yet though, as someone else had said that they're planning on doing a review as well; although they seem to have forgotten. \$\endgroup\$ – Carcigenicate Jan 8 '16 at 15:26
  • \$\begingroup\$ @Carcigenicate Not to be that gal but that was a month ago :) \$\endgroup\$ – Emily L. Jan 8 '16 at 22:08
  • \$\begingroup\$ I know :/. I'm hoping they remember at some point. Don't worry, every once in awhile I go over my posts and accept any outstanding questions. \$\endgroup\$ – Carcigenicate Jan 8 '16 at 22:10
  • \$\begingroup\$ Sorry, I forgot. As promised though, I checked back! Thanks again. \$\endgroup\$ – Carcigenicate Feb 12 '16 at 0:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.