Public Group

Code Review & Cool Stuff

This topic is 634 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

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();

// 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 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
}

// 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++){
(*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)->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);
}
}
}
}
(*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){
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){
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
}

// 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;
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.

Share on other sites

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.

Share on other sites
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++){
...
} 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++){
...
} else {
j = enemyEntities.erase(j); //memory leak!
}
}

for(auto j = enemyEntities.begin(); j != enemyEntities.end(); j++){
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);
}
}
}

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);
...
}

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.

Share on other sites

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.

Share on other sites

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.

Share on other sites

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?

Thanks.

Share on other sites

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.

Share on other sites
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.

1. 1
2. 2
Rutin
19
3. 3
khawk
18
4. 4
A4L
14
5. 5

• 12
• 16
• 26
• 10
• 44
• Forum Statistics

• Total Topics
633768
• Total Posts
3013736
×