Zombie Shooter 2000

[TCVM]

Hello! I have been trying to get into coding small games for a while now, but I could never get them finished. I have just started coding in C++ in 2012, and created my first "original" game in April. This is my third game I am proud to show off.

If someone could review my use of static arrays in the following classes, it would be appreciated

userInterface.hpp, textureManager.hpp, soundPlayer.hpp

Github: https://github.com/TheCandianVendingMachine/ZombieKiller2000
Compiled Game: https://app.box.com/s/6mql02691rr71uw8szffimup816crw0q

Zombie Shooter 2000 is about you - a triangle on the streets minding his own business until BOOM. Zombie outbreak. Survive for as long as you can, and kill off the endless waves of zombies

[DarkRoku12]

#include "soundPlayer.hpp"
#include <iostream>
#include <memory>

std::deque<sf::Sound> soundPlayer::playingSounds = std::deque<sf::Sound>();

void soundPlayer::playSound(const std::string &filepath)
    {
        sf::SoundBuffer *buffer = new sf::SoundBuffer;
        if (buffer->loadFromFile(filepath))
            {
                sf::Sound newSound;
                playingSounds.emplace_back(newSound);
                playingSounds.back().setBuffer(*buffer);
                playingSounds.back().play();
            }
    }

When you need to release the memory occupped by buffer , how you do it? or , when you do it?

        if (playingSounds.front().getStatus() == sf::Sound::Stopped)
            {
                playingSounds.pop_front();
            }
 

What if others sounds are stopped and not only the first one?
When you pop a sound with and unique (own) buffer you might release the buffer too unless you won't reuse the buffer.


In others files you use: std::map, i think is better to use std::unordered_map for fast access, i think you don't need keep the textures in order.

If someone could review my use of static arrays in the following classes, it would be appreciated

But if your arrays are meant to be static why don't use std::vector or even std::array?

[Brax]

Couple of things:

The game stops to respond and it needs to be forcefully closed if the window focus is lost and when roughly 5 seconds have passed (the console is working btw). My wild guess is that you have a malfunctioning timestep with window ".hasFocus()" function. Check your "while" loops and all sf::Clocks and sf::Time.

Also it would be good if you could limit the framerate, because it eats up the CPU if application does not have a frame limit.

Other than that, everything works fine. Managed to kill 89 vertex eating triangles.

[Hapax]

That SoundPlayer design is a little odd.
Inside playSound() method, you create a brand new sound buffer with a raw pointer, load the buffer from a file, start it playing, then exit the method where the pointer is destroyed.
Since you now have memory allocated to a soundbuffer but no pointer to that memory, "When you need to release the memory occupped by buffer , how you do it?" (as DarkRoku said)
Keep track of your pointers and their memory or, preferably, consider reading this about RAII.
It could be better to store your sound buffers (or their pointers) as a member of the class. Store the sound buffers in an std::vector and they'll be automatically tidied up.
Loading a sound ad-hoc when you want to play it not the greatest of designs. It would be better to store all the sound buffers and load them all in advance at once at the start. Otherwise, your game's sound is based on the ability of its hard disk.

If someone could review my use of static arrays in the following classes, it would be appreciated

userInterface.hpp, textureManager.hpp, soundPlayer.hpp

Why are they static?

The zombie texture and player texture are identical other than their colours. You could create a white version and the change the colour of the sprite instead of using two separate textures.

[TCVM]

Thanks for the feedback. About the soundPlayer design - I was going to use a std::unique_ptr, but I thought they would be destroyed as soon as the function ends. I never thought about having a std::vector holding the  sound buffers

I couldn't figure out how to delete the soundBuffers after they stopped playing without having either a dedicated thread for it, or it's own update function. Neither of which sounded good, so I opted for checking for stopped sounds at the front whenever I played another


The arrays are static because I wanted to be able to edit certain variables from other classes without passing in an instance of the object. Now that I am describing what I wanted to do, I just realized many ways I could fix it without static variables

And the textureManager was meant to be used for repeat, random textures. It's just that my art skills aren't the greatest, believe it or not, so I never got around to that

Thanks for taking time to read through the bits of my code I thought was iffy. Really appreciated

[Estivo]

I looked at your code a little.

In Entity.cpp - If you use something more than once, I think it's better to store it ( getGlobalBounds could do something extra each call )

        auto globalBounds = entSprite.getGlobalBounds();
        entSprite.setOrigin(globalBounds.width / 2, globalBounds.height / 2);

In textureManager.cpp

sf::Texture* textureManager::addTexture(const std::string &filepath, const std::string &name)
{
        sf::Texture* newTexture = getTexture(name);
        if( newTexture )
                return newTexture;

        newTexture = new sf::Texture;

        if(newTexture->loadFromFile(filepath)) // otherwise, load a new texture from desired path
        {
                s_allTextures[name] = std::make_shared<sf::Texture>(*newTexture); // and then add it to the map
                return newTexture;
        }

        std::cout << "Could not load " << filepath << std::endl;    // if it can't load, filepath didn't work
        return nullptr;
}

sf::Texture* textureManager::getTexture(const std::string &name)
{
        auto ret = m_allTextures.find(name);
        if( ret != m_allTextures.end() )
        {
                return ret->second;
        }

        return nullptr;
}

I didn't test it. I wrote getTexture more clear. Same with creating new Texture

Old

sf::Texture *newTexture = new sf::Texture;
newTexture = getTexture(name);

if (newTexture)       // check if getTexture doesnt return nullptr
        {
                return newTexture;    // if it does, return it
        }

newTexture = new sf::Texture;

if (newTexture->loadFromFile(filepath)) // otherwise, load a new texture from desired path
        {
                allTextures[name] = std::make_shared<sf::Texture>(*newTexture); // and then add it to the map
                return allTextures[name].get();
        }

std::cout << "Could not load " << filepath << std::endl;    // if it can't load, filepath didn't work
return nullptr;
 

Here - sf::Texture *newTexture = new sf::Texture; - you created a new texture and than ?
BUM - newTexture = getTexture(name); - you request a pointer to texture (old texture wasn't released)
and 7 lines under BAM - newTexture = new sf::Texture; again new texture ( this time is needed ). So you had mem leak.

In collision.cpp

sf::Vector2f clsn::handleCollision(sf::FloatRect &AABBFirst, sf::FloatRect &AABBSecond)   // A is object colliding. I.E, player and the wall
{
        sf::Vector2f ret(0, 0);
        sf::Vector2f centerA(AABBFirst.left + (AABBFirst.width / 2), AABBFirst.top + (AABBFirst.height / 2));
        sf::Vector2f centerB(AABBSecond.left + (AABBSecond.width / 2), AABBSecond.top + (AABBSecond.height / 2));

        sf::Vector2f distance(centerA - centerB);
        sf::Vector2f minDistance((AABBFirst.width / 2) + (AABBSecond.width / 2), (AABBFirst.height / 2) + (AABBSecond.height / 2));

        if (abs(distance.x) < minDistance.x && abs(distance.y) < minDistance.y)
        {
                ret = sf::Vector2f(distance.x > 0 ? minDistance.x - distance.x : -minDistance.x - distance.x,
                                                   distance.y > 0 ? minDistance.y - distance.y : -minDistance.y - distance.y);
        }

        return ret;
}

You could have just 1 return.

I don't have time for checking whole code. Keep going.