1

I'm trying to write the Space Invaders clone for Windows console, and it felt like everything was going fine, but here comes this crash. The program compiles without errors, however instantly crashes on start up. I admit that I don't know pointers very well, and I believe that the problem is somewhere there.

#include "stdafx.h"
#include <vector>
#include <random>
#include <chrono>
#include <thread>
#include <memory>
#include <SDKDDKVer.h>
#include "Vector2D.h"
#include "Renderer.h"

std::default_random_engine rGen;
typedef std::uniform_int_distribution<int> intRand;
typedef std::uniform_real_distribution<float> floatRand;

char ObjectType[][64] =
{
    "ot_AlienShip",
    "ot_PlayerShip",
    "ot_AlienLaser",
    "ot_PlayerLaser",
    "ot_Explosion"
};

class PlayField;
class GameObject
{
public:
    char* m_objType = nullptr;
    Vector2D pos;
    unsigned char sprite;

    virtual void Update(PlayField& world) {};
    virtual bool DecreaseHealth() { return true; };
};

class Input
{
public:
    virtual bool Left() = 0;
    virtual bool Right() = 0;
    virtual bool Fire() = 0;
};

class RndInput : public Input
{
public: 
    virtual bool Left() override { floatRand keyRate(0, 1); return (keyRate(rGen) < 0.3f); }
    virtual bool Right() override { floatRand keyRate(0, 1); return (keyRate(rGen) < 0.4f); };
    virtual bool Fire() override { floatRand keyRate(0, 1); return (keyRate(rGen) < 0.5f); };
};

class PlayField
{
private:
    typedef GameObject* GameObjPtr;
    std::vector<GameObjPtr> gameObjects;


public:
    Input* cotrollerInput = nullptr;
    GameObject* it = new GameObject;
    //it = new GameObject;
    Vector2D bounds;
    // Number of available active laser slots for aliens and player
    int AlienLasers = 10;
    int PlayerLasers = 4;

    explicit PlayField(Vector2D iBounds) : bounds(iBounds) {};
    const std::vector<GameObjPtr>& GameObjects() { return gameObjects; }

    void Update()
    {
        // update list of active objects in the world
        for (auto it : gameObjects)
        {
            it->Update(*this);  //The crash is here "Unhandled exception thrown: read access violation. it was 0xFFFFFFFFFFFFFFFF."
        }
    }

    GameObject* GetPlayerObject()
    {
        auto it = std::find_if(gameObjects.begin(), gameObjects.end(), [](GameObjPtr in) { return (strcmp(in->m_objType,"ot_PlayerShip")==0); });
        if (it != gameObjects.end())
            return (*it);
        else
            return nullptr;
    }

    void SpawnLaser(GameObject* newObj)
    {
        if (strcmp(newObj->m_objType, "ot_AlienLaser")==0)
            AlienLasers--;
        else if (strcmp(newObj->m_objType, "ot_PlayerLaser")==0)
            PlayerLasers--;
        AddObject(newObj);
    }
    void DespawnLaser(GameObject* newObj)
    {
        if (strcmp(newObj->m_objType, "ot_AlienLaser")==0)
            AlienLasers++;
        else if (strcmp(newObj->m_objType, "ot_PlayerLaser")==0)
            PlayerLasers++;
        RemoveObject(newObj);
    }

    void AddObject(GameObject* newObj)
    {
        //gameObjectsToAdd.push_back(GameObjPtr(newObj));
        gameObjects.push_back(newObj);
    }
    void RemoveObject(GameObject* newObj)
    {
        //gameObjectsToRemove.push_back(newObj);
        auto it = std::find_if(gameObjects.begin(), gameObjects.end(), [&](GameObjPtr in) { return (in==newObj); });
        gameObjects.erase(it);
    }
};

class Explosion : public GameObject
{
public:
    // Explosion lasts 5 ticks before it dissappears
    int timer = 5;
    Explosion() { m_objType = new char[64];  strcpy(m_objType, "ot_Explosion"); sprite = RS_Explosion; }
    ~Explosion() { delete[] m_objType; }
    void Update(PlayField& world) override
    {
        timer--;
        if (!timer)
        {
            world.RemoveObject(this);
            delete this;
        }
    }
};

class AlienLaser : public GameObject
{
public:
    AlienLaser() { m_objType = new char[64]; strcpy(m_objType, "ot_AlienLaser"); sprite = RS_AlienLaser; }
    ~AlienLaser() { delete[] m_objType; }

    void Update(PlayField& world) override
    {
        bool deleted = false;
        pos.y += 1.f;
        if (pos.y > world.bounds.y)
        {
            deleted = true;
        }
        GameObject* player = world.GetPlayerObject();
        if (player && pos.IntCmp(player->pos))
        {
            deleted = true;
            //Spawn explosion, kill player
            GameObject& no = *(new Explosion);
            no.pos = pos;
            world.AddObject(&no);
            world.RemoveObject(player);
        }

        if (deleted)
        {
            world.DespawnLaser((GameObject*)this);
            delete this;
        }
    }
};

class PlayerLaser : public GameObject
{
public:
    PlayerLaser() { m_objType = new char[64]; strcpy(m_objType, "ot_PlayerLaser"); sprite = RS_PlayerLaser; }
    ~PlayerLaser() { delete[] m_objType; }

    void Update(PlayField& world) override
    {
        bool deleted = false;
        pos.y -= 1.f;
        if (pos.y < 0)
        {
            deleted = true;
        }

        for (auto it : world.GameObjects())
        {
            if (strcmp(it->m_objType,"ot_AlienShip")==0 && it->pos.IntCmp(pos))
            {
                deleted = true;
                //Spawn explosion, kill the alien that we hit
                GameObject& no = *(new Explosion);
                no.pos = pos;
                world.AddObject(&no);
                if (it->DecreaseHealth())
                    world.RemoveObject(it);
            }
        }

        if (deleted)
        {
            world.DespawnLaser(this);
            delete this;
        }
    }
};

class Alien : public GameObject
{
public:
    Alien() { m_objType = new char[64]; strcpy(m_objType, "ot_AlienShip"); sprite = RS_Alien; }
    ~Alien() { delete m_objType; }

private:
    const float maxUpdateRate = 0.01f;
    const float transformEnergy = 1.f;
    enum AlienState
    {
        as_Normal,
        as_Better
    };
    // Variables dictating energy level for upgrade, direction of movement, and current speed
    float health = 1.f;
    float energy = 0.f;
    float direction = 1.f;
    float velocity = 0.5f;
    AlienState state{};

    void Transform()
    {
        state = as_Better;
        sprite = RS_BetterAlien;
        velocity *= 2.f;
    }
    bool DecreaseHealth() override { health -= 1.f; return health <= 0;  }

    void Update(PlayField& world) override
    {
        pos.x += direction * velocity;
        // Border check
        if (pos.x < 0 || pos.x >= world.bounds.x - 1)
        {
            direction = -direction;
            pos.y += 1;
        }

        // Border check vertical:
        if (pos.y >= world.bounds.y - 1)
        {
            // kill player
            GameObject* player = world.GetPlayerObject();
            if (player && pos.IntCmp(player->pos))
            {
                //Spawn explosion
                GameObject& no = *(new Explosion);
                no.pos = pos;
                world.AddObject(&no);
                world.RemoveObject(player);
            }
        }

        // Transform into better Alien
        if (state!=as_Better)
        {
            floatRand updateRate(-maxUpdateRate, 2*maxUpdateRate);
            energy += updateRate(rGen);
            if (energy >= transformEnergy)
                Transform();
        }

        floatRand fireRate(0, 1);
        if (fireRate(rGen) < 0.5 && world.AlienLasers>0)
        {
            //Spawn laser
            GameObject& newLaser = *(new AlienLaser);
            newLaser.pos = pos;
            world.SpawnLaser(&newLaser);
        }
    }
};

class PlayerShip : public GameObject
{
public:
    PlayerShip() { m_objType = new char[64]; strcpy(m_objType, "ot_PlayerShip"); sprite = RS_Player; }
    ~PlayerShip() { delete m_objType; }

    void Update(PlayField& world) override
    {
        if (world.cotrollerInput->Left())
            pos.x -= 1;
        else if (world.cotrollerInput->Right())
            pos.x += 1;

        if (world.cotrollerInput->Fire() && world.PlayerLasers>0)
        {
            //Spawn laser
            GameObject& newLaser = *(new PlayerLaser);
            newLaser.pos = pos;
            world.SpawnLaser(&newLaser);
        }
    }
};

int main()
{
    rGen.seed(1);
    Vector2D size(80, 28);
    Renderer mainRenderer(size);
    PlayField world(size);

    intRand xCoord(0, (int)size.x-1);
    intRand yCoord(0, 10);

    // Populate aliens
    for (int k = 0; k < 20; k++)
    {
        Alien& a = *(new Alien);
        a.pos.x = (float)xCoord(rGen);
        a.pos.y = (float)yCoord(rGen);
        world.AddObject(&a);
    }
    // Add player
    PlayerShip& p = *(new PlayerShip);
    p.pos = Vector2D(40, 27);
    world.AddObject(&p);

        world.Update();
        {
            RenderItemList rl;
            for (auto it : world.GameObjects())
            {
                RenderItem a = RenderItem(Vector2D(it->pos), it->sprite);
                rl.push_back(a);
            }

            mainRenderer.Update(rl);
            // Sleep a bit so updates don't run too fast
            std::this_thread::sleep_for(std::chrono::milliseconds(1));
        }

    return 0;
}

In addition, VS gives the following info: enter image description here

I assume, that the pointer (or the object) appears to be cleared somewhere before, but I have no idea how to trace it. Thanks in advance.

ivanshv
  • 47
  • 1
  • 2
  • 7
  • 2
    Any way you can make this example minimal? That's a lot of code you're expecting us to debug. – NathanOliver Dec 23 '19 at 16:12
  • 4
    Also, **never** do `PlayerShip& p = *(new PlayerShip);`. If you need an object just get one using `PlayerShip p;` or `PlayerShip p{};`. Doing `PlayerShip& p = *(new PlayerShip);` is just an invitation for trouble (You're leaking memory). – NathanOliver Dec 23 '19 at 16:13
  • 1
    `char* m_objType = nullptr;` -- Why is this not `std::string`? Also -- *The program compiles without errors, however instantly crashes* -- A program compiling successfully only means there are no syntax errors, and that the linker finds all the functions you're calling. It has no bearing on whether or not you have logical errors. – PaulMcKenzie Dec 23 '19 at 16:17
  • 1
    And why is `ObjectType` no enum? Or at least use strings to save you the `strcmp() == 0`. The error probably comes from code like `GameObject& no = *(new Explosion); [...] world.AddObject(&no);` Leaking memory and storing pointers to temporary instances. Do you perhaps come from java-background? – Lukas-T Dec 23 '19 at 16:23
  • 1
    Also, your `GameObject` and `Input` classes are missing virtual destructors, thus you are susceptible to undefined behavior occurring if your code creates derived objects from a base class pointer, and then you `delete` such a pointer. – PaulMcKenzie Dec 23 '19 at 16:24
  • @PaulMcKenzie yes, that's what I meant, that the syntax is fine, but I cannot trace logical errors. Thanks for your comment. – ivanshv Dec 23 '19 at 16:25
  • 1
    `0xdddddddd` is a magic debugging code: [https://stackoverflow.com/a/127404/487892](https://stackoverflow.com/a/127404/487892) – drescherjm Dec 23 '19 at 16:25
  • @churill I do, that's why pointers are dark matter for me. Thanks for your comment, will try to work on that. – ivanshv Dec 23 '19 at 16:29
  • 1
    `delete this;` is generally a good thing to avoid... – ChrisMM Dec 23 '19 at 16:30
  • @NathanOliver-ReinstateMonica Thank you for your comment. Will fix this. And sorry for publishing the whole cpp, I just wasn't sure which parts are necessary. Sorry. – ivanshv Dec 23 '19 at 16:30
  • 2
    @ivanshv You should strive to lessen, if not eliminate the usage of raw pointers. Use containers (which you seem to be doing), and smart pointers such as `std::unique_ptr` and if it seems to fit your use case, `std::shared_ptr`. There should be no `new[]` calls in user code in this day and age of C++, but your code has them. Using `new[]` is superseded by `std::vector` or similar container. – PaulMcKenzie Dec 23 '19 at 16:31
  • 2
    `for (auto it : world.GameObjects())` -- Undefined behavior, since you cannot "adjust" `GameObjects` (remove / add entries) in a range-based `for` loop if the iterator references `GameObjects`. As the answer given to you states, better to have a member variable that's a boolean, determining if the object is still alive. Then you can use [something like this](https://stackoverflow.com/questions/29703553/c-vector-object-erase/29703829#29703829) to erase the objects. – PaulMcKenzie Dec 23 '19 at 16:48

1 Answers1

3

The various ::Update methods you have add and remove objects from gameObjects while you are traversing it in Playfield::Update. This is a guaranteed crash as it invalidates the implicit iterator in your for loop.

To solve this, either:

  • Have GameObject::Update return a boolean that signifies if the object can be removed or not. This requires you to rewrite the loop in Playfield::Update with explicit iterators so you an do it = gameObjects.erase(it);. This will still not allow you to add new objects, though.
  • Defer additions and removals until the end of the game loop. Add a markForAddition / markForRemoval method that will remove these objects from the game world after Playfield::Update has gone through. You will need to do some extra bookkeeping to make sure you do not update or draw objects that have been removed earlier in the same loop, but that is surmountable.
  • Switch data structures: a std::list does not invalidate its iterators if you remove an object somewhere. You do still need to be careful with the current element though.
Botje
  • 26,269
  • 3
  • 31
  • 41