## **Screen resolution** ##
`SIZE = (1365, 705)` is plainly bad, users have different screen resolutions, setting a default resolution doesn't make sense at all, because on larger screens (i.e. 3840x2160) the window looks tiny, and users will want games to be full-screen. So it is better to get the user's screen resolution.
On Windows, use the following to get screen resolution:
```python
from win32api import GetSystemMetrics
RESOLUTION = tuple(map(GetSystemMetrics, (0, 1)))
```
Note this doesn't work on Linux, most users will probably be on Windows anyway, but if a user is using Linux and runs the program, it will raise exceptions. But of course, the Linux user can most probably fix the issue if you haven't fixed it.
## **Repetition `elif` ladder** ##
Your code has a lot of repetition, a lot of redundancy, you used the same expressions multiple times, have multiple if statements with similar conditions, et cetera, you should follow [Don't Repeat Yourself](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) principle and clean those up.
```python
if keys[pygame.K_UP]:
overworld.change_pos(pygame.Vector2(0, -MOVEMENT_SPEED))
if keys[pygame.K_DOWN]:
overworld.change_pos(pygame.Vector2(0, MOVEMENT_SPEED))
if keys[pygame.K_LEFT]:
overworld.change_pos(pygame.Vector2(-MOVEMENT_SPEED, 0))
if keys[pygame.K_RIGHT]:
overworld.change_pos(pygame.Vector2(MOVEMENT_SPEED, 0))
```
In the above code, each `if` statement is checking if a key is pressed and the action is extremely similar.
This is a perfect candidate to refactor to a `for` loop, you just need to store the keys in a sequence, and loop through that sequence to check if the key is pressed and do the action associated with the key. In this way, you only need to write one `if` statement.
You can get the keys by doing this:
```python
def get_key(key): return getattr(pygame, f"K_{key}")
KEYS = list(map(get_key, ("UP", "DOWN", "LEFT", "RIGHT")))
```
The keys all have a prefix `'K_'`, the second line loops through the key names and adds the prefix, then gets the value of the key name by getting the value of the attribute of `pygame` with the key as name by using `getattr`, and then assigns the result to the `KEYS` `list`.
Your movement vectors follow a clear pattern. They are of the form `(0, n)`. The sign of `n` is dependent on the direction of movement, and the order of the two numbers is dependent on the axis of movement. So you can just write a function to get the vectors.
```
from itertools import product
MOVEMENT_SPEED = 5
def get_vector(bools):
vertical, positive = bools
a, b = MOVEMENT_SPEED * (-1, 1)[positive], 0
if vertical:
a, b = b, a
return pygame.Vector2(a, b)
VECTORS = dict(zip(KEYS, map(get_vector, product((1, 0), (0, 1)))))
```
`product((1, 0), (0, 1))` gives `[(1, 0), (1, 1), (0, 0), (0, 1)]` which is then passed to `get_vector`, the arguments are the four `tuples` and they are unpacked, then the sign of the vector is set according to `positive` flag using indexing, and the order of the two numbers is swapped if `vertical` flag is set, finally the `pygame.Vector2` object is created and returned.
And then your code allows pressing opposite arrow keys at the same time. If both up and down or both left and right keys are pressed, the player is moved then moved back to the original position. This shouldn't be allowed and the second move should be invalidated.
Observe that opposite keys have indices in `KEYS` with opposite oddness, so we can detect if opposite keys are pressed by detecting if both odd index and even index is present.
```python
def handle_events():
for event in pygame.event.get():
if event.type == pygame.QUIT:
return True
keys = pygame.key.get_pressed()
exclusive = [False, False]
for i, key in enumerate(KEYS):
if keys[key]:
exclusive[i % 2] = True
if all(exclusive):
break
overworld.change_pos(VECTORS[key])
return False
```
## **`while` condition** ##
```python
def run_game():
while True:
if handle_events():
break
...
```
Don't do the above. `while True` is used to start infinite loops with no single predicate exit condition, and while it has legitimate uses, here it is not the case. You have one specific exit condition that breaks the loop, you should use that condition for the `while` condition, in this way the code is more concise and it is clearer when the loop should exit.
```
def run_game():
while not handle_events():
...
```
## **Making ranges** ##
```
self.x_range = range(0, self.size)
```
Don't do `range(0, n)`, just write `range(n)`, if you pass a single number as argument, Python knows that is the end of the `range`, and the start is by default 0 unless explicitly stated otherwise. It is more concise not to pass it and everyone knows what it means.
## ***Repetition yet again*** ##
```python
def update(self, block_pos, new_block=None):
if new_block == None:
self.surface.blit(self.backround_texture, pygame.Vector2(block_pos)*BLOCK_SIZE)
self.surface.blit(self.block_textures[self.blocks[block_pos]], pygame.Vector2(block_pos)*BLOCK_SIZE)
else:
self.blocks[block_pos] = new_block
self.surface.blit(self.backround_texture, pygame.Vector2(block_pos)*BLOCK_SIZE)
self.surface.blit(self.block_textures[new_block], pygame.Vector2(block_pos)*BLOCK_SIZE)
```
In the above code you used `pygame.Vector2(block_pos)*BLOCK_SIZE` four times, and `self.surface.blit(self.backround_texture, pygame.Vector2(block_pos)*BLOCK_SIZE)` twice, without the regard to the if statement.
`pygame.Vector2(block_pos)*BLOCK_SIZE` should be a named variable so that you don't have to recalculate it when you need to use it again, and `self.surface.blit(self.backround_texture, pygame.Vector2(block_pos)*BLOCK_SIZE)` should be moved outside of `if` `else` statements so that it will automatically executed without regard to the condition and you don't have to write it twice.
Then your second commands are very similar, the only difference is the key will be the default value if `new_block` is `None`, else the key will be `new_block` and the corresponding value will be updated.
You can just check the emptiness of the variable and assign it if non-empty in one-line by leveraging the `or` operator.
```
def update(self, block_pos, new_block=None):
vector = pygame.Vector2(block_pos) * BLOCK_SIZE
self.surface.blit(self.backround_texture, vector)
self.blocks[block_pos] = block = new_block or self.blocks[block_pos]
self.surface.blit(block, vector)
```
## ***Repetition yet another `elif` ladder*** ##
```
def generate(self):
for x in self.x_range:
for y in self.y_range:
x_pos, y_pos = int(self.pos.x)+x, int(self.pos.y)+y
surface = 0
bedrock = 16
soil_amount = 3
if y_pos == surface:
block = 2
elif y_pos > surface and y_pos <= surface + soil_amount:
block = 3
elif y_pos > surface + soil_amount and y_pos < bedrock:
block = 4
elif y_pos == bedrock:
block = 7
elif y_pos > bedrock:
block = 0
else:
block = 1
self.update((x, y), block)
```
You are making several mistakes here, first you are using nested `for` loops to iterate through two entire iterables, while nested `for` loops can have legitimate uses, it is not the case here, you need to use `itertools.product` to get rid of one unnecessary nesting.
Then you are repeatedly assigning these variables `surface = 0; bedrock = 16; soil_amount = 3` inside the loop, the variables aren't mutated at all within the loop, they don't belong in a loop. This is completely pointless and hinders performance. They need to be moved outside of the loop. Or better yet, since you are using a class, they need to be become class variables, since they won't ever change (I guess).
Then the `elif` ladder, this is completely inefficient. Here you have three exact matches, `(0, 3, 16)`, and three values for each of them `(2, 3, 7)`. This is the perfect opportunity to use a `dict`, like this: `{0: 2, 3: 3, 16: 7}`, you can then check if a key is present by using `in` operator and get the corresponding value by querying the `dict`.
Then you have three explicit conditions, `0 < x < 3`, `3 < x < 16` and `16 < x`, with another implicit one that can only be tested if x is negative. What you are trying to do here is to find the closest element in the `list` `[0, 3, 16]` that is less than `x`. You can use `bisect.bisect` to find the index of that element (plus one).
Putting it all together, the code becomes:
```
class Chunk:
surface = 0
bedrock = 16
ground = 3
starts = (surface, ground, bedrock)
levels = dict(zip(starts, (2, 3, 7)))
block_sizes = (3, 4, 0)
def generate(self):
for x, y in product(self.x_range, self.y_range):
y_pos = int(self.pos.y) + y
if y_pos in self.levels:
block = self.levels[y_pos]
else:
i = bisect(self.starts, y_pos) - 1
block = self.block_sizes[i]
block = max(block, 0)
self.update((x, y), block)
```
## ***Nested loops, yet again*** ##
```
def set_blocks(self, blocks):
self.blocks = blocks
for x in self.x_range:
for y in self.y_range:
self.update((x, y))
```
I won't repeat myself, use the following:
```
def set_blocks(self, blocks):
self.blocks = blocks
for x, y in product(self.x_range, self.y_range):
self.update((x, y))
```
## ***Repetition floating assignments*** ##
`self.loader_chunk_pos = pygame.Vector2(convert(loader_pos, self.actual_chunk_size, self.actual_chunk_size)[0])`
You have used the above line three times, each time only changing the first argument to the `convert` function. Make a function for that.
```
class World:
def __init__(self, loader_pos, blah, blahh):
self.set_loader_chunk_pos(loader_pos)
def set_loader_chunk_pos(self, pos):
self.loader_chunk_pos = pygame.Vector2(
convert(pos, self.actual_chunk_size, self.actual_chunk_size)[0]
)
```
## ***Unnecessary list comprehension and inefficient way to get difference of sets*** ##
```
def handle_chunk_loader(self):
chunks_needed = [chunk_pos for chunk_pos in self.chunks_to_load(self.loader_chunk_pos)]
chunks_currently_loaded = [chunk_pos for chunk_pos in self.loaded_chunks]
self.load_chunks([chunk_pos for chunk_pos in chunks_needed if chunk_pos not in chunks_currently_loaded])
self.unload_chunks([chunk_pos for chunk_pos in chunks_currently_loaded if chunk_pos not in chunks_needed])
```
Don't do a list comprehension without any conditions whatsoever just to get all the elements in a collection and build a `list`. It is terribly inefficient and bad practice in general.
`[e for e in d]` is functionally equivalent to `list(d)`, the latter: `list` constructor is much more concise and it is exactly designed for this very reason, just use it.
However using a `list` is not the correct thing to do here, immediately after converting you are checking all elements that are members of A and not members of B, in other words you are calculating the set difference between A and B, and there is a basic Python data class that is exactly designed for this sort of thing: `set`. Use it. You can then calculate the difference by `seta - setb` (if they are not `set`s, convert them first).
<strike>
```
def handle_chunk_loader(self):
chunks_needed = set(self.chunks_to_load())
loaded_chunks = set(self.loaded_chunks)
self.load_chunks(chunks_needed - loaded_chunks)
self.unload_chunks(loaded_chunks - chunks_needed)
```
</strike>
As you can see, I have not passed `self.loader_chunk_pos`, because I of course have refactored that function as well, also because you most likely won't pass any other argument to that function. So it shouldn't take any arguments (other than `self`).
## Update ##
My previous advice holds but very unfortunately that only works for hashable data types and for whatever reason `pygame.math.Vector2` is unhashable.
So it doesn't work--that is, without modifications. So I patched the unhashable class by subclassing it and overriding `__hash__` dunder method, and it worked.
```
class Vector(pygame.math.Vector2):
def __hash__(self):
return id(self)
def handle_chunk_loader(self):
chunks_needed = {Vector(i) for i in self.chunks_to_load()}
loaded_chunks = {Vector(i) for i in self.loaded_chunks}
self.load_chunks(chunks_needed - loaded_chunks)
self.unload_chunks(loaded_chunks - chunks_needed)
```
## ***More repetition and other errors*** ##
```
def load_chunks(self, chunks_pos):
for pos in chunks_pos:
chunk_pos = (pos.x, pos.y)
block_data = self.innactive_block_data.get(chunk_pos, False)
self.loaded_chunks[chunk_pos] = Chunk(pos, self.chunk_size)
if block_data:
self.loaded_chunks[chunk_pos].set_blocks(block_data)
else:
self.loaded_chunks[chunk_pos].generate()
def unload_chunks(self, chunks_pos):
for chunk_pos in chunks_pos:
chunk = self.loaded_chunks.pop(chunk_pos)
if not self.innactive_block_data.get(chunk_pos, False) == chunk.blocks:
self.innactive_block_data[chunk_pos] = chunk.blocks
```
You have used `self.innactive_block_data.get(chunk_pos, False)` twice, make a function to that as well.
```
A = func()
if A:
do_something()
```
Don't do the above, instead, by using the Walrus (`:=`) operator, you can check and assign at the same time:
```
if A := func():
do_something()
```
And never ever use `not A == B` (unless you are overloading `__ne__`), it is unnecessary and against the fundamentals of Python. You are doing the inequality check and Python has an inequality operator `!=`. `A != B` is the same as `not A == B` but much more concise.
And you can use ternary instead of `if` `else` to reduce some nesting.
```
def get_data(self, chunk_pos):
return self.innactive_block_data.get(chunk_pos, False)
def load_chunks(self, chunks_pos):
for pos in chunks_pos:
chunk_pos = (pos.x, pos.y)
self.loaded_chunks[chunk_pos] = Chunk(pos, self.chunk_size)
(
self.loaded_chunks[chunk_pos].set_blocks(block_data)
if (block_data := self.get_data(chunk_pos))
else self.loaded_chunks[chunk_pos].generate()
)
def unload_chunks(self, chunks_pos):
for chunk_pos in chunks_pos:
chunk = self.loaded_chunks.pop(chunk_pos)
if self.get_data(chunk_pos) != chunk.blocks:
self.innactive_block_data[chunk_pos] = chunk.blocks
```
## ***Repetition nth time and nested for loops*** ##
```
def chunks_to_load(self, loader_pos):
return [pygame.Vector2(chunk_pos_x*self.chunk_size, chunk_pos_y*self.chunk_size)
for chunk_pos_x in range(int(loader_pos.x)-self.loader_distance, int(loader_pos.x)+self.loader_distance+1)
for chunk_pos_y in range(int(loader_pos.y)-self.loader_distance, int(loader_pos.y)+self.loader_distance+1)]
```
Don't pass the `loader_pos` argument, since the function only gets called once in the entire script and with only one argument that is unlikely to be changed, you should access that variable directly.
And nested loops yet again. Use `product`.
Your ranges are constructed in the exact same way, the only difference is that you are accessing different attributes, you can actually make a function to create the ranges for you, and access the attributes with `str`s by using `getattr`.
```
def get_range(self, axis):
n = int(getattr(self.loader_chunk_pos, axis))
return range(
n - self.loader_distance,
n + self.loader_distance + 1,
)
def chunks_to_load(self):
return [
pygame.Vector2(chunk_pos_x * self.chunk_size, chunk_pos_y * self.chunk_size)
for chunk_pos_x, chunk_pos_y in product(
self.get_range("x"), self.get_range("y")
)
]
```
***Repetition + 1***
--------------
```
def change_pos(self, pos):
self.loader_pos += pos
self.loader_chunk_pos = pygame.Vector2(convert(self.loader_pos, self.actual_chunk_size, self.actual_chunk_size)[0])
def set_pos(self, pos):
self.loader_pos = pos
self.loader_chunk_pos = pygame.Vector2(convert(pos, self.actual_chunk_size, self.actual_chunk_size)[0])
```
Don't use the above, use below example:
```
def set_loader_chunk_pos(self, pos):
self.loader_chunk_pos = pygame.Vector2(
convert(pos, self.actual_chunk_size, self.actual_chunk_size)[0]
)
def change_pos(self, pos):
self.loader_pos += pos
self.set_loader_chunk_pos(self.loader_pos)
def set_pos(self, pos):
self.loader_pos = pos
self.set_loader_chunk_pos(pos)
```
## Unnecessary variable assignment ##
`chunk_screen_pos = ch.actual_pos`
While it is generally a good idea to assign named variables, here it is completely unnecessary as it is only used immediately after assignment once.
Your code is of very poor quality, though it is understandable considering you very likely copied from ChatGPT.
----------
## ***2D_MC.py*** ##
```python
import pygame
import world
from itertools import product
from win32api import GetSystemMetrics
TITLE = "2d_MC"
RESOLUTION = tuple(map(GetSystemMetrics, (0, 1)))
MOVEMENT_SPEED = 5
def get_key(key): return getattr(pygame, f"K_{key}")
KEYS = list(map(get_key, ("UP", "DOWN", "LEFT", "RIGHT")))
def get_vector(bools):
vertical, positive = bools
a, b = MOVEMENT_SPEED * (-1, 1)[positive], 0
if vertical:
a, b = b, a
return pygame.Vector2(a, b)
VECTORS = dict(zip(KEYS, map(get_vector, product((1, 0), (0, 1)))))
def handle_events():
for event in pygame.event.get():
if event.type == pygame.QUIT:
return True
keys = pygame.key.get_pressed()
exclusive = [False, False]
for i, key in enumerate(KEYS):
if keys[key]:
exclusive[i % 2] = True
if all(exclusive):
break
overworld.change_pos(VECTORS[key])
return False
def draw():
screen.fill((255, 255, 255))
overworld.render(screen, True, other_offset=window_rect.center)
def game_logic():
overworld.handle_chunk_loader()
def run_game():
while not handle_events():
game_logic()
draw()
pygame.display.update(window_rect)
clock.tick()
print(clock.get_fps())
pygame.quit()
if __name__ == "__main__":
pygame.init()
screen = pygame.display.set_mode(RESOLUTION)
window_rect = pygame.Rect((0, 0), RESOLUTION)
clock = pygame.time.Clock()
pygame.display.set_caption(TITLE)
pygame.event.set_allowed([pygame.KEYDOWN, pygame.QUIT])
overworld = world.World(pygame.Vector2(0, 0), 2, 8)
run_game()
```
----------
## ***world.py*** ##
```python
import pygame
from bisect import bisect
from itertools import product
BLOCK_SIZE = 64
BLOCK_TEXTURE_NAMES = [
"textures/void.png",
"textures/air.png",
"textures/grass_block.png",
"textures/dirt.png",
"textures/stone.png",
"textures/sandstone.png",
"textures/sand.png",
"textures/bedrock.png",
"textures/oak_log.png",
"textures/oak_leaves.png",
"textures/cobblestone.png",
]
def block_texture(texture):
return pygame.transform.scale(
pygame.image.load(texture), (BLOCK_SIZE, BLOCK_SIZE)
).convert_alpha()
def convert(pos, convert_value_x, convert_value_y):
"""Converts coordinate systems."""
nx, rx = divmod(pos.x, convert_value_x)
ny, ry = divmod(pos.y, convert_value_y)
return ((nx, ny), (rx, ry))
class Chunk:
surface = 0
bedrock = 16
ground = 3
starts = (surface, ground, bedrock)
levels = dict(zip(starts, (2, 3, 7)))
block_sizes = (3, 4, 0)
def __init__(self, pos, size):
self.size = size
self.actual_size = size * BLOCK_SIZE
self.block_textures = [
block_texture(texture) for texture in BLOCK_TEXTURE_NAMES
]
self.backround_texture = block_texture("textures/backround.png")
self.pos = pos
self.actual_pos = pos * BLOCK_SIZE
self.surface = pygame.Surface((self.actual_size, self.actual_size))
self.x_range = range(self.size)
self.y_range = range(self.size)
self.blocks = {}
def update(self, block_pos, new_block=None):
vector = pygame.Vector2(block_pos) * BLOCK_SIZE
self.surface.blit(self.backround_texture, vector)
self.blocks[block_pos] = block = new_block or self.blocks[block_pos]
self.surface.blit(block, vector)
def generate(self):
for x, y in product(self.x_range, self.y_range):
y_pos = int(self.pos.y) + y
if y_pos in self.levels:
block = self.levels[y_pos]
else:
i = bisect(self.starts, y_pos) - 1
block = self.block_sizes[i]
block = max(block, 0)
self.update((x, y), block)
def set_blocks(self, blocks):
self.blocks = blocks
for x, y in product(self.x_range, self.y_range):
self.update((x, y))
def __str__(self):
return str([self.blocks[(x, y)] for x in self.x_range for y in self.y_range])
class Vector(pygame.math.Vector2):
def __hash__(self):
return id(self)
class World:
def __init__(self, loader_pos, loader_distance, chunk_size):
self.chunk_size = chunk_size
self.actual_chunk_size = chunk_size * BLOCK_SIZE
self.loaded_chunks = {}
self.loader_pos = loader_pos
self.set_loader_chunk_pos(loader_pos)
self.loader_distance = loader_distance
self.rendered = pygame.Surface((1, 1)).convert_alpha()
self.innactive_block_data = {}
def handle_chunk_loader(self):
chunks_needed = {Vector(i) for i in self.chunks_to_load()}
loaded_chunks = {Vector(i) for i in self.loaded_chunks}
self.load_chunks(chunks_needed - loaded_chunks)
self.unload_chunks(loaded_chunks - chunks_needed)
def get_data(self, chunk_pos):
return self.innactive_block_data.get(chunk_pos, False)
def load_chunks(self, chunks_pos):
for pos in chunks_pos:
chunk_pos = (pos.x, pos.y)
self.loaded_chunks[chunk_pos] = Chunk(pos, self.chunk_size)
(
self.loaded_chunks[chunk_pos].set_blocks(block_data)
if (block_data := self.get_data(chunk_pos))
else self.loaded_chunks[chunk_pos].generate()
)
def unload_chunks(self, chunks_pos):
for chunk_pos in chunks_pos:
chunk = self.loaded_chunks.pop(chunk_pos)
if self.get_data(chunk_pos) != chunk.blocks:
self.innactive_block_data[chunk_pos] = chunk.blocks
def get_block(self, pos):
chunk_pos, block_pos = convert(pos, self.chunk_size, self.chunk_size)
try:
return self.loaded_chunks[chunk_pos].blocks[block_pos]
except IndexError:
return False
def get_range(self, axis):
n = int(getattr(self.loader_chunk_pos, axis))
return range(
n - self.loader_distance,
n + self.loader_distance + 1,
)
def chunks_to_load(self):
return [
pygame.Vector2(chunk_pos_x * self.chunk_size, chunk_pos_y * self.chunk_size)
for chunk_pos_x, chunk_pos_y in product(
self.get_range("x"), self.get_range("y")
)
]
def set_loader_chunk_pos(self, pos):
self.loader_chunk_pos = pygame.Vector2(
convert(pos, self.actual_chunk_size, self.actual_chunk_size)[0]
)
def change_pos(self, pos):
self.loader_pos += pos
self.set_loader_chunk_pos(self.loader_pos)
def set_pos(self, pos):
self.loader_pos = pos
self.set_loader_chunk_pos(pos)
def render(self, screen, use_pos, other_offset=0):
for ch in self.loaded_chunks.values():
screen.blit(
ch.surface,
(ch.actual_pos - self.loader_pos * int(use_pos)) + other_offset,
)
```