The sdl2-game from annahowell

Cleanup resources in deconstructors

The code has several places that allocate memory but do not de-allocate the memory when the classes are deconstructed:

It is a good habit to clean up resources whenever you can. This will save a lot of memory leak hunting in the future.

// layer.hpp
class Layer
{
// ..
public:
   ~Layer();
}

// layer.hpp

// Deconstructor of Layer. Will free all allocated resources
Layer::~Layer()
{
    if (bg1 != nullptr)
    {
        SDL_DestroyTexture(bg1);
        bg1 = nullptr;
    }
    // repeat for all bg's

    // make sure to set textures to 0 to indicate we have no textures
    textures = 0;
}

It is important to keep in mind where the resources are owned. For example Layer has a field SDL_Renderer *renderer;. This field is passed in from Game, which is passed in from main.cpp. In this case, main.cpp is the "owner" of this allocation, and should be responsible for cleaning up the resources.

Generally you either allocate and clean up resources in the same class, unless you specifically pass the allocated pointer to a different class.

Change speed to be independent of render speed

SDL2-Game/src/game.cpp

Line 152 in 6cfa2ca

angle += 0.05;

Currently the game updates a fixed amount of distance and rotation per frame. This means that on systems that run at 1 fps, the ship rotates and moves a lot slower than people who run the game at 100fps.

It's a good habit to get the time difference between the last frame with SDL_GetTicks. This would look something like:

Uint32 time = SDL_GetTicks();
Uint32 dTime = time - lastTickTime;
lastTickTime = time; // lastTickTime is a field on the game class

// update logic here

And then you can run your update logic like:

    if(turningRight)
    {
        angle += dTime * TURN_SPEED;
    }

Where TURN_SPEED is a constant which you can tweak.

It's often good to also store turnSpeed and moveSpeed variables in your ship, as a user could get upgrades or effects that change their move/rotate speed.

Change layer properties into a list

SDL2-Game/src/layer.hpp

Lines 13 to 16 in 6cfa2ca

    
           SDL_Texture *bg1, *bg2, *bg3, *bg4; 
        
           SDL_Rect outputRect1a, outputRect1b, outputRect2a, outputRect2b, outputRect3a, outputRect3b, outputRect4a, outputRect4b; 
        
           std::string file1, file2, file3, file4;

Repetitive code is usually messy and error-prone, it would be better to make a list and a loop:

// layer.hpp
class Layer
{
private:
    SDL_Renderer *renderer;
    int SCREEN_WIDTH, SCREEN_HEIGHT;
    std::vector<InnerLayer> layers;
public:
    Layer(SDL_Renderer *renderer, SCREEN_WIDTH, SCREEN_HEIGHT);
    void addLayer(const char* file);
    void render();
}

class InnerLayer
{
private:
    SDL_Texture *bg;
    SDL_Rect outputRectA, outputRectB;
public:
    InnerLayer(SDL_Texture* bg, SDL_Rect outputRectA, SDL_Rect outputRectB);
    void setXoffset();
    void setYoffset();
}

// layer.cpp
// Several functions omitted

void Layer::addLayer(const char* file)
{
    SDL_Rect outputRectA = {0, 0, SCREEN_WIDTH, SCREEN_HEIGHT};
    SDL_Rect outputRectB = {0, 0, SCREEN_WIDTH, SCREEN_HEIGHT};

    // Create third SDL surface from image
    SDL_Surface* surface = IMG_Load(file);

    // Create third hardware optimised SDL texture from the surface
    SDL_Texture* bg = SDL_CreateTextureFromSurface(renderer, surface);

    // Free the surface after texture is made
    SDL_FreeSurface(surface);

    layers.push_back(InnerLayer(bg, outputRectA, outputRectB));
}

void Layer::render()
{
    for(const InnerLayer& layer : layers)
    {
        SDL_RenderCopy(renderer, layer.bg, nullptr, &layer.outputRectA);
        SDL_RenderCopy(renderer, layer.bg, nullptr, &layer.outputRectB);
    }
}

Misplaced comments

SDL2-Game/src/particle.cpp

Lines 41 to 48 in 6cfa2ca

    
           /** -------------------------------------------------------------------------------------- 
        
            Sets the position of the particle on either the horiontal x axis or the vertical y axis 
        
            @param x   New position of the particle on the horizontal x axis 
        
            @param y   New position of the particle on the vertical y axis 
        
            */ 
        
           void Particle::setPositionX(float x) { this->x = x; } 
        
           void Particle::setPositionY(float y) { this->y = y; }

These comments are written to be applicable for both methods, but the way they are set up now they only apply to the first item. When using a tool to generate documentation, this will be picked up incorrectly.

Should be:

 /** -------------------------------------------------------------------------------------- 
  Sets the position of the particle on the horizontal x axis
  
  @param x   New position of the particle on the horizontal x axis 
  */ 
 void Particle::setPositionX(float x) { this->x = x; } 

 /** -------------------------------------------------------------------------------------- 
  Sets the position of the particle on the vertical y axis
  
  @param y   New position of the particle on the vertical y axis 
  */ 
 void Particle::setPositionY(float y) { this->y = y; }

Implement vsync instead of a fixed delay

SDL2-Game/src/main.cpp

Line 28 in 6cfa2ca

SDL_Renderer* renderer = SDL_CreateRenderer(window, -1, 0);

SDL2-Game/src/game.cpp

Line 181 in 6cfa2ca

SDL_Delay(16);

Often it is better to use vsync to synchronize with the refresh rate of your monitor. More information can be found here: https://lazyfoo.net/tutorials/SDL/14_animated_sprites_and_vsync/index.php

Replacing your

SDL_Renderer* renderer = SDL_CreateRenderer(window, -1, 0);

with:

SDL_Renderer* renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);

And removing SDL_Delay(16); should make your game sync up properly with the user's screen

Consider passing a `const char*` instead of a `std::string`

SDL2-Game/src/layer.cpp

Line 11 in 6cfa2ca

    
           Layer::Layer(SDL_Renderer *renderer, int SCREEN_WIDTH, int SCREEN_HEIGHT, std::string file1)

Often if you're not consuming the std::string in the function, it is better to request a const char* from the caller. The caller can then decide if this is a static variable, or an actual string.

const char* SOME_CONSTANT = "foo.png";
Layer layer(.., .., .., SOME_CONSTANT); // no allocation

std::string str = "...";  // allocate some string here
Layer layer2(.., .., .., str.c_str());

Simplify if-else statements

SDL2-Game/src/game.cpp

Lines 83 to 90 in 6cfa2ca

    
           if (currentKeyStates[SDL_SCANCODE_UP]) 
        
           { 
        
               thrusting = true; 
        
           } 
        
           else 
        
           { 
        
               thrusting = false; 
        
           }

These statements are in the form of

    if (currentKeyStates[SDL_SCANCODE_UP])
    {
        thrusting = true;
    }
    else
    {
        thrusting = false;
    }

Because the value assigned to thrusting is always the same as the value retrieved from currentKeyStates[SDL_SCANCODE_UP], it would be cleaner to write:

thrusting = currentKeyStates[SDL_SCANCODE_UP] == 1;

As per documentation:

 A value of 1 means that the key is pressed and a value of 0 means that it is not

You could also make a helper function

bool isKeyPressed(size_t key) {
    return currentKeyStates[key] == 1;
}

thrusting = isKeyPressed(SDL_SCANCODE_UP);

annahowell / sdl2-game Goto Github PK

sdl2-game's People

Contributors

Stargazers

Watchers

sdl2-game's Issues

Cleanup resources in deconstructors

Change speed to be independent of render speed

Change layer properties into a list

Misplaced comments

Implement vsync instead of a fixed delay

Consider passing a `const char*` instead of a `std::string`

Simplify if-else statements

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	SDL_Texture bg1, bg2, bg3, bg4;
	SDL_Rect outputRect1a, outputRect1b, outputRect2a, outputRect2b, outputRect3a, outputRect3b, outputRect4a, outputRect4b;

	std::string file1, file2, file3, file4;

	/** --------------------------------------------------------------------------------------
	Sets the position of the particle on either the horiontal x axis or the vertical y axis

	@param x New position of the particle on the horizontal x axis
	@param y New position of the particle on the vertical y axis
	*/
	void Particle::setPositionX(float x) { this->x = x; }
	void Particle::setPositionY(float y) { this->y = y; }

	if (currentKeyStates[SDL_SCANCODE_UP])
	{
	thrusting = true;
	}
	else
	{
	thrusting = false;
	}