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:
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 principle and clean those up.
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:
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.
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
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
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 = (1, 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)
block = self.block_sizes[i]
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 sets, convert them first).
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)
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 int(self.x * 10 + self.y)
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 strs 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. If you didn't copy from ChatGPT, then the quality of your code is about the same level as what ChatGPT would generate. You have a long way to go. But I don't mean to discourage you, you just have a lot to learn and huge potential to improve.
2D_MC.py
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
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 = (1, 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)
block = self.block_sizes[i]
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 int(self.x * 10 + self.y)
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,
)
handle_events()returns a 0 / 1intit looks like C code. But this is python, where a predicate can return a True / Falsebool, andmypywould be happy to check that for you. Thewhile True:loop would more naturally be awhile handle_events():loop. \$\endgroup\$