Code Review &#38; Cool Stuff

120

Author

March 23, 2017 03:18 PM

Hello,

My Background:

I'm learning SDL and C++ with LazyFoo's Tutorial and Accelerated C++ by Andrew Koening and Barbara E. Moo.

I've read most of LazyFoo's early tutorials on rendering shapes and textures but not the multithreading yet. From the C++ book, I've learned about basic containers like vectors, lists, and queues but nothing in depth yet because I'm still working through it.

Stuff You Might Like:

I've finished a playable version of pong and I'm making progress on a bullet hell engine.

Here's a ZIP of things that are playable (hopefully) on Windows.

It has:

pong.exe - It's pong.

game.exe - A level I've made with my engine so far. WASD to move and LSHIFT to slow down.

superpong.exe - A goofy version of pong that I never finished with way too many balls and no player 2.

projectiles.exe - Test version of my bullet engine that makes nice patterns if you press 1-7.

Helping Me:

I'm posting some code snips from my engine. I'd like some tips on using the C++ containers because they are a new tool for me. I would also appreciate any style tips.

JGame.h:


#include <SDL.h>
#include <SDL_image.h>
#include <SDL_ttf.h>
#include <list>
#include <cmath>
#include <string>
#include <cstdlib>
#include "LProjectile.h"
#include "LBullet.h"
#include "LEntity.h"
#include "..\LTimer.h"
#include "..\LCircle.h"
#include "..\LPlayer.h"
#include "..\LTexture.h"

const int SCREEN_WIDTH = 640;
const int SCREEN_HEIGHT = 480;
const int FPS = 60;

class JGame {
    public:
        JGame();
        ~JGame();

        void play();

        LEntity* ent;
    private:
        // initialize SDL window & renderer, SDL_image, SDL_font
        bool init();
        void reset();

        bool loadMedia();
        
        // triggers corresponding game element functions
        void handleEvents();
        void handleKeystates();
        void act();
        void render();

        // clean up
        void close();

        // creates a bullet on the player and shoots it to the 
        // top of the screen
        void shootFromPlayer();

        void spawnOrbittingEnemy(bool fromLeft);

        bool initSuccess, loadSuccess;
        bool playing;
        SDL_Window* gameWindow;
        SDL_Renderer* gameRenderer;
        LTimer* gameTimer;

	// Container for each wave of enemies to spawn
        std::queue<std::queue<EntityInfo> > entityWaves;
		
        std::list<LEntity*> enemyEntities;
        std::list<LEntity*> allyEntities;

        Circle centerMarker;
        Circle hLinear;
        int hLinLimit;
        int hLinModifier;

	int frames;
        int score;
        int maxScore;
        bool gameJustEnded;

        LPlayer* player;
        LTexture background;
		
	// # of invincibility frames
        int iFrames;

	// Functions that add a wave of enemies to entityWaves
        void wave1();
        void RLRL_Wave();
        void level1();
        void stall(int frames);
        void alternateSliderPair();
        void clumpSpawn();
};

JGame.cpp:


#include "JGame.h"

// functions of interest, the rest removed to keep file shorter
// act(), render(), wave1()

// I use the containers in act and render
// in wave1 I do some memory stuff that might be really bad but I don't know


JGame::JGame(){
    // not important
}

JGame::~JGame(){
    close();
}

void JGame::play(){
    playing = initSuccess;

    while(playing){
        handleEvents();
        handleKeystates();
        act();
        render();
    }
}

bool JGame::init(){
    // not important
}

void JGame::reset(){
    // not important
}

bool JGame::loadMedia(){
    // not important
}

void JGame::handleEvents(){
    // not important
}

void JGame::handleKeystates(){
    // not important
}

void JGame::act(){
    // if there is another wave of enemies, spawn them
    // else the game is over and print out player score
    if(!entityWaves.empty()){
        if(!entityWaves.front().empty()){
            std::queue<EntityInfo> *currentWave = &entityWaves.front();
            if(currentWave->front().framesBeforeSpawning <= 0){
                enemyEntities.push_back(spawnEntity(currentWave->front()));
                currentWave->pop();
            } else {
                currentWave->front().framesBeforeSpawning--;
            }
        } else {
            entityWaves.pop();
        }
    } else {
        if(enemyEntities.empty() && gameJustEnded){
            SDL_Log("final score: %d/%d\n", score, maxScore);
            if(!score) score = 1;
            int performance = maxScore * 10 / score - 10;
            switch(performance){
                case 0:{
                    if(score == maxScore){
                        SDL_Log("PERFECT! YOU HAVE ACHIEVED EXCELLENCE!\n");
                        break;
                    }
                }
                case 1:{
                    SDL_Log("GREAT! YOU CAN STILL DO BETTER!\n");
                    break;
                }
                case 2:{
                    SDL_Log("YOU'RE GETTING THE HANG OF IT!\n");
                    break;
                }
                case 3:{
                    SDL_Log("YOU CAN DO THIS! I BELIEVE IN YOU!\n");
                    break;
                }
                default: {
                    SDL_Log("KEEP TRYING! DON'T GIVE UP!\n");
                    break;
                }
            }
            SDL_Log("R to retry\n");
            gameJustEnded = false;
        }
    }
	
    if(hLinear.x > hLinLimit || hLinear.x < 64){
        hLinModifier *= -1;
    }
    hLinear.x += hLinModifier;

    // do player actions
    player->act();

    // do friendly entity actions
    std::list<LEntity*>::iterator i = allyEntities.begin();
    for(;i != allyEntities.end();i++){
        if(!(*i)->isDead()){
            (*i)->act();
        } else {
            i = allyEntities.erase(i);
        }
    }
    
    Circle* pC = player->getHitCircle();
	
    // do enemy collisions with player or bullet
    // on hit with player add 2 seconds of iFrames
    // on hit with bullet take damage, 'hurt' the bullet back so it is erased in the next loop
    // if enemy has a bullet to spawn, in its subqueue mark it to be spawned to the game
    std::list<EntityInfo> enemiesToSpawn;
    std::list<LEntity*>::iterator j = enemyEntities.begin();
    for(;j != enemyEntities.end();j++){
        if(!(*j)->isDead()){
            if((*j)->getType() == enemy){
                i = allyEntities.begin();
                for(;i != allyEntities.end();i++){
                    if((*i)->getType()== player_bullet){
                        if(checkCircles((*j)->getCircle(), (*i)->getCircle())){
                            (*j)->takeDamage(1);
                            (*i)->takeDamage(1);
                        }
                    }
                }
            }
            if(!(*j)->isDead()){
                (*j)->act();
                Circle* eC = (*j)->getCircle();        
                if(checkCircles(eC,pC) && iFrames == 0){
                    iFrames = FPS * 2;
                }
                if((*j)->spawnFromSubQueue()){
                    EntityInfo* frontInfo = (*j)->getFrontSubEntityInfo();
                    frontInfo->position = (*j)->getPosition();
                
                    enemiesToSpawn.push_back(*frontInfo);
                }
            } else {
                SDL_Log("enemy down! +50!\n");
                score += 50;
                if(!iFrames){
                    SDL_Log("NO BLINKING BONUS +50!");
                    score += 50;
                }
            }
        } else {
            j = enemyEntities.erase(j);
        }
    }

    // spawn enemy bullets to the field and lerp them at the player
    std::list<EntityInfo>::iterator k = enemiesToSpawn.begin();
    for(;k != enemiesToSpawn.end();k++){
        EntityInfo info = (*k);

        LEntity* spawn = new LEntity(info.type, info.renderer,
                                     info.position.x, info.position.y);
        MotionInfo* lerpToPlayer = new MotionInfo();
        MotionInfo* end = new MotionInfo();

        lerpToPlayer->motion = lerp;
        lerpToPlayer->frames = 4 * FPS;
        lerpToPlayer->goForward = true;

        lerpToPlayer->params.push_back(player->getCenter().x);
        lerpToPlayer->params.push_back(player->getCenter().y);
        lerpToPlayer->params.push_back(lerpToPlayer->frames / 2 * 1000/FPS);
        lerpToPlayer->params.push_back(false);

        end->motion = kill;
        end->frames = 1;

        spawn->pushMotion(*lerpToPlayer);
        spawn->pushMotion(*end);

        enemyEntities.push_back(spawn);
    }
}

void JGame::render(){
    SDL_SetRenderDrawColor(gameRenderer, 0,0,0,0xFF);
    SDL_RenderClear(gameRenderer);

    background.render(gameRenderer, 0, 0, NULL, 0, NULL, SDL_FLIP_NONE);

    //run render functions below

    SDL_Color green = {0,0xFF,0x55,0xFF};
    renderCircle(&hLinear, green, gameRenderer);

    player->render(0,0);

    SDL_Color red = {0xFF,0,0,0xFF};
    renderCircle(&centerMarker, red, gameRenderer);

    SDL_Color cyan = {0,0xFF,0xFF,0xFF};

    if(!iFrames){
        renderCircle(player->getHitCircle(), cyan, gameRenderer);
    } else {
        if(iFrames % 6 < 2){
			// make player hitcircle blink
            renderCircle(player->getHitCircle(), cyan, gameRenderer);
        }
    }

    std::list<LEntity*>::iterator i = allyEntities.begin();
    for(;i != allyEntities.end();i++){
        (*i)->render();
    }

    std::list<LEntity*>::iterator j = enemyEntities.begin();
    for(;j != enemyEntities.end();j++){
        (*j)->render();
    }

    SDL_RenderPresent(gameRenderer);
    frames++;
    if(iFrames) iFrames--;
}


void JGame::close(){
    // not important 
}

void JGame::shootFromPlayer(){
    // not important
}

void addBullet(EntityInfo *super, int frameDelay){
    // not important
}

void JGame::wave1(){
    std::queue<EntityInfo> theWave;

    EntityInfo* rightSlider = new EntityInfo();
    
    rightSlider->type = enemy;
    rightSlider->position.x = 0;
    rightSlider->position.y = 100;
    rightSlider->renderer = gameRenderer;

    MotionInfo* moveRight = new MotionInfo();
    MotionInfo* end = new MotionInfo();

    moveRight->motion = horizontal;
    moveRight->frames = 6 * FPS;
    moveRight->goForward = true;

    moveRight->params.push_back(3);

    end->motion = kill;
    end->frames = 1;

    pushMotion(rightSlider->motionQueue, *moveRight);
    pushMotion(rightSlider->motionQueue, *end);

    EntityInfo* sliders[4];
    int waveSize = 4;
    
    for(int i = 0; i<4; i++){
        sliders[i] = new EntityInfo();
        memcpy(sliders[i], rightSlider, sizeof(EntityInfo));
        sliders[i]->position.y += 100 * i;
        sliders[i]->framesBeforeSpawning = FPS;
        addBullet(sliders[i], 0);
        addBullet(sliders[i], FPS);
        addBullet(sliders[i], FPS);
        addBullet(sliders[i], FPS);
        theWave.push(*sliders[i]);
    }

    entityWaves.push(theWave);

    maxScore += waveSize * 100;

    delete rightSlider;
    delete moveRight;
    delete end;
}

void JGame::RLRL_Wave(){
    // not important
}

// stalls by waiting <frames> and spawning an entity that instantly dies
void JGame::stall(int frames){
    // not important
}

void JGame::alternateSliderPair(){
    // not important
}

void JGame::clumpSpawn(){
    // not important
}

void JGame::level1(){
    RLRL_Wave();
    RLRL_Wave();
    stall(FPS);
    wave1();
    wave1();
    stall(FPS);
    int ASPCount = 10;
    for(int i=0; i<ASPCount; i++){
        alternateSliderPair();
        stall(FPS/2);
    }
    stall(4*FPS);
    clumpSpawn();
    stall(6*FPS);
    clumpSpawn();
    stall(6*FPS);
}

If you need to see the whole code base I've attached it.

Thank you for your time.

Bregma

9,461

March 23, 2017 05:47 PM

I see no reason to use std::list in that code. Your better choice would be std::vector (which is almost always the answer, and std::list is almost never the answer).
I would also urge you to typedef your use of standard library containers because (a) it is clearer what the type is, in a domain-oriented way as opposed to an implementation-oriented way, and (2) it's usually less typing. Better yet, use a current version of C++ and rely on using and auto.


using EntityBag = std::vector<LEntity*>;
// ...
EntityBag enemyEntities;
// ...
for(auto enemy: enemyEntities) {
  // ...
}

Stephen M. Webb
Professional Free Software Developer

Fonzki

120

Author

March 23, 2017 08:35 PM

Thanks Bregma,

I will adopt that using & auto convention because it is much cleaner and clearer. I've also gotta look into writing more modern C++, I'll see what I can find out with google.

I originally used vectors but swapped them for lists because I read that lists are better for deleting elements randomly where vectors excel at deleting from either end as they increase in size.

You've me a lot of useful information thanks again.

Khatharr

8,814

March 23, 2017 08:35 PM

Your attachment didn't happen (I think the GDN attachment system is offline.), so I was unable to look at the rest of the code. You can try posting to GitHub, which is a good habit to be in and makes it easy to share your code. If you're using Visual Studio there is a GitHub plugin that can be used to publish your project fairly easily from the team management pane.

Bregma is correct about avoiding std::list, as it is recognized as being woefully inefficient compared to other containers in almost all cases. He also showed you the use of the range-based for loop, which was introduced in C++11. In all those cases where you're iterating an entire container you can do so far more easily with this syntax.

I see some raw pointer usage that caught my attention, and sure enough I found a fairly serious memory leak:

std::list<LEntity*>::iterator j = enemyEntities.begin();
for(;j != enemyEntities.end();j++){
    if(!(*j)->isDead()){
        ...
    } else {
        j = enemyEntities.erase(j); //memory leak!
    }
}

The problem here is that you have a container of pointers which point to objects that were allocated using 'new' and are not owned elsewhere.
Look at a trivial case and consider what's happening:

std::list<Foo*> bar;
{
  Foo* baz = new Foo; //allocate a Foo and store its address in baz
  bar.push_back(baz); //copy baz to bar
}
bar.erase(bar.begin()); //remove baz from bar
//at this point we have lost the address of the Foo that was allocated

The easy way around this is to use a smart pointer:

#include <vector>
#include <memory>
#include <iostream>

struct Foo {
  int a;
  virtual ~Foo() = default;
};

struct Derived : public Foo {
  Derived(int val) {
    std::cout << "Derived constructor\n";
    a = val;
  }
  ~Derived() { std::cout << "Derived destructor - value was " << a << "\n"; }
};

int main() {
  std::vector<std::unique_ptr<Foo>> bar; //this works fine with derived types
  {
    //prefer std::make_unique to new
    //.emplace_back() will construct the new smart pointer in-place in the container
    bar.emplace_back(std::make_unique<Derived>(42));
    //you have a spawnEntity function that returns a pointer, that can also work here:
    //enemyEntities.emplace_back(spawnEntity(whatever));
  }
  //when the unique_ptr is removed its destructor will be called, which calls delete 
  //on the pointer it wraps, releasing the allocated memory and calling the object's
  //destructor
  bar.erase(bar.begin());

  std::cin.get();
}

I also see this:

std::queue<EntityInfo> *currentWave = &entityWaves.front();
if(currentWave->front().framesBeforeSpawning <= 0){

Which should really be this:

std::queue<EntityInfo> &currentWave = entityWaves.front();
if(currentWave.front().framesBeforeSpawning <= 0){

Since you're taking a reference to something owned elsewhere and you don't need to reseat the reference at any point, just use an actual reference instead of a pointer. It's easier to work with and it's less error-prone since you know that you don't need to delete it, and you can't accidentally reseat it.

LEntity* ent;

What is that and why is it public?

Returning to this:

std::list<LEntity*>::iterator j = enemyEntities.begin();
for(;j != enemyEntities.end();j++){
    if(!(*j)->isDead()){
        ...
    } else {
        j = enemyEntities.erase(j); //memory leak!
    }
}

How about:

for(auto j = enemyEntities.begin(); j != enemyEntities.end(); j++){
    if((*j)->isDead()){
        j = enemyEntities.erase(j);
        continue;
    }
    ...
}

This reversal of the condition produces similar logic but lets you reduce the scoping depth, which means there are fewer things for the reader to remember about as they proceed through the code.
The scoping in that section is getting extreme. That's usually a sign that you should be delegating to other functions. In this case you may want to delegate collision detection to LEntitiy such as:

void doCollisions(std::vector<std::unique_ptr<LEntity>>& collidables) {
  auto myCircle = getCircle();
  for(auto& foe : collidables) {
    if(checkCircles((myCircle, foe.getCircle())){
      //you may want to replace this part with a virtual function
      foe.takeDamage(1);
      takeDamage(1);
      if(isDead()) }{ break; }
    }
  }
}

then just

//clean the list first
for(auto iter = enemyEntities.begin(); iter != enemyEntities.end(); iter++) {
  if(iter->isDead()) { iter = enemyEntities.erase(iter); }
}

//then run the logic
for(auto& ent : enemyEntites) {
  ent.doCollisions(allyBullets);
  if(ent.isDead()) { continue; }
  ...
}

I also see if((*j)->getType() == enemy){ which is a code smell (see the L in SOLID), and with good reason in this case. From what I've read it looks like you're storing enemies and their bullets in the same containers and then differentiating between them manually like this. Rather than doing that, just store them in two containers. You'll get a performance boost and you'll write less code.

That's a lot for now, so I'll stop there.
This is a very good start. Keep going.

void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

Fonzki

120

Author

March 23, 2017 11:53 PM

Thanks Khatharr,

I've put the code onto git here. If you or anyone else needs it to further critique. I'll go learn about smart pointers, SOLID, and C++11.

There's leftovers that from debugging that I didn't fully clear out, that's why that Ent line is public so I'm taking it out.

I want to update my engine with all the new things you guys are telling me. I think I'll start with the auto typedef then loops, and tackle my pointer usage. Is there anything else I should keep in mind to keep the code from becoming a mess?

It feels great to start learning the stuff I didn't know I didn't know. Thanks a lot.

Alberth

10,209

March 24, 2017 05:25 AM

Maybe this blog post gives you a few more pointers

http://blog.smartbear.com/c-plus-plus/the-biggest-changes-in-c11-and-why-you-should-care/

Khatharr

8,814

March 24, 2017 05:36 AM

I had assumed based on your use of pointers that LEntity was polymorphic, but it looks like you're just using switching everywhere. Are you familiar with C++ inheritance?

Also, you appear to be loading an instance of the texture for every individual entity, which is wasteful. You should only load one instance of each texture you need for a scene and then have the entities all share that texture. In this case you could load the three textures used by LEntity in your scene, then pass in a pointer to the desired LTexture when constructing the entity. Another approach is to have a 'Sprite' class that stores things like the position and rotation that you want to draw at and holds a pointer or handle to the texture to draw from.

void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

Fonzki

120

Author

March 24, 2017 02:17 PM

Thanks for the resource Alberth.

I know of inheritance as the "x is a type of y" relationship example: a hammer is a type of tool and so is a screwdriver.

But I don't know how to use inheritance properly so I didn't because I read it could become messy if you don't know what you're doing. From what you said are suggesting that I use LEntity as a parent class since everything on screen is a type of entity?

I'll be sure to copy the textures instead loading them over and over.

Thanks.

Alberth

10,209

March 24, 2017 04:21 PM

because I read it could become messy if you don't know what you're doing

By that argument, I would strongly advice not to use C++ or any other programming language :p

In general, anything that you cannot mess up by careless use, isn't worth your time, as its expressive power would be so limited, it's basically useless.

Inheritance is easily used too much. However, it is hard to define when that is. Simplest way to find out is to start using it when it seems appropriate. Likely, you'll end up at the wrong side of the balance a few times, but it's all learning experience.

Khatharr

8,814

March 24, 2017 07:56 PM

You've basically already created a polymorphic type, so you may as well use the idiomatic means of doing so instead of storing a type variable and switching.

void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

Code Review & Cool Stuff

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Code Review & Cool Stuff

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines