-1

I am building a C++ game using SDL2. Everything was going well and I was able to make the player shoot a single bullet. I later modified my code to render multiple bullets as shown below.

Main game loop

void Game::gameLoop(){
    Entity player;
    Entity bulletHead;
    Entity *bulletTail = new Entity();

    bulletTail = &bulletHead;

    player.x = 500;
    player.y = 640;
    player.dx = 0;
    player.dy = 0;
    player.texture = loadTexture((char*)"res/images/player.png");
    SDL_QueryTexture(player.texture, NULL, NULL, &player.w, &player.h);

    SDL_Texture *bulletTexture = loadTexture((char*)"res/images/bullet.png");

    while(gameState != GameState::EXIT){
        prepareScene(); // sets up rendering
        handleEvents(); // collects and precesses user input

        player.x += player.dx;
        player.y += player.dy;

        // Player key input
        if(player.reload > 0) player.reload--;

        if (up)
        {
            player.y -= 4;
        }

        if (down)
        {
            player.y += 4;
        }

        if (left)
        {
            player.x -= 4;
        }

        if (right)
        {
            player.x += 4;
        }

        // allow fire bullet every 8 frames
        if(fire && player.reload == 0){
            player.reload = 8;

            // Create bullet
            Entity *bullet = new Entity();
            memset(bullet, 0, sizeof(Entity));

            bulletTail->next = bullet;
            bulletTail = bullet;

            bullet->x = player.x + 8;
            bullet->y = player.y;
            bullet->dx = 0;
            bullet->dy = -PLAYER_BULLET_SPEED;
            bullet->health = 1;
            bullet->texture = bulletTexture;
            SDL_QueryTexture(bullet->texture, NULL, NULL, &bullet->w, &bullet->h); 
        }

        // handle physics and render for each bullet
             Entity *b = new Entity();
             Entity *prev = new Entity();
             prev = &bulletHead;
// Error starts here  
             for (b = bulletHead.next ; b != NULL ; b = b->next){
                 b->x += b->dx;
                 b->y += b->dy;

                 blit(b->texture, b->x, b->y);
            
                 if(b->y < 0){
                    if (b == bulletTail)
                    {
                        bulletTail = prev;
                    }
                    prev->next = b->next;
                    free(b);
                    b = prev;
                 }

                 prev = b;

             }
            
// Error stops here 

        blit(player.texture, player.x, player.y); // display image


        presentScene(); // displays scene
        SDL_Delay(16); // limits fps to around 62fps
    }

    SDL_DestroyWindow(window);
    SDL_Quit();
}

As can be seen above, I can successfully create bullets but whenever I include the for loop to go through my linked list of bullets and move/draw them, the program closes unexpectedly. No error is shown, it just closes as soon as I ran it. Removing the for loop works but I need a way to loop through the linked list of bullets and draw/animate them.

genpfault
  • 51,148
  • 11
  • 85
  • 139
lool
  • 345
  • 1
  • 6
  • 11
  • 1
    Is there a particular reason you're not using standard containers like `std::vector`? Can't have bugs in code you don't have to write. – Botje Apr 06 '23 at 12:53
  • 1
    `std::vector bullets;` Problem solved. No need to write your own containers (which in any case is badly bugged), when it has already been done for you. – john Apr 06 '23 at 13:00
  • @Botje I had tried to use vector but in the loop I check to see if the bullet is out of the screen and if it is I free/delete it from dynamic memory. I am not sure if clearing the vector would release memory. – lool Apr 06 '23 at 13:01
  • 1
    @lool A vector will release memory, that's how it is designed. But note the correct code is `std::vector bullets;` not `std::vector bullets;`, then you might have trouble releasing memory. Too many beginners try to use pointers everywhere. The way to avoid pointer problems is not to use pointers. – john Apr 06 '23 at 13:01
  • My quick guess is that your bullets don't have a `next` pointer initialized, which means that it does point to *something* instead of `NULL`. But as the other guy said, don't debug that, use a container. Since you plan on doing lots of deletions, `std::list` might technically be a better choice, but most likely it won't be that important. – Aziuth Apr 06 '23 at 13:03
  • Also, in general, aside from john saying that you should avoid using pointers when not necessary, in situations in which you do want to use pointers, consider using smart pointers (`std::shared_ptr` and `std::unique_ptr`). Because every time you do manual memory management is a time when you can easily make a mistake. – Aziuth Apr 06 '23 at 13:06
  • @john ok I'll try to use a vector, I was just confused about using a vectors erase method to clear memory after seeing [this](https://stackoverflow.com/questions/10464992/c-delete-vector-objects-free-memory) – lool Apr 06 '23 at 13:13
  • @lool All memory occupied by the vector will be released when the vector goes out of scope, that is guaranteed. It is true that calling `clear` will not necessarily release all the memory immediately, but that doesn't mean it's a memory leak. This should not be a concern for you, unless you are creating literally millions of bullets then precisely when the memory gets released should not be a concern as long as it gets released eventually (which it will). And even then the swap trick explained in the first answer is for all intents and purposes a guaranteed immediate release of memory. – john Apr 06 '23 at 13:37

1 Answers1

0

The standard library has lots of containers for you to use, for example std::vector. The examples below assume you have a std::vector<Entity> bullets. If you want to walk a vector and have the option to remove items as you go, there are two options, you can use the erase method, which removes an item from the vector and returns a new iterator.

To use:

for (auto it = bullets.begin(); it != bullets.end(); /* no-op, see below */) {
  // do stuff with *it
  if (/* bullet needs to be removed */) {
    it = bullets.erase(it);
  } else {
    // advance to the next item
    it++;
}
HolyBlackCat
  • 78,603
  • 9
  • 131
  • 207
Botje
  • 26,269
  • 3
  • 31
  • 41
  • My problem is that if I use the erase method, it will remove the bullet from the vector but it wont free memory according to [this](https://stackoverflow.com/questions/10464992/c-delete-vector-objects-free-memory) – lool Apr 06 '23 at 13:29
  • @lool I think you are misunderstanding what you are reading, or maybe you are concerned about things you do not need to be concerned about. Either way the above code is correct, and does not leak memory. Perhaps you need to state more explicitly what your concern is. – john Apr 06 '23 at 13:42