0
\$\begingroup\$

I am creating a tile-map in python using pygame. The code I have (see below) works OK, but I was wandering if there were any improvements.

This code creates a tilemap and scrolls it horizontally:

from enum import Enum

import pygame
pygame.init()

SCREEN_WIDTH = 800
SCREEN_HEIGHT = 500

TILE_SIZE = 50


class TileTypes(Enum):
    AIR = 0
    GRASS = 1
    ROCK = 2
    LAVA = 3


screen = pygame.display.set_mode((SCREEN_WIDTH, SCREEN_HEIGHT))

screen.fill((255,) * 3)
clock = pygame.time.Clock()

MAP_WIDTH = SCREEN_WIDTH // TILE_SIZE
MAP_HEIGHT = SCREEN_HEIGHT // TILE_SIZE

map_layout = [[TileTypes.AIR for j in range(MAP_WIDTH)]
              for i in range(MAP_HEIGHT)]


def draw_map():
    y = 0
    for row in map_layout:
        x = 0
        for tile in row:
            match tile:
                case TileTypes.AIR:
                    pass

                case TileTypes.GRASS:
                    pygame.draw.rect(screen, (0, 255, 0), (x, y, TILE_SIZE, TILE_SIZE))

                case TileTypes.ROCK:
                    pygame.draw.rect(screen, (150, 150, 150), (x, y, TILE_SIZE, TILE_SIZE))

            x += TILE_SIZE

        y += TILE_SIZE


def scroll_map():
    for row in map_layout:
        row.append(row.pop(0))

map_layout[-2] = map_layout[-1] = [TileTypes.ROCK for i in range(MAP_WIDTH)]
map_layout[-3] = [TileTypes.GRASS for i in range(MAP_WIDTH)]
map_layout[-4][2:4] = map_layout[-4][6:8] = [TileTypes.GRASS for i in range(2)]


done = False


while not done:
    if any(event.type == pygame.QUIT for event in pygame.event.get()):
        done = True

    scroll_map()

    screen.fill((255,) * 3)
    draw_map()

    pygame.display.flip()

    clock.tick(20)

pygame.quit()

tilemap

I would be grateful for any improvements.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ thank you for pointing this out to me. I have now fixed the code so that the tilemap scrolls horizontally and made the question about general, not specific improvements, is it on topic now? \$\endgroup\$ Apr 6, 2023 at 13:50
  • \$\begingroup\$ Thank you. Yes, it appears to be on-topic and I have re-opened it. \$\endgroup\$ Apr 6, 2023 at 16:06

1 Answer 1

1
\$\begingroup\$

I like this code. It accomplishes a simple goal in a simple way, very direct and clear.

Conforming to pep8 and using Enum is very nice.

It reminds me of POKE -16298, 0 for LORES graphic mode.

Let's examine low level details first.


__main__ guard

pygame.init()
...
screen = pygame.display.set_mode((SCREEN_WIDTH, SCREEN_HEIGHT))
screen.fill((255,) * 3)

I'm not crazy about doing a bunch of stuff at top level. Prefer to protect such statements with a main guard. The standard idiom is:

if __name__ == "__main__":
    pygame.init()
    screen = pygame.display.set_mode((SCREEN_WIDTH, SCREEN_HEIGHT))

Why do we do this? Well, I understand that no file has an import scrolling_game statement, yet. But we anticipate that could happen in future. For example if you write a unit test or use a couple of modules to implement a fancier game.

The idea is that another file should be able to import this one with no side effects, e.g. print() statements or game windows opening. So class / function definitions are fair game, plus constants like width / height. But the meat of the application typically goes into def main(): which we run according to what the module __name__ is. Notice that, for someone importing this module, the module name will look like "scrolling_game" rather than "__main__".

That third statement, which blanks out the screen with white, appears to be leftover from initial development / debug, and should just be deleted. Once the main display loop begins it becomes superfluous.

And defining a WHITE manifest constant wouldn't hurt.


cells dictate the display

def draw_map():
            ...
            match tile:
                case TileTypes.AIR:
                    pass
while not done:
    ...
    screen.fill((255,) * 3)  # <-- recommend you delete this
    draw_map()

I'm not keen on the pass, there. Prefer to repaint every tile, every time, rather than defaulting all tiles to AIR followed by selective repaint of non-AIR tiles. The current code makes sense, but I will introduce an alternate repainting strategy below.

Also, the match works out fine. But it would be better to put a color attribute on each Enum value, so we could unconditionally do lookup + render.


use an array

map_layout = [[TileTypes.AIR for j in range(MAP_WIDTH)]
              for i in range(MAP_HEIGHT)]

In Lisp and in Python a list-of-lists datastructure can be a very nice representation of N items that are "all the same thing", like tiles. But it suffers from chasing N object pointers, which aren't cache friendly. CPU hardware is good at predicting and prefetching sequential reads, but less so for random reads.

Prefer to represent N tiles with a vector, perhaps an array or an ndarray. That leaves us with something like

map_layout = np.ones((MAP_WIDTH, MAP_HEIGHT)) * TileTypes.AIR

slice when scrolling

A central concern is how to efficiently move the grid by one tile in any direction.

Let's start with a level that spans several screens:

level = np.ones((5 * MAP_WIDTH, 3 * MAP_HEIGHT)) * TileTypes.AIR

Pure AIR would be boring, so fill in additional level tiles to taste.

Now let's render a portion of that level:

x, y = get_current_position()
map_layout = level[x : x+MAP_WIDTH][y : y+MAP_HEIGHT]

We can advance x or y by +/- 1 pixel and re-render to scroll to a new spot within the level. Rather than interpreted bytecode we are looping in C, so it's essentially MAP_HEIGHT memcpy()'s per screen refresh, with no random reads from chasing object pointers.

If all levels have similar dimensions, then you could move from two dimensions to a three-dimensional array: level[lvl][x][y]


This is solid code. I would be happy to delegate or accept maintenance tasks for it.

Recommend the use of array slicing for frame updates.

\$\endgroup\$
2
  • \$\begingroup\$ np.ones((MAP_WIDTH, MAP_HEIGHT)) * TileTypes.AIR results in an error: TypeError: unsupported operand type(s) for +: 'float' and 'TileTypes. Wouldn't something like np.full() work better? \$\endgroup\$ Apr 7, 2023 at 10:34
  • \$\begingroup\$ Agreed. Though we'll still want AIR (or AIR.value) to give us a small integer, even if used as fill_value for .full(). Plus AIR.color should give us an RGB white value, GRASS.color should give a green triple, and so on, which is easily arranged, so no need for a separate enum_to_color dict. \$\endgroup\$
    – J_H
    Apr 7, 2023 at 15:25

This site is temporarily in read-only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .