1
\$\begingroup\$

I've created a 2-player tank battle game. The code however seems like it could be greatly simplified and there are a few issues. One of which is the hitboxes of the tank seem a bit unreliable. Sometimes when a bullet is fired, it will hit the empty space next to a tank and count as a hit, but other times it will pass straight through the tank.

This is using Pygame for Python 3.6.3. This program is a modification of a program shown in "More Python Programming for the Absolute Beginner" by Jonathon Harbour (Chapter 12). I’m looking for ways to improve and simplify my program.

# Tank 2-Player Battle Game

import sys, time, random, math, pygame
from pygame.locals import *
from My_Library import *

class Bullet():
    def __init__(self, position):
        self.alive = True
        self.color = (250, 20, 20)
        self.position = Point(position.x, position.y)
        self.velocity = Point(0, 0)
        self.rect = Rect(0, 0, 4, 4)
        self.owner = ""

    def update(self, ticks):
        self.position.x -= self.velocity.x * 10.0
        self.position.y -= self.velocity.y * 10.0
        if self.position.x < 0 or self.position.x > 800 \
           or self.position.y < 0 or self.position.y > 600:
            self.alive = False
        self.rect = Rect(self.position.x, self.position.y, 4, 4)

    def draw(self, surface):
        pos = (int(self.position.x), int(self.position.y))
        pygame.draw.circle(surface, self.color, pos, 4, 0)

def fire_cannon(tank):
    position = Point(tank.turret.X, tank.turret.Y)
    bullet = Bullet(position)
    angle = tank.turret.rotation + 90
    bullet.velocity = angular_velocity(angle)
    bullets.append(bullet)
    play_sound(shoot_sound)
    return bullet

def player_fire_cannon():
    bullet = fire_cannon(player)
    bullet.owner = "player"
    bullet.color = (30, 250, 30)

def player2_fire_cannon():
    bullet = fire_cannon(player2)
    bullet.owner = "player2"
    bullet.color = (250, 30, 30)

class Tank(MySprite):
    def __init__(self, tank_file, turret_file):
        MySprite.__init__(self)
        self.load(tank_file, 50, 60, 4)
        self.speed = 0.0
        self.scratch = None
        self.float_pos = Point(0, 0)
        self.velocity = Point(0, 0)
        self.turret = MySprite()
        self.turret = MySprite()
        self.turret.load(turret_file, 32, 64, 4)
        self.fire_timer = 0

    def update(self,ticks):
        # update chassis
        MySprite.update(self, ticks, 100)
        self.rotation = wrap_angle(self.rotation)
        self.scratch = pygame.transform.rotate(self.image, -self.rotation)
        angle = wrap_angle(self.rotation-90)
        self.velocity = angular_velocity(angle)
        self.float_pos.x += self.velocity.x * 2
        self.float_pos.y += self.velocity.y * 2

        # warp tank around screen edges (keep it simple)
        if self.float_pos.x < -50: self.float_pos.x = 800
        elif self.float_pos.x > 800: self.float_pos.x = -50
        if self.float_pos.y < -60: self.float_pos.y = 600
        elif self.float_pos.y > 600: self.float_pos.y = -60

        # transfer float position to integer position for drawing
        self.X = int(self.float_pos.x)
        self.Y = int(self.float_pos.y)

        # update turret
        self.turret.position = (self.X, self.Y)
        self.turret.last_frame = 0
        self.turret.update(ticks, 100)
        self.turret.rotation = wrap_angle(self.turret.rotation)
        angle = wrap_angle(self.turret.rotation)
        self.turret.scratch = pygame.transform.rotate(self.turret.image, -angle)

    def draw(self, surface):
        # draw the chassis
        width, height = self.scratch.get_size()
        center = Point(width/2, height/2)
        surface.blit(self.scratch, (self.X-center.x, self.Y-center.y))        
        # draw the turret
        width, height = self.turret.scratch.get_size()
        center = Point(width/2, height/2)
        surface.blit(self.turret.scratch, (self.turret.X-center.x,
                                           self.turret.Y-center.y))

    def __str__(self):
        return MySprite.__str__(self) + "," + str(self.velocity)

# this function initializes the game
def game_init():
    global screen, backbuffer, font, timer, player_group, player, \
           player2, bullets

    pygame.init()
    screen = pygame.display.set_mode((800, 600))
    backbuffer = pygame.Surface((800, 600))
    pygame.display.set_caption("Tank Battle Game")
    font = pygame.font.Font(None, 30)
    timer = pygame.time.Clock()
    pygame.mouse.set_visible(False)

    # create player tank
    player = Tank("tank.png", "turret.png")
    player.float_pos = Point(400, 300)

    # create second player tank
    player2 = Tank("enemy_tank.png", "enemy_turret.png")
    player2.float_pos = Point(random.randint(50, 760), 50)

    # create bullets
    bullets = list()

# this function initializes the audio system
def audio_init():
    global shoot_sound, boom_sound

    # initialize the audio mixer
    pygame.mixer.init()

    # load sound files
    shoot_sound = pygame.mixer.Sound("shoot.wav")
    boom_sound = pygame.mixer.Sound("boom.wav")

# this function uses any available channel to play a sound clip
def play_sound(sound):
    channel = pygame.mixer.find_channel(True)
    channel.set_volume(0.5)
    channel.play(sound)

# main program begins
game_init()
audio_init()
game_over = False
player_score = 0
player2_score = 0
last_time = 0
action1 = False
action2 = False
action3 = False
action4 = False
action5 = False
action6 = False

# main loop
while True:
    timer.tick(30)
    ticks = pygame.time.get_ticks()

    # event section
    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.display.quit()
            pygame.quit()
            sys.exit()
        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_LEFT:
                action1 = True
            if event.key == pygame.K_RIGHT:
                action2 = True
            if event.key == pygame.K_a:
                action3 = True
            if event.key == pygame.K_d:
                action4 = True
            if event.key == pygame.K_UP:
                action5 = True
            if event.key == pygame.K_w:
                action6 = True
        if event.type == pygame.KEYUP:
            if event.key == pygame.K_LEFT:
                action1 = False
            if event.key == pygame.K_RIGHT:
                action2 = False
            if event.key == pygame.K_a:
                action3 = False
            if event.key == pygame.K_d:
                action4 = False
            if event.key == pygame.K_UP:
                action5 = False
            if event.key == pygame.K_w:
                action6 = False

    if action1 == True:
        player.rotation -= 4.0
        player.turret.rotation -= 4.0
    if action2 == True:
        player.rotation += 4.0
        player.turret.rotation += 4.0
    if action3 == True:
        player2.rotation -= 4.0
        player2.turret.rotation -= 4.0
    if action4 == True:
        player2.rotation += 4.0
        player2.turret.rotation += 4.0
    if action5 == True:
        if ticks > player.fire_timer + 1000:
            player.fire_timer = ticks
            player_fire_cannon()
    if action6 == True:
        if ticks > player2.fire_timer + 1000:
            player2.fire_timer = ticks
            player2_fire_cannon()


    # update section
    if not game_over:
        # move tank
        player.update(ticks)

        # update player two
        player2.update(ticks)

        # update bullets
        for bullet in bullets:
                bullet.update(ticks)
                if bullet.owner == "player":
                    if pygame.sprite.collide_rect(bullet, player2):
                        player_score += 1
                        bullet.alive = False
                        play_sound(boom_sound)
                elif bullet.owner == "player2":
                    if pygame.sprite.collide_rect(bullet, player):
                        player2_score += 1
                        bullet.alive = False
                        play_sound(boom_sound)

    # drawing section
    backbuffer.fill((100, 100, 20))

    for bullet in bullets:
        bullet.draw(backbuffer)

    player.draw(backbuffer)

    player2.draw(backbuffer)

    screen.blit(backbuffer, (0, 0))

    if not game_over:
        print_text(font, 0, 0, "PLAYER 1: " + str(player_score))
        print_text(font, 650, 0, "PLAYER 2: " + str(player2_score))
    else:
        print_text(font, 0, 0, "GAME OVER")

    pygame.display.update()

    # remove expired bullets
    for bullet in bullets:
        if bullet.alive == False:
            bullets.remove(bullet)

Here is the My_Library file that is imported at the beginning:

# MyLibrary.py
import sys, time, random, math, pygame
from pygame.locals import *

# calculates velocity of an angle
def angular_velocity(angle):
    vel = Point(0, 0)
    vel.x = math.cos( math.radians(angle) )
    vel.y = math.sin( math.radians(angle) )
    return vel

# calculates angle between two points
def target_angle(x1, y1, x2, y2):
    delta_x = x2 - x1
    delta_y = y2 - y1
    angle_radians = math.atan2(delta_y, delta_x)
    angle_degrees = math.degrees(angle_radians)
    return angle_degrees

# wraps a degree angle at boundary
def wrap_angle(angle):
    return abs(angle % 360)

# prints text using the supplied font
def print_text(font, x, y, text, color = (255, 255, 255)):
    imgText = font.render(text, True, color)
    screen = pygame.display.get_surface()
    screen.blit(imgText, (x, y))

# MySprite class extends pygame.sprite.Sprite
class MySprite(pygame.sprite.Sprite):
    def __init__(self):
        pygame.sprite.Sprite.__init__(self) # extend the base Sprite class
        self.master_image = None
        self.frame = 0
        self.old_frame = -1
        self.frame_width = 1
        self.frame_height = 1
        self.first_frame = 0
        self.last_frame = 0
        self.columns = 1
        self.last_time = 0
        self.direction = 0
        self.velocity = Point(0, 0)
        self.rotation = 0.0 # degrees # added
        self.old_rotation = 0.0 # added

    # X property
    def _getx(self): return self.rect.x
    def _setx(self, value): self.rect.x = value
    X = property(_getx, _setx)

    # Y property
    def _gety(self): return self.rect.y
    def _sety(self, value): self.rect.y = value
    Y = property(_gety, _sety)

    # position property
    def _getpos(self): return self.rect.topleft
    def _setpos(self, pos): self.rect.topleft = pos
    position = property(_getpos, _setpos)

    def load(self, filename, width, height, columns):
        self.master_image = pygame.image.load(filename).convert_alpha()
        self.frame_width = width
        self.frame_height = height
        self.rect = Rect(0, 0, width, height)
        self.columns = columns
        # try to auto-calculate total frames
        rect = self.master_image.get_rect()
        self.last_frame = (rect.width // width) * (rect.height // height) - 1

    def update(self, current_time, rate=30):
        if self.last_frame > self.first_frame:
            # update animation frame number
            if current_time > self.last_time + rate:
                self.frame += 1
                if self.frame > self.last_frame:
                    self.frame = self.first_frame
                self.last_time = current_time
        else:
            self.frame = self.first_frame

        # build current frame only if it changed
        if self.frame != self.old_frame:
            frame_x = (self.frame % self.columns) * self.frame_width
            frame_y = (self.frame // self.columns) * self.frame_height
            rect = Rect(frame_x, frame_y, self.frame_width, self.frame_height)
            self.image = self.master_image.subsurface(rect)
            self.old_frame = self.frame

    def __str__(self):
        return str(self.frame) + "," + str(self.first_frame) + \
               "," + str(self.last_frame) + "," + str(self.frame_width) + \
               "," + str(self.frame_height) + "," + str(self.columns) + \
               "," + str(self.rect)

    def load(self, filename, width = 0, height = 0, columns = 1):
        self.master_image = pygame.image.load(filename).convert_alpha()
        self.set_image(self.master_image, width, height, columns)

    def set_image(self, image, width = 0, height = 0, columns = 1):
        self.master_image = image
        if width == 0 and height == 0:
            self.frame_width = image.get_width()
            self.frame_height = image.get_height()
        else:
            self.frame_width = width
            self.frame_height = height
            rect = self.master_image.get_rect()
            self.last_frame = (rect.width//width) * (rect.height//height) - 1
        self.rect = Rect(0, 0, self.frame_width, self.frame_height)
        self.columns = columns

# Point class
class Point(object):
    def __init__(self, x, y):
        self.__x = x
        self.__y = y

    # X property
    def getx(self): return self.__x
    def setx(self, x): self.__x = x
    x = property(getx, setx)

    # Y property
    def gety(self): return self.__y
    def sety(self, y): self.__y = y
    y = property(gety, sety)

    def __str__(self):
        return "{X:" + "{:.0f}".format(self.__x) + \
               ",Y:" + "{:.0f}".format(self.__y) + "}"
\$\endgroup\$

closed as off-topic by 200_success, esote, Graipher, Mast, яүυк Apr 19 at 13:51

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – 200_success, esote, Graipher, Mast, яүυк
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Is there a repo with the other files? \$\endgroup\$ – Austin Hastings Apr 16 at 3:18
  • \$\begingroup\$ All of the other files are built into python and pygame. I don’t know a lot about their inner workings, but the information can all be found on the internet. Here’s a link to the pygame website: pygame.org/docs/tut/ImportInit.html \$\endgroup\$ – MuckinAround145 Apr 16 at 3:35
  • \$\begingroup\$ It wouldn’t let me edit my comment further, but I found the link to the Python repository: pypi.org/search/?q=time \$\endgroup\$ – MuckinAround145 Apr 16 at 3:42
  • \$\begingroup\$ Are the bullets supposed to be rectangular or circular? You're drawing them as circles, yet checking for intersection as rectangular. \$\endgroup\$ – Snowbody Apr 16 at 3:44
  • \$\begingroup\$ The bullets are supposed to be circular. \$\endgroup\$ – MuckinAround145 Apr 16 at 3:46
5
\$\begingroup\$

You have provided a lot of code, and it's not super well organized. So I'm going to focus on that, plus some things I noticed in passing. There's probably enough for another review once you incorporate what you learn from this one.

First, you have some good ideas. Moving "basic" things into a library is a good idea. Using classes is a good idea. Breaking your conceptually related statements into functions is a good idea. For the most part, you are on target with what you are doing and how you are doing it.

Now for the not-most part: ;-)

PEP 8

You code isn't PEP-8 in a lot of ways. Some of them are harmful, and some are just irritating. My_Library is irritating and harmful: not every filesystem is case-aware, so you can't depend on caps; and the name is useless. Either name it something application related (tank_library) or something platform related (pygame_utils).

Comments

Comments are funny things. Most coding classes try to encourage you to use them, but don't provide you with enough of a challenge to actually need them. So they encourage you to put in worthless comments, just to get in the habit. That appears to have happened to you. Consider this:

# MySprite class extends pygame.sprite.Sprite
class MySprite(pygame.sprite.Sprite):

This is a classic "worthless comment." The comment merely says in English exactly what the code says in the syntax of Python. It's right up there with

x = x + 1  # add 1 to x

You should delete any comment like this, since it provides no value presently, and might eventually drift into being wrong and providing negative value.

Now consider this:

# prints text using the supplied font
def print_text(font, x, y, text, color = (255, 255, 255)):

This is almost a worthless comment. But it's also almost a useful comment. Except that there are no useful comments on functions. What you want is to make this into a useful docstring:

def print_text(...etc...):
    ''' Display text on screen at position (x, y) using font & color. '''

Even more useful would be to specify what position (x, y) means. Is that the top left, bottom left, the center, the baseline? Also, color is semi-obvious since you provide a default, but how about font? Is that a string name, or a Font object, or ... ?

The nice thing about docstrings is that you can write as much as you want, and it can be nicely useful in a lot of ways, including just doing help(print_text) in the REPL. Comments, not so much.

Finally, consider this:

# transfer float position to integer position for drawing
self.X = int(self.float_pos.x)
self.Y = int(self.float_pos.y)

This seems like a useful comment. It explains too much of what you're doing -- after all, I can see you are converting from float to int. But it does explain why you are doing something non-obvious. That provides some value.

(Note: the presence of .X as an integer drawable version of .x might not have value, per se. But the comment has value, since it makes clear something that wouldn't be clear otherwise.)

Organization

Class Bullet

Class Bullet has an update and a draw method, but it is not a subclass of anything:

class Bullet():
    def __init__(self, position): ...
    def update(self, ticks): ...
    def draw(self, surface): ...

If it's not a subclass, you can drop the parentheses after the name. But it probably should be a subclass, either of Sprite or MySprite. Pygame provides sprite groups to do what you are doing manually. You should put your bullets into one.

Class Tank

Now this:

def fire_cannon(tank):
    ...

def player_fire_cannon():
    bullet = fire_cannon(player)
    ...

def player2_fire_cannon():
    bullet = fire_cannon(player2)
    ...

class Tank(MySprite):

If only there were some mechanism whereby you could write a function that would have access to a collection of related data... Oh, wait! You could make fire_cannon a method of class Tank.

def __init__(self, tank_file, turret_file):
    MySprite.__init__(self)

def update(self,ticks):
    # update chassis
    MySprite.update(self, ticks, 100)

The built-in function you are looking for here is super().

And speaking of the update method: delegate! You Tank has a chassis and a turret, and they get drawn differently. So make the turret (or the chassis) a separate sprite, and "manage" it from the Tank class.

You might even consider making both of them separate sprites, and having the Tank be invisible or not a sprite at all. This would be the difference between "is-a" sprite and "has-a" sprite. If the tank is just a holder for a collection of other sprites (and a central position), a lot of your code probably gets shorter because the classes can handle it.

Pygame

You need Pygame in both your library and your main file. Consider trying to push all the explicit pygame dependencies into a single file, and your non-pygame dependencies into the other file. This won't actually result in an app/library distribution, but it might help you to identify "pure" objects that you can optimize in different ways.

class Point

This is horrible:

self.position = Point(position.x, position.y)

If position is a Point, why can't you just either refer to it, or initialize a new one using an instance of the same class?

self.position = position
# or
from copy import copy
self.position = copy(position)
# or
self.position = Point(position)

In fact, I'd suggest that you use namedtuple for your point class. It's built-in, and it does almost everything you want:

from collections import namedtuple
Point = namedtuple('Point', 'x y')
p1 = Point(123, 456)
p2 = Point(*p1)

Note the "splat:" *p1. Or you could inherit from the named tuple and provide your own __new__ method which does the splatting for you:

def __new__(cls, p_or_x, y=None):
    if isinstance(p_or_x, cls):
        # New Point from old Point
        return super().__new__(cls, *p_or_x)
    else:
        # New Point from x, y
        return super().__new__(cls, p_or_x, y)

Speaking of PEP 8: please don't use __x and __y unless you know that you need to. (You don't.) Names that start with double underscores, other than the special "dunder" names, are "mangled" internally. That's great for solving a specific problem, but you don't have that problem. Just use x until you have a property method, then switch to _x.

Here are some unrelated lines of code:

    self.position.x -= self.velocity.x * 10.0
    self.position.y -= self.velocity.y * 10.0
    pos = (int(self.position.x), int(self.position.y))

If you have a look at the "dunder methods" available, you will find that it's possible to implement things like scalar multiplication and in-place subtraction. You might even find a way to truncate values ;-)

from math import trunc

def __trunc__(self):
    return self.__class__(trunc(self.x), trunc(self.y))
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.