11
\$\begingroup\$

I started learning Python a couple of weeks ago, and to test my new knowledge of Python, I decided to use Pygame to code a random walk terrain generator.

# Basic random walker program
import random
import pygame
import sys


# Screen width and height
WIDTH = 900
HEIGHT = 900


# Initalize pygame
pygame.init()
screen = pygame.display.set_mode((WIDTH, HEIGHT))
pygame.display.set_caption("Random Walker")


# Tile size
TILE_SIZE = 10


# Tile image variables
VILLAGE = pygame.transform.scale(pygame.image.load("village.png"), (TILE_SIZE, TILE_SIZE))
GRASS = pygame.transform.scale(pygame.image.load("grass.png"), (TILE_SIZE, TILE_SIZE))
WATER = pygame.transform.scale(pygame.image.load("water.png"), (TILE_SIZE, TILE_SIZE))
SAND = pygame.transform.scale(pygame.image.load("sand.png"), (TILE_SIZE, TILE_SIZE))
TREE = pygame.transform.scale(pygame.image.load("tree.png"), (TILE_SIZE, TILE_SIZE))


# World generator class, generates the world
class WorldGenerator(object):
    def __init__(self):
        self.WORLD_SIZE = 90
        self.world = [[WATER for _ in range(self.WORLD_SIZE)] for _ in range(self.WORLD_SIZE)]
        self.walker_x = random.randint(15, 25)
        self.walker_y = random.randint(15, 25)

    # Generate a grass sections of the world
    def generate_grass(self):
        for _ in range(random.randint(700, 800)):

            # Create a new grass tile
            try: self.world[self.walker_y][self.walker_x] = GRASS
            except IndexError: break

            # Change the walker position
            self.walker_x += random.choice([-1, -1, -1, 0, 1, 1, 1])
            self.walker_y += random.choice([-1, -1, -1, 0, 1, 1, 1])


    # Generate a beach section of the world
    def generate_beaches(self):
        for y in range(self.WORLD_SIZE):
            for x in range(self.WORLD_SIZE):

                # Add a new sand tile
                try:
                    add_sand = random.choice([True, True, False, False, False])
                    if add_sand and self.world[y][x] == WATER and (self.world[y+1][x] == GRASS or self.world[y][x+1] == GRASS or self.world[y+1][x+1] == GRASS or self.world[y-1][x] == GRASS or self.world[y][x-1] == GRASS or self.world[y-1][x-1] == GRASS):
                        self.world[y][x] = SAND
                except IndexError: pass


    # Generate trees in the world
    def generate_trees(self):
        for y in range(self.WORLD_SIZE):
            for x in range(self.WORLD_SIZE):

                # Randomly add the tree tile
                add_tree = random.randint(1, 4) < random.randint(1, 4) < random.randint(1, 4)
                if add_tree and self.world[y][x] == GRASS:
                    self.world[y][x] = TREE

    # Add "villages" to the world
    def generate_villages(self):
        for y in range(self.WORLD_SIZE):
            for x in range(self.WORLD_SIZE):

                # Randomly add a village tile
                try:
                    add_village = random.choice([False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, True])
                    if add_village and self.world[y][x] == GRASS:
                        self.world[y][x] = VILLAGE
                except IndexError: pass

    # Generate the final world
    def generate_world(self):
        for _ in range(random.randint(5, 10)):
            self.generate_grass()
            self.generate_beaches()
            self.generate_trees()
            self.generate_villages()

    # Render the completed world
    def render_world(self):
        for y in range(self.WORLD_SIZE):
            for x in range(self.WORLD_SIZE):
                screen.blit(self.world[y][x], (x*TILE_SIZE, y*TILE_SIZE))


# Main program loop
def main_program_loop():
    world_generator = WorldGenerator()
    world_generator.generate_world()
    world_generator.render_world()

    # Run forever and see when user wants to quit
    while True:
        pygame.display.update()
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                pygame.quit()
                sys.exit(0)


# Run the program
if __name__ == "__main__":
    main_program_loop()                    

I'm not particularly proud of line 81

add_village = random.choice([False, False, False, False, False, …, True])

so I'd like any suggestions on what to do there. Anywhere else I can improve is welcome.

\$\endgroup\$
  • \$\begingroup\$ How many False are there? I don't really want to count. \$\endgroup\$ – TheCoffeeCup Dec 23 '14 at 17:32
  • 1
    \$\begingroup\$ @MannyMeng Well, print len([False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, True])-2 Would work... \$\endgroup\$ – Ethan Bierlein Dec 23 '14 at 17:57
  • \$\begingroup\$ This is one interesting program, could you add an example output so that we can see what it produces? \$\endgroup\$ – Simon Forsberg Jan 3 '15 at 13:05
  • \$\begingroup\$ @SimonAndréForsberg I'll try, it'll be hard to do though since the program is on another computer. \$\endgroup\$ – Ethan Bierlein Jan 5 '15 at 2:11
12
\$\begingroup\$

Extract repeated code into functions. Loading the tile images is the same operation with a different file name.

def load_scaled_image(file_name):
    return pygame.transform.scale(pygame.image.load(file_name), (TILE_SIZE, TILE_SIZE))

VILLAGE = load_scaled_image("village.png")
GRASS = load_scaled_image("grass.png")
WATER = load_scaled_image("water.png")
SAND = load_scaled_image("sand.png")
TREE = load_scaled_image("tree.png")

Extract the configuration parameters into constants. You do it properly with TILE_SIZE and WORLD_SIZE. But it stops there. The range of locations for the walker. The range of possible number of steps from walker to add grass. All of these are parameters that can be isolated away from the code so that they are more easily adjusted.

Since there are some many parameters that go into configuring world generation, it makes sense to group them in a single data structure. The structure just contains the require values and nothing else. Your world generation code can take this structure as an argument and generate any world based on the values. Currently, your code can only generate a world with the single set of parameters it is using. Even though none of the functionality changes, you have to dive into the generation functionality just to adjust the likelihood that some operation happens.


Don't manually create your weighted random selection inputs. You knew this was bad, but yet it is still there. A quick search brought up a number of results for ways to do this in a configurable manor. This question has a number of answers that can be used as functions to perform this operation.

The worst part of doing it manually is that it is extremely hard to read and ensure that you did it correctly. The comments brought up this point. There could be 10 Falses, or there could be 100. This makes a big difference, but makes a reader work very hard to find the answer.


You have a number of functions that all iterate over the whole world. Instead, you can write small functions that take in coordinates as arguments. Then they perform the one operation needed to add a specific tile type. Then you have one double for loop that calls each tile function. This will make it easier to read, more efficient and easier to test.

\$\endgroup\$
  • 2
    \$\begingroup\$ Line 81 is what I would call unholy sampling. \$\endgroup\$ – 200_success Dec 23 '14 at 21:02
10
\$\begingroup\$

This is a short answer to your question about line 81. Instead of all the False, you can do this:

add_village = random.randint(1, n) == 1;

where n is how many false you have.

random.randint(1, n) generates a number between 1 and n, and == 1 checks if it is one. It has a 1 in n chance of returning true, just like what you have. This increases the efficiency and is much less of a pain to type.

\$\endgroup\$
4
\$\begingroup\$

Your first comment should be a docstring:

"""
Basic random walker program
"""

Preferably, it should also be fleshed out with useful information.

Many of your other comments are mostly useless:

  • # Screen width and height: already said by the variable names.
  • # Initalize pygame: it's called pygame.init so this is obvious.
  • # Tile size: as above.
  • # Tile image variables: if it's not clear by the name, use better names.
  • # World generator class, generates the world: as above.

and many others. Some are useful, though, such as # Add a new sand tile (although that can be removed by just improving the code).

Comments above functions like

# This does foo to bar
def foo():
    ...

should be turned into docstrings:

def foo():
    """
    This does foo to bar.
    """
    ...

You should order your imports.

main_program_loop should just be called main by convention.

screen and the tiles being loaded should be done inside a local scope like main.

self.WORLD_SIZE shouldn't be in capitals.

The tiles (VILLAGE, GRASS, WATER, ...) should be encapsulated into a Tiles or Resources class. This makes it simple to pass around handles to resources without making them globals.

You repeat

pygame.transform.scale(pygame.image.load("tree.png"), (TILE_SIZE, TILE_SIZE))

a lot; you should probably encapsulate this.

It makes sense to move TILE_SIZE into the resource manager.

WIDTH and HEIGHT don't really need to be global; they would be fine as arguments to main:

def main(width, height):
    ...

if __name__ == "__main__":
    main(width=900, height=900)

pygame.init() doesn't actually need to be called. Personally I prefer loading subsystems explicitly when it's shown I need them rather than the catch-all pygame.init. In this case you can just get rid of the call.

WorldGenerator should take world_size as an argument rather than hard-coding it. It should be calculated from the screen size.

Both world_size and tile_size should be split into _height and _width variants.

self.world can be initialized with

self.world = [[self.tiles.water] * self.world_width for _ in range(self.world_height)]

since self.tiles.water isn't generated each access.

You should make walker_x and walker_y local variables to generate_grass, not instance variables.

Don't write:

    try: self.world[walker_y][walker_x] = self.tiles.grass
    except IndexError: break

There's no shortage of newlines, so go ahead and use them. The break would probably be better as return.

Your random.choices in this function can be replaced with

if random.random() < 6/7.:
    walker_x += random.choice([-1, +1])

if random.random() < 6/7.:
    walker_y += random.choice([-1, +1])

IMHO, this is more explicit about what the probabilities are. I would also do from __future__ import division and remove the decimal point.

Your

try:
    self.world[walker_y][walker_x] = self.tiles.grass
except IndexError:
    return

also allows walker to go negative because Python makes x[-1] refer to the last element in the list. Although this is cool, it's inconsistent with how it acts in different directions. I would just use:

self.world[walker_y % self.world_width][walker_x % self.world_height] = self.tiles.grass

Similar things are true for the other functions, so I'd make an auxillary getter-setter to make this easier.

def get(self, x, y):
    return self.world[y % self.world_width][x % self.world_height]

def set(self, x, y, value):
    self.world[y % self.world_width][x % self.world_height] = value

I would also change

walker_x = random.randint(15, 25)
walker_y = random.randint(15, 25)

to

walker_x = random.randrange(self.world_width)
walker_y = random.randrange(self.world_height)

For generate_beaches, first wrap the line and then move to the getter-setter. Use early exit to split up the checks. Make a list of surrounding tiles and check if self.tiles.grass in surroundings. Short-circuit with any.

In generate_trees, you have

random.randint(1, 4) < random.randint(1, 4) < random.randint(1, 4)

This is a really, really wierd thing to do. A much more effective method is

random.random() < 1/16

which is the same probability.

For generate_villages a similar problem occurs. Just use a probability of 1/79.

For generate_world, you run things 5 to 10 times. Since walker_x and walker_y aren't instance variables, you will then have to feed this back to the function.

For main, it's better to just return than run sys.exit. It's also better not to do a busy loop while waiting for input. Use pygame.event.wait to keep CPU costs low.

The code would end up like:

"""
Basic random walker program
"""

from __future__ import division

import pygame
import random
import sys

class Tiles(object):
    def __init__(self, tile_width, tile_height):
        self.tile_width = tile_width
        self.tile_height = tile_height
        self.tile_shape = tile_width, tile_height

    def make_blank(self, color):
        tile = pygame.Surface(self.tile_shape)
        tile.fill(pygame.Color(color))
        return tile

    def image_from_filesystem(self, image):
        raw_image = pygame.image.load(image)
        return pygame.transform.scale(raw_image, self.tile_shape)

    def load_from_filesystem(self):
        self.village = self.image_from_filesystem("village.png")
        self.grass   = self.image_from_filesystem("grass.png")
        self.water   = self.image_from_filesystem("water.png")
        self.sand    = self.image_from_filesystem("sand.png")
        self.tree    = self.image_from_filesystem("tree.png")

    def load_defaults(self):
        self.village = self.make_blank("brown")
        self.grass   = self.make_blank("green")
        self.water   = self.make_blank("blue")
        self.sand    = self.make_blank("orange")
        self.tree    = self.make_blank("grey")


class WorldGenerator(object):
    def __init__(self, tiles, world_width, world_height):
        self.tiles = tiles
        self.world_width = world_width
        self.world_height = world_height
        self.world = [[self.tiles.water] * self.world_width for _ in range(self.world_height)]

    def get(self, x, y):
        return self.world[y % self.world_width][x % self.world_height]

    def set(self, x, y, value):
        self.world[y % self.world_width][x % self.world_height] = value

    def generate_grass(self, walker_x, walker_y):
        for _ in range(random.randint(700, 800)):
            self.set(walker_x, walker_y, self.tiles.grass)

            if random.random() < 6/7:
                walker_x += random.choice([-1, +1])

            if random.random() < 6/7:
                walker_y += random.choice([-1, +1])

        return walker_x, walker_y

    def generate_beaches(self):
        deltas = {
            (-1, -1), (-1,  0), (-1, +1),
            ( 0, -1),           ( 0, +1),
            (+1, -1), (+1,  0), (+1, -1)
        }

        for y in range(self.world_height):
            for x in range(self.world_width):
                if random.random() < 3/5:
                    continue

                if self.get(x, y) != self.tiles.water:
                    continue

                if any(self.get(x+dx, y+dy) == self.tiles.grass for dx, dy in deltas):
                    self.set(x, y, self.tiles.sand)

    def generate_trees(self):
        for y in range(self.world_height):
            for x in range(self.world_width):
                if random.random() < 15/16:
                    continue

                if self.get(x, y) == self.tiles.grass:
                    self.set(x, y, self.tiles.tree)

    def generate_villages(self):
        for y in range(self.world_height):
            for x in range(self.world_width):
                if random.random() < 78/79:
                    continue

                if self.get(x, y) == self.tiles.grass:
                    self.set(x, y, self.tiles.village)

    def generate_world(self):
        grass_gen = random.randrange(self.world_width), random.randrange(self.world_height)

        for _ in range(random.randint(5, 10)):
            grass_gen = self.generate_grass(*grass_gen)
            self.generate_beaches()
            self.generate_trees()
            self.generate_villages()

    def render_world(self, screen):
        for y in range(self.world_height):
            for x in range(self.world_width):
                screen.blit(self.world[y][x], (x*self.tiles.tile_width, y*self.tiles.tile_height))


def main(width, height):
    screen = pygame.display.set_mode((width, height))
    pygame.display.set_caption("Random Walker")

    tiles = Tiles(tile_width=10, tile_height=10)

    try:
        tiles.load_from_filesystem()
    except pygame.error:
        tiles.load_defaults()

    world_generator = WorldGenerator(tiles, width//tiles.tile_width, height//tiles.tile_height)
    world_generator.generate_world()
    world_generator.render_world(screen)

    # Run forever and see when user wants to quit
    while True:
        pygame.display.update()

        event = pygame.event.wait()
        if event.type == pygame.QUIT:
            pygame.quit()
            sys.exit(0)


if __name__ == "__main__":
    main(width=900, height=900)

Two more things to consider are:

  • Do your generate_trees and generate_villages really need to be run several times? Isn't it better just to run them once with a higher probability at the end?

  • Shouldn't you use random selection rather than random filtering for generating the trees and villages? That would prevent you needing to visit all of the tiles.

\$\endgroup\$

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.