Movement function for a Sokoban game using Python [closed]
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
4
down vote
favorite
I've been working on the functions that handle when a player's sprite interacts with either a wall (0) or say the crate to push (ã)
I'm just wondering how this looks and if there alternative methods, as I've been told I can make things more complicated than they really need to be.
def MOVE_UP():
x = MapGrid.getCharLocation(PlayerMan.getRow() - 1, PlayerMan.getCol())
y = MapGrid.getCharLocation(Boxes.getRow() - 1, Boxes.getCol())
if x == '0':
pass
elif x == 'ã':
if y == '0':
pass
else:
MapGrid.despawn(Boxes.getRow(), Boxes.getCol())
Boxes.pushUp()
MapGrid.spawnBoxes(Boxes.getChar(), Boxes.getRow(), Boxes.getCol())
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(Boxes.displayOnScreen())
else:
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(MapGrid.displayOnScreen())
print(PlayerMan.displayOnScreen())
This code, in Python 3.6, is repeated for the other three directions.
python object-oriented python-3.x game pygame
closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 at 4:15
This question appears to be off-topic. The users who voted to close gave this specific reason:
- "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." â Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian
add a comment |Â
up vote
4
down vote
favorite
I've been working on the functions that handle when a player's sprite interacts with either a wall (0) or say the crate to push (ã)
I'm just wondering how this looks and if there alternative methods, as I've been told I can make things more complicated than they really need to be.
def MOVE_UP():
x = MapGrid.getCharLocation(PlayerMan.getRow() - 1, PlayerMan.getCol())
y = MapGrid.getCharLocation(Boxes.getRow() - 1, Boxes.getCol())
if x == '0':
pass
elif x == 'ã':
if y == '0':
pass
else:
MapGrid.despawn(Boxes.getRow(), Boxes.getCol())
Boxes.pushUp()
MapGrid.spawnBoxes(Boxes.getChar(), Boxes.getRow(), Boxes.getCol())
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(Boxes.displayOnScreen())
else:
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(MapGrid.displayOnScreen())
print(PlayerMan.displayOnScreen())
This code, in Python 3.6, is repeated for the other three directions.
python object-oriented python-3.x game pygame
closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 at 4:15
This question appears to be off-topic. The users who voted to close gave this specific reason:
- "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." â Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian
1
what isgetCharLocation
,PlayerMan
,Boxes
. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely.
â Maarten Fabré
Jun 8 at 12:02
also checking out how@property
works will make your code cleaner
â Maarten Fabré
Jun 8 at 12:10
you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
â Maarten Fabré
Jun 8 at 12:12
add a comment |Â
up vote
4
down vote
favorite
up vote
4
down vote
favorite
I've been working on the functions that handle when a player's sprite interacts with either a wall (0) or say the crate to push (ã)
I'm just wondering how this looks and if there alternative methods, as I've been told I can make things more complicated than they really need to be.
def MOVE_UP():
x = MapGrid.getCharLocation(PlayerMan.getRow() - 1, PlayerMan.getCol())
y = MapGrid.getCharLocation(Boxes.getRow() - 1, Boxes.getCol())
if x == '0':
pass
elif x == 'ã':
if y == '0':
pass
else:
MapGrid.despawn(Boxes.getRow(), Boxes.getCol())
Boxes.pushUp()
MapGrid.spawnBoxes(Boxes.getChar(), Boxes.getRow(), Boxes.getCol())
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(Boxes.displayOnScreen())
else:
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(MapGrid.displayOnScreen())
print(PlayerMan.displayOnScreen())
This code, in Python 3.6, is repeated for the other three directions.
python object-oriented python-3.x game pygame
I've been working on the functions that handle when a player's sprite interacts with either a wall (0) or say the crate to push (ã)
I'm just wondering how this looks and if there alternative methods, as I've been told I can make things more complicated than they really need to be.
def MOVE_UP():
x = MapGrid.getCharLocation(PlayerMan.getRow() - 1, PlayerMan.getCol())
y = MapGrid.getCharLocation(Boxes.getRow() - 1, Boxes.getCol())
if x == '0':
pass
elif x == 'ã':
if y == '0':
pass
else:
MapGrid.despawn(Boxes.getRow(), Boxes.getCol())
Boxes.pushUp()
MapGrid.spawnBoxes(Boxes.getChar(), Boxes.getRow(), Boxes.getCol())
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(Boxes.displayOnScreen())
else:
MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
PlayerMan.MOVE_UP()
MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())
print(MapGrid.displayOnScreen())
print(PlayerMan.displayOnScreen())
This code, in Python 3.6, is repeated for the other three directions.
python object-oriented python-3.x game pygame
edited Jun 7 at 21:32
Phrancis
14.6k644137
14.6k644137
asked Jun 7 at 20:52
James D
211
211
closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 at 4:15
This question appears to be off-topic. The users who voted to close gave this specific reason:
- "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." â Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian
closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 at 4:15
This question appears to be off-topic. The users who voted to close gave this specific reason:
- "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." â Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian
1
what isgetCharLocation
,PlayerMan
,Boxes
. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely.
â Maarten Fabré
Jun 8 at 12:02
also checking out how@property
works will make your code cleaner
â Maarten Fabré
Jun 8 at 12:10
you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
â Maarten Fabré
Jun 8 at 12:12
add a comment |Â
1
what isgetCharLocation
,PlayerMan
,Boxes
. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely.
â Maarten Fabré
Jun 8 at 12:02
also checking out how@property
works will make your code cleaner
â Maarten Fabré
Jun 8 at 12:10
you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
â Maarten Fabré
Jun 8 at 12:12
1
1
what is
getCharLocation
, PlayerMan
, Boxes
. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely.â Maarten Fabré
Jun 8 at 12:02
what is
getCharLocation
, PlayerMan
, Boxes
. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely.â Maarten Fabré
Jun 8 at 12:02
also checking out how
@property
works will make your code cleanerâ Maarten Fabré
Jun 8 at 12:10
also checking out how
@property
works will make your code cleanerâ Maarten Fabré
Jun 8 at 12:10
you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
â Maarten Fabré
Jun 8 at 12:12
you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
â Maarten Fabré
Jun 8 at 12:12
add a comment |Â
1 Answer
1
active
oldest
votes
up vote
2
down vote
strings
I seems your grid is a nested list of strings. It would be clearer, if you change that to Enum
s
from enum import Enum
class Tiles(Enum):
WALL = '0'
PLAYER = "*"
BOX = 'ã'
EMPTY = '.'
so you can do something like this:
w = Tiles.WALL
e = Tiles.EMPTY
b = Tiles.BOX
p = Tiles.PLAYER
grid = [
[w,w,w,w,w],
[w,e,e,e,w],
[w,e,e,b,w],
[w,b,b,p,w],
[w,e,e,b,w],
[w,e,e,b,w],
[w,e,e,e,w],
[w,w,w,w,w],
]
getter and setters
Python does not have the Java tradition of setters and getters. If you want to work with a calculated property, of protect a property, you can use @property
. This can work like this
class Boxes:
def __init__(self, row, ...):
self._row = row
...
@property
def row(self):
return self._row
and so on for char and column
double work
The position of the Boxes and the player is kept both in the MapGrid
, and in PlayerMan
and Boxes
. This is double work, and prone to errors.
Now to move something, you have to adapt it both in the grid (.despawn
and .spawnXXX
and on the PlayerMan
and Boxes
, resulting in a lot of unnecessary code.
All you really need to capture the state of the board is this:
class SokobanGrid():
def __init__(self, grid, position, strength=None):
grid = list(map(list, grid)) # make a copy of the grid
assert grid[position[0]][position[1]] == Tiles.PLAYER
num_players = sum(
sum(tile == Tiles.PLAYER for tile in row)
for row in grid
)
assert num_players == 1
self._grid = grid
self._dimensions = len(grid), len(grid[0])
self.position = position
self.strength = strength
def copy(self):
return SokobanGrid(self._grid, position=self.position)
displayOnScreen()
instead of making a method that returns a string representation of the board, you can define __str__
and/or __repr__
def __str__(self):
rows = (
f'xidx: ' + ''.join(tile.value for tile in row)
for idx, row in enumerate(self._grid)
)
header = ' ' * 4 + ''.join(map(str, range(self._dimensions[1]))) + 'n'
return header + 'n'.join(rows)
you can omit the position
parameter, and go look for it yourself.
g = SokobanGrid(grid=grid, position=(3, 3))
print(g)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã*0
x4: 0..ã0
x5: 0..ã0
x6: 0...0
x7: 00000
The interactions with the tile then go like this:
def tile(self, x, y):
return self._grid[x][y]
def iswall(self, x, y):
return self.tile(x, y) == Tiles.WALL
def isbox(self, x, y):
return self.tile(x, y) == Tiles.BOX
def isempty(self, x, y):
return self.tile(x, y) == Tiles.EMPTY
def set_tile(self, x, y, tile):
self._grid[x][y] = tile
def swap_tiles(self, old, new):
old_tile = self.tile(*old)
new_tile = self.tile(*new)
self.set_tile(*old, new_tile)
self.set_tile(*new, old_tile)
moving
Instead of a move method for each direction, you can make a generic one, that accepts a direction. The directions can be an Enum
, with a value the change in coordinates it entails (UP
is (-1, 0)
, because row 0 is the top row)
class Direction(Enum):
UP = (-1, 0)
DOWN = (1, 0)
LEFT = (0, -1)
RIGHT = (0, 1)
The first helper method yields all the moves in a certain direction until it hits a wall (not included) or an empty spot (included)
def _moves(self, Direction):
x, y, = self.position
x_max, y_max = self._dimensions
dx, dy = Direction.value
while 0 <= x < x_max and 0 <= y < y_max:
if self.iswall(x, y):
return
yield x, y
if self.isempty(x, y):
return
x, y = x + dx, y + dy
the second helper method checks whether the player can move in a certain direction:
def can_move(self, direction):
for boxes, position in enumerate(self._moves(direction)):
if self.isempty(*position):
return True
if self.iswall(*position):
return False
if self.strength and boxes > self.strength:
return False
return False
This raises a class IllegalMove(Exception): pass
to signal to the user interface that a move is not allowed. You might use a ValueError
instead
This can be tested with
d = Direction.DOWN
u = Direction.UP
l = Direction.LEFT
r = Direction.RIGHT
assert g.can_move(u)
assert g.can_move(d)
assert not g.can_move(l)
assert not g.can_move(r)
The meaning of the strength parameter, is the maximum amount of boxes the player can push. If we set this to 1, it should not be able to push down anymore:
g_1 = g.copy()
g_1.strength = 1
assert g_1.can_move(u)
assert not g_1.can_move(d)
assert not g_1.can_move(l)
assert not g_1.can_move(r)
Now to move, we must start from the first empty tile to the player, and swap the tiles piece by piece
def move(self, direction):
if not self.can_move(direction):
raise IllegalMove
moves = self._moves(direction)
moves = reversed(list(moves))
for old, new in pairwise(moves):
self.swap_tiles(old, new)
pairwise
is from the itertools recipes
from itertools import tee, takewhile
def pairwise(iterable):
"s -> (s0,s1), (s1,s2), (s2, s3), ..."
a, b = tee(iterable)
next(b, None)
return zip(a, b)
so:
g_move = g.copy()
g_move.move(d)
print(g_move)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã.0
x4: 0..*0
x5: 0..ã0
x6: 0..ã0
x7: 00000
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
strings
I seems your grid is a nested list of strings. It would be clearer, if you change that to Enum
s
from enum import Enum
class Tiles(Enum):
WALL = '0'
PLAYER = "*"
BOX = 'ã'
EMPTY = '.'
so you can do something like this:
w = Tiles.WALL
e = Tiles.EMPTY
b = Tiles.BOX
p = Tiles.PLAYER
grid = [
[w,w,w,w,w],
[w,e,e,e,w],
[w,e,e,b,w],
[w,b,b,p,w],
[w,e,e,b,w],
[w,e,e,b,w],
[w,e,e,e,w],
[w,w,w,w,w],
]
getter and setters
Python does not have the Java tradition of setters and getters. If you want to work with a calculated property, of protect a property, you can use @property
. This can work like this
class Boxes:
def __init__(self, row, ...):
self._row = row
...
@property
def row(self):
return self._row
and so on for char and column
double work
The position of the Boxes and the player is kept both in the MapGrid
, and in PlayerMan
and Boxes
. This is double work, and prone to errors.
Now to move something, you have to adapt it both in the grid (.despawn
and .spawnXXX
and on the PlayerMan
and Boxes
, resulting in a lot of unnecessary code.
All you really need to capture the state of the board is this:
class SokobanGrid():
def __init__(self, grid, position, strength=None):
grid = list(map(list, grid)) # make a copy of the grid
assert grid[position[0]][position[1]] == Tiles.PLAYER
num_players = sum(
sum(tile == Tiles.PLAYER for tile in row)
for row in grid
)
assert num_players == 1
self._grid = grid
self._dimensions = len(grid), len(grid[0])
self.position = position
self.strength = strength
def copy(self):
return SokobanGrid(self._grid, position=self.position)
displayOnScreen()
instead of making a method that returns a string representation of the board, you can define __str__
and/or __repr__
def __str__(self):
rows = (
f'xidx: ' + ''.join(tile.value for tile in row)
for idx, row in enumerate(self._grid)
)
header = ' ' * 4 + ''.join(map(str, range(self._dimensions[1]))) + 'n'
return header + 'n'.join(rows)
you can omit the position
parameter, and go look for it yourself.
g = SokobanGrid(grid=grid, position=(3, 3))
print(g)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã*0
x4: 0..ã0
x5: 0..ã0
x6: 0...0
x7: 00000
The interactions with the tile then go like this:
def tile(self, x, y):
return self._grid[x][y]
def iswall(self, x, y):
return self.tile(x, y) == Tiles.WALL
def isbox(self, x, y):
return self.tile(x, y) == Tiles.BOX
def isempty(self, x, y):
return self.tile(x, y) == Tiles.EMPTY
def set_tile(self, x, y, tile):
self._grid[x][y] = tile
def swap_tiles(self, old, new):
old_tile = self.tile(*old)
new_tile = self.tile(*new)
self.set_tile(*old, new_tile)
self.set_tile(*new, old_tile)
moving
Instead of a move method for each direction, you can make a generic one, that accepts a direction. The directions can be an Enum
, with a value the change in coordinates it entails (UP
is (-1, 0)
, because row 0 is the top row)
class Direction(Enum):
UP = (-1, 0)
DOWN = (1, 0)
LEFT = (0, -1)
RIGHT = (0, 1)
The first helper method yields all the moves in a certain direction until it hits a wall (not included) or an empty spot (included)
def _moves(self, Direction):
x, y, = self.position
x_max, y_max = self._dimensions
dx, dy = Direction.value
while 0 <= x < x_max and 0 <= y < y_max:
if self.iswall(x, y):
return
yield x, y
if self.isempty(x, y):
return
x, y = x + dx, y + dy
the second helper method checks whether the player can move in a certain direction:
def can_move(self, direction):
for boxes, position in enumerate(self._moves(direction)):
if self.isempty(*position):
return True
if self.iswall(*position):
return False
if self.strength and boxes > self.strength:
return False
return False
This raises a class IllegalMove(Exception): pass
to signal to the user interface that a move is not allowed. You might use a ValueError
instead
This can be tested with
d = Direction.DOWN
u = Direction.UP
l = Direction.LEFT
r = Direction.RIGHT
assert g.can_move(u)
assert g.can_move(d)
assert not g.can_move(l)
assert not g.can_move(r)
The meaning of the strength parameter, is the maximum amount of boxes the player can push. If we set this to 1, it should not be able to push down anymore:
g_1 = g.copy()
g_1.strength = 1
assert g_1.can_move(u)
assert not g_1.can_move(d)
assert not g_1.can_move(l)
assert not g_1.can_move(r)
Now to move, we must start from the first empty tile to the player, and swap the tiles piece by piece
def move(self, direction):
if not self.can_move(direction):
raise IllegalMove
moves = self._moves(direction)
moves = reversed(list(moves))
for old, new in pairwise(moves):
self.swap_tiles(old, new)
pairwise
is from the itertools recipes
from itertools import tee, takewhile
def pairwise(iterable):
"s -> (s0,s1), (s1,s2), (s2, s3), ..."
a, b = tee(iterable)
next(b, None)
return zip(a, b)
so:
g_move = g.copy()
g_move.move(d)
print(g_move)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã.0
x4: 0..*0
x5: 0..ã0
x6: 0..ã0
x7: 00000
add a comment |Â
up vote
2
down vote
strings
I seems your grid is a nested list of strings. It would be clearer, if you change that to Enum
s
from enum import Enum
class Tiles(Enum):
WALL = '0'
PLAYER = "*"
BOX = 'ã'
EMPTY = '.'
so you can do something like this:
w = Tiles.WALL
e = Tiles.EMPTY
b = Tiles.BOX
p = Tiles.PLAYER
grid = [
[w,w,w,w,w],
[w,e,e,e,w],
[w,e,e,b,w],
[w,b,b,p,w],
[w,e,e,b,w],
[w,e,e,b,w],
[w,e,e,e,w],
[w,w,w,w,w],
]
getter and setters
Python does not have the Java tradition of setters and getters. If you want to work with a calculated property, of protect a property, you can use @property
. This can work like this
class Boxes:
def __init__(self, row, ...):
self._row = row
...
@property
def row(self):
return self._row
and so on for char and column
double work
The position of the Boxes and the player is kept both in the MapGrid
, and in PlayerMan
and Boxes
. This is double work, and prone to errors.
Now to move something, you have to adapt it both in the grid (.despawn
and .spawnXXX
and on the PlayerMan
and Boxes
, resulting in a lot of unnecessary code.
All you really need to capture the state of the board is this:
class SokobanGrid():
def __init__(self, grid, position, strength=None):
grid = list(map(list, grid)) # make a copy of the grid
assert grid[position[0]][position[1]] == Tiles.PLAYER
num_players = sum(
sum(tile == Tiles.PLAYER for tile in row)
for row in grid
)
assert num_players == 1
self._grid = grid
self._dimensions = len(grid), len(grid[0])
self.position = position
self.strength = strength
def copy(self):
return SokobanGrid(self._grid, position=self.position)
displayOnScreen()
instead of making a method that returns a string representation of the board, you can define __str__
and/or __repr__
def __str__(self):
rows = (
f'xidx: ' + ''.join(tile.value for tile in row)
for idx, row in enumerate(self._grid)
)
header = ' ' * 4 + ''.join(map(str, range(self._dimensions[1]))) + 'n'
return header + 'n'.join(rows)
you can omit the position
parameter, and go look for it yourself.
g = SokobanGrid(grid=grid, position=(3, 3))
print(g)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã*0
x4: 0..ã0
x5: 0..ã0
x6: 0...0
x7: 00000
The interactions with the tile then go like this:
def tile(self, x, y):
return self._grid[x][y]
def iswall(self, x, y):
return self.tile(x, y) == Tiles.WALL
def isbox(self, x, y):
return self.tile(x, y) == Tiles.BOX
def isempty(self, x, y):
return self.tile(x, y) == Tiles.EMPTY
def set_tile(self, x, y, tile):
self._grid[x][y] = tile
def swap_tiles(self, old, new):
old_tile = self.tile(*old)
new_tile = self.tile(*new)
self.set_tile(*old, new_tile)
self.set_tile(*new, old_tile)
moving
Instead of a move method for each direction, you can make a generic one, that accepts a direction. The directions can be an Enum
, with a value the change in coordinates it entails (UP
is (-1, 0)
, because row 0 is the top row)
class Direction(Enum):
UP = (-1, 0)
DOWN = (1, 0)
LEFT = (0, -1)
RIGHT = (0, 1)
The first helper method yields all the moves in a certain direction until it hits a wall (not included) or an empty spot (included)
def _moves(self, Direction):
x, y, = self.position
x_max, y_max = self._dimensions
dx, dy = Direction.value
while 0 <= x < x_max and 0 <= y < y_max:
if self.iswall(x, y):
return
yield x, y
if self.isempty(x, y):
return
x, y = x + dx, y + dy
the second helper method checks whether the player can move in a certain direction:
def can_move(self, direction):
for boxes, position in enumerate(self._moves(direction)):
if self.isempty(*position):
return True
if self.iswall(*position):
return False
if self.strength and boxes > self.strength:
return False
return False
This raises a class IllegalMove(Exception): pass
to signal to the user interface that a move is not allowed. You might use a ValueError
instead
This can be tested with
d = Direction.DOWN
u = Direction.UP
l = Direction.LEFT
r = Direction.RIGHT
assert g.can_move(u)
assert g.can_move(d)
assert not g.can_move(l)
assert not g.can_move(r)
The meaning of the strength parameter, is the maximum amount of boxes the player can push. If we set this to 1, it should not be able to push down anymore:
g_1 = g.copy()
g_1.strength = 1
assert g_1.can_move(u)
assert not g_1.can_move(d)
assert not g_1.can_move(l)
assert not g_1.can_move(r)
Now to move, we must start from the first empty tile to the player, and swap the tiles piece by piece
def move(self, direction):
if not self.can_move(direction):
raise IllegalMove
moves = self._moves(direction)
moves = reversed(list(moves))
for old, new in pairwise(moves):
self.swap_tiles(old, new)
pairwise
is from the itertools recipes
from itertools import tee, takewhile
def pairwise(iterable):
"s -> (s0,s1), (s1,s2), (s2, s3), ..."
a, b = tee(iterable)
next(b, None)
return zip(a, b)
so:
g_move = g.copy()
g_move.move(d)
print(g_move)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã.0
x4: 0..*0
x5: 0..ã0
x6: 0..ã0
x7: 00000
add a comment |Â
up vote
2
down vote
up vote
2
down vote
strings
I seems your grid is a nested list of strings. It would be clearer, if you change that to Enum
s
from enum import Enum
class Tiles(Enum):
WALL = '0'
PLAYER = "*"
BOX = 'ã'
EMPTY = '.'
so you can do something like this:
w = Tiles.WALL
e = Tiles.EMPTY
b = Tiles.BOX
p = Tiles.PLAYER
grid = [
[w,w,w,w,w],
[w,e,e,e,w],
[w,e,e,b,w],
[w,b,b,p,w],
[w,e,e,b,w],
[w,e,e,b,w],
[w,e,e,e,w],
[w,w,w,w,w],
]
getter and setters
Python does not have the Java tradition of setters and getters. If you want to work with a calculated property, of protect a property, you can use @property
. This can work like this
class Boxes:
def __init__(self, row, ...):
self._row = row
...
@property
def row(self):
return self._row
and so on for char and column
double work
The position of the Boxes and the player is kept both in the MapGrid
, and in PlayerMan
and Boxes
. This is double work, and prone to errors.
Now to move something, you have to adapt it both in the grid (.despawn
and .spawnXXX
and on the PlayerMan
and Boxes
, resulting in a lot of unnecessary code.
All you really need to capture the state of the board is this:
class SokobanGrid():
def __init__(self, grid, position, strength=None):
grid = list(map(list, grid)) # make a copy of the grid
assert grid[position[0]][position[1]] == Tiles.PLAYER
num_players = sum(
sum(tile == Tiles.PLAYER for tile in row)
for row in grid
)
assert num_players == 1
self._grid = grid
self._dimensions = len(grid), len(grid[0])
self.position = position
self.strength = strength
def copy(self):
return SokobanGrid(self._grid, position=self.position)
displayOnScreen()
instead of making a method that returns a string representation of the board, you can define __str__
and/or __repr__
def __str__(self):
rows = (
f'xidx: ' + ''.join(tile.value for tile in row)
for idx, row in enumerate(self._grid)
)
header = ' ' * 4 + ''.join(map(str, range(self._dimensions[1]))) + 'n'
return header + 'n'.join(rows)
you can omit the position
parameter, and go look for it yourself.
g = SokobanGrid(grid=grid, position=(3, 3))
print(g)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã*0
x4: 0..ã0
x5: 0..ã0
x6: 0...0
x7: 00000
The interactions with the tile then go like this:
def tile(self, x, y):
return self._grid[x][y]
def iswall(self, x, y):
return self.tile(x, y) == Tiles.WALL
def isbox(self, x, y):
return self.tile(x, y) == Tiles.BOX
def isempty(self, x, y):
return self.tile(x, y) == Tiles.EMPTY
def set_tile(self, x, y, tile):
self._grid[x][y] = tile
def swap_tiles(self, old, new):
old_tile = self.tile(*old)
new_tile = self.tile(*new)
self.set_tile(*old, new_tile)
self.set_tile(*new, old_tile)
moving
Instead of a move method for each direction, you can make a generic one, that accepts a direction. The directions can be an Enum
, with a value the change in coordinates it entails (UP
is (-1, 0)
, because row 0 is the top row)
class Direction(Enum):
UP = (-1, 0)
DOWN = (1, 0)
LEFT = (0, -1)
RIGHT = (0, 1)
The first helper method yields all the moves in a certain direction until it hits a wall (not included) or an empty spot (included)
def _moves(self, Direction):
x, y, = self.position
x_max, y_max = self._dimensions
dx, dy = Direction.value
while 0 <= x < x_max and 0 <= y < y_max:
if self.iswall(x, y):
return
yield x, y
if self.isempty(x, y):
return
x, y = x + dx, y + dy
the second helper method checks whether the player can move in a certain direction:
def can_move(self, direction):
for boxes, position in enumerate(self._moves(direction)):
if self.isempty(*position):
return True
if self.iswall(*position):
return False
if self.strength and boxes > self.strength:
return False
return False
This raises a class IllegalMove(Exception): pass
to signal to the user interface that a move is not allowed. You might use a ValueError
instead
This can be tested with
d = Direction.DOWN
u = Direction.UP
l = Direction.LEFT
r = Direction.RIGHT
assert g.can_move(u)
assert g.can_move(d)
assert not g.can_move(l)
assert not g.can_move(r)
The meaning of the strength parameter, is the maximum amount of boxes the player can push. If we set this to 1, it should not be able to push down anymore:
g_1 = g.copy()
g_1.strength = 1
assert g_1.can_move(u)
assert not g_1.can_move(d)
assert not g_1.can_move(l)
assert not g_1.can_move(r)
Now to move, we must start from the first empty tile to the player, and swap the tiles piece by piece
def move(self, direction):
if not self.can_move(direction):
raise IllegalMove
moves = self._moves(direction)
moves = reversed(list(moves))
for old, new in pairwise(moves):
self.swap_tiles(old, new)
pairwise
is from the itertools recipes
from itertools import tee, takewhile
def pairwise(iterable):
"s -> (s0,s1), (s1,s2), (s2, s3), ..."
a, b = tee(iterable)
next(b, None)
return zip(a, b)
so:
g_move = g.copy()
g_move.move(d)
print(g_move)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã.0
x4: 0..*0
x5: 0..ã0
x6: 0..ã0
x7: 00000
strings
I seems your grid is a nested list of strings. It would be clearer, if you change that to Enum
s
from enum import Enum
class Tiles(Enum):
WALL = '0'
PLAYER = "*"
BOX = 'ã'
EMPTY = '.'
so you can do something like this:
w = Tiles.WALL
e = Tiles.EMPTY
b = Tiles.BOX
p = Tiles.PLAYER
grid = [
[w,w,w,w,w],
[w,e,e,e,w],
[w,e,e,b,w],
[w,b,b,p,w],
[w,e,e,b,w],
[w,e,e,b,w],
[w,e,e,e,w],
[w,w,w,w,w],
]
getter and setters
Python does not have the Java tradition of setters and getters. If you want to work with a calculated property, of protect a property, you can use @property
. This can work like this
class Boxes:
def __init__(self, row, ...):
self._row = row
...
@property
def row(self):
return self._row
and so on for char and column
double work
The position of the Boxes and the player is kept both in the MapGrid
, and in PlayerMan
and Boxes
. This is double work, and prone to errors.
Now to move something, you have to adapt it both in the grid (.despawn
and .spawnXXX
and on the PlayerMan
and Boxes
, resulting in a lot of unnecessary code.
All you really need to capture the state of the board is this:
class SokobanGrid():
def __init__(self, grid, position, strength=None):
grid = list(map(list, grid)) # make a copy of the grid
assert grid[position[0]][position[1]] == Tiles.PLAYER
num_players = sum(
sum(tile == Tiles.PLAYER for tile in row)
for row in grid
)
assert num_players == 1
self._grid = grid
self._dimensions = len(grid), len(grid[0])
self.position = position
self.strength = strength
def copy(self):
return SokobanGrid(self._grid, position=self.position)
displayOnScreen()
instead of making a method that returns a string representation of the board, you can define __str__
and/or __repr__
def __str__(self):
rows = (
f'xidx: ' + ''.join(tile.value for tile in row)
for idx, row in enumerate(self._grid)
)
header = ' ' * 4 + ''.join(map(str, range(self._dimensions[1]))) + 'n'
return header + 'n'.join(rows)
you can omit the position
parameter, and go look for it yourself.
g = SokobanGrid(grid=grid, position=(3, 3))
print(g)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã*0
x4: 0..ã0
x5: 0..ã0
x6: 0...0
x7: 00000
The interactions with the tile then go like this:
def tile(self, x, y):
return self._grid[x][y]
def iswall(self, x, y):
return self.tile(x, y) == Tiles.WALL
def isbox(self, x, y):
return self.tile(x, y) == Tiles.BOX
def isempty(self, x, y):
return self.tile(x, y) == Tiles.EMPTY
def set_tile(self, x, y, tile):
self._grid[x][y] = tile
def swap_tiles(self, old, new):
old_tile = self.tile(*old)
new_tile = self.tile(*new)
self.set_tile(*old, new_tile)
self.set_tile(*new, old_tile)
moving
Instead of a move method for each direction, you can make a generic one, that accepts a direction. The directions can be an Enum
, with a value the change in coordinates it entails (UP
is (-1, 0)
, because row 0 is the top row)
class Direction(Enum):
UP = (-1, 0)
DOWN = (1, 0)
LEFT = (0, -1)
RIGHT = (0, 1)
The first helper method yields all the moves in a certain direction until it hits a wall (not included) or an empty spot (included)
def _moves(self, Direction):
x, y, = self.position
x_max, y_max = self._dimensions
dx, dy = Direction.value
while 0 <= x < x_max and 0 <= y < y_max:
if self.iswall(x, y):
return
yield x, y
if self.isempty(x, y):
return
x, y = x + dx, y + dy
the second helper method checks whether the player can move in a certain direction:
def can_move(self, direction):
for boxes, position in enumerate(self._moves(direction)):
if self.isempty(*position):
return True
if self.iswall(*position):
return False
if self.strength and boxes > self.strength:
return False
return False
This raises a class IllegalMove(Exception): pass
to signal to the user interface that a move is not allowed. You might use a ValueError
instead
This can be tested with
d = Direction.DOWN
u = Direction.UP
l = Direction.LEFT
r = Direction.RIGHT
assert g.can_move(u)
assert g.can_move(d)
assert not g.can_move(l)
assert not g.can_move(r)
The meaning of the strength parameter, is the maximum amount of boxes the player can push. If we set this to 1, it should not be able to push down anymore:
g_1 = g.copy()
g_1.strength = 1
assert g_1.can_move(u)
assert not g_1.can_move(d)
assert not g_1.can_move(l)
assert not g_1.can_move(r)
Now to move, we must start from the first empty tile to the player, and swap the tiles piece by piece
def move(self, direction):
if not self.can_move(direction):
raise IllegalMove
moves = self._moves(direction)
moves = reversed(list(moves))
for old, new in pairwise(moves):
self.swap_tiles(old, new)
pairwise
is from the itertools recipes
from itertools import tee, takewhile
def pairwise(iterable):
"s -> (s0,s1), (s1,s2), (s2, s3), ..."
a, b = tee(iterable)
next(b, None)
return zip(a, b)
so:
g_move = g.copy()
g_move.move(d)
print(g_move)
01234
x0: 00000
x1: 0...0
x2: 0..ã0
x3: 0ãã.0
x4: 0..*0
x5: 0..ã0
x6: 0..ã0
x7: 00000
edited Jun 8 at 14:20
answered Jun 8 at 14:11
Maarten Fabré
3,204214
3,204214
add a comment |Â
add a comment |Â
1
what is
getCharLocation
,PlayerMan
,Boxes
. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely.â Maarten Fabré
Jun 8 at 12:02
also checking out how
@property
works will make your code cleanerâ Maarten Fabré
Jun 8 at 12:10
you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
â Maarten Fabré
Jun 8 at 12:12