1
\$\begingroup\$

I am working on a top down 2d shooter game. I have a player Sprite and a Bullet class. I made a vector of pointers to Bullet to store the bullets. Here is the code of the vector and the functions I’m using:

    std::vector<std::unique_ptr<Bullet>> b;


    while(win.isOpen)
    {
    //Getting Key Press
    if (Keyboard::isKeyPressed(Keyboard::Space))
    {
        space = true;
    }
    else
    {
        space = false;
    }
    if (space)
    {
        Bullet b1;
        b1.setPos(player.getPosition());
        b.push_back(std::make_unique<Bullet>(b1));
    }



    for (auto& it : b)
    {
        //Keep Updating
        it->update();   
    }

    //Bullet out of Screen
    for (int i = 0; i < b.size(); i++)
    {
        if (b[i]->getPos().y < 0.0f)
        {
            b.erase(b.begin() + i);
            i--;
        }
    }

    //Drawing Bullet
    for (auto& i : b)
    {
        win.draw(i->getSprite());
    }
     }

The game runs fine but the bullet’s texture won’t load. I’m assuming it has something to do with the vector but I don’t know how to solve it. Here is the Bullet class: Header:

 #pragma once
 #include <SFML/Graphics.hpp>

 using namespace sf;

 class Bullet
 {
private:
Texture t;
Sprite bullet;
Vector2f pos;
public:
Bullet();
void setPos(Vector2f pos);
void update();
Sprite& getSprite();
Vector2f getPos();
};

Cpp File: #include "pch.h" #include "Bullet.h" #include

Bullet::Bullet()
{
//Setting Texture
if (!t.loadFromFile("Graphics/bullet.png"))
{
    std::cout << "Bullet Texture Failed to Load\n";
}
else
{
    t.loadFromFile("Graphics/bullet.png");
    bullet.setTexture(t);
}

}

void Bullet::setPos(Vector2f pos)
{
//Setting Position
this->pos = pos;
bullet.setPosition(this->pos);
}


void Bullet::update()
{
pos = bullet.getPosition();

bullet.move(0.0f, -4.0f);

}

Sprite& Bullet::getSprite()
{
return bullet;
}

Vector2f Bullet::getPos()
{
return pos;
}
\$\endgroup\$
3
  • \$\begingroup\$ What if you used a unique ptr for the texture in your bullet instead of a stack object? I'll be able to write an answer later tonight. \$\endgroup\$
    – Vaillancourt
    Commented Sep 29, 2018 at 12:35
  • \$\begingroup\$ Also ask yourself if you really need the unique_pointers. Unless you need them, they will just be inefficient and less reliable compared to storing them directly in a vector of Bullet objects. \$\endgroup\$
    – simulate
    Commented Sep 30, 2018 at 23:23
  • 1
    \$\begingroup\$ @stimulate Yeah, for now, I'd call it premature optimization. Look at how the bullets are deleted. Moving pointers is more efficient than moving objects. I'd focus on something that works. "Efficiency" will come later when they have more experience with this. \$\endgroup\$
    – Vaillancourt
    Commented Oct 1, 2018 at 16:55

1 Answer 1

2
\$\begingroup\$

Why is there this issue?

The issue is here:

if (space)
{
  Bullet b1;
  b1.setPos(player.getPosition());
  b.push_back(std::make_unique<Bullet>(b1));
}

Let's look at the Bullet class (the relevant parts):

class Bullet
{
private:
  Texture t;
  Sprite bullet;

public:
  Bullet()
  {
    //Setting Texture
    if (!t.loadFromFile("Graphics/bullet2.png"))
    {
      std::cout << "Bullet Texture Failed to Load\n";
    }
    else
    {
      t.loadFromFile("Graphics/bullet2.png");
      bullet.setTexture(t);
    }      
  }
};

and the Sprite class (the relevant parts):

class SFML_GRAPHICS_API Sprite : public Drawable, public Transformable
{
public:
    Sprite() : 
      m_texture (NULL) {}
    void setTexture(const Texture& texture, bool resetRect = false) 
    { 
      m_texture = &texture; 
    }
    const Texture* m_texture;     ///< Texture of the sprite
};

When a Bullet instance is created, the object is created on the stack. The issue comes when you add it to the vector and then get out of the scope. It's most likely moving the objects within to a new location, moving the Sprite and the Texture. But notice that the Sprite has a reference to the texture? This texture will be deleted when we'll come out of the scope of the if (space). Even if we move everything, there is nothing to tell the Sprite that the Texture has moved.

How to solve it?

The trivial solution is to create your Bullet on the heap right away, using std::make_unique:

if (space)
{
  b.push_back(std::make_unique<Bullet>());
  b[b.size()-1]->setPos (player.getPosition());
}

This way, the references will always stay in place in memory and the image will show.

Doing this will "fix" your issue. But the next step is: you'll notice that when you spawn a Bullet, there is some slight lag.

This is most likely due to the lines:

if (!t.loadFromFile("Graphics/bullet2.png"))
{
  std::cout << "Bullet Texture Failed to Load\n";
}
else
{
  t.loadFromFile("Graphics/bullet2.png");
  bullet.setTexture(t);
}

You're reading a file twice. IO operations are the most costly operations on a system. We can fix this by loading all of our textures at startup, and then passing a reference to them to our Bullet instance.

We'll then end up with something like this in your main():

...
//Creating the Vector
std::vector<std::unique_ptr<Bullet>> b;
std::map<std::string, std::unique_ptr<Texture>> textures;

auto load_texture = [&]( std::string texture_path, std::string texture_id ) {

  std::unique_ptr<Texture> new_texture = std::make_unique<Texture>();

  if ( new_texture->loadFromFile( texture_path ) )
  {
    textures[texture_id] = std::move( new_texture );
  }
  else
  {
    std::cout << "Failed to load \"" << texture_id << "\" for path \"" << texture_path << "\".\n";
  }
};

load_texture( "Graphics/bullet2.png", "bullet" );

bool space = false;

...

if (space)
{
  auto texture = textures.find( "bullet" );

  b.push_back(std::make_unique<Bullet>(*texture->second));
  b[b.size()-1]->setPos (player.getPosition());
}

...

And a Bullet class like this:

class Bullet
{
private:
  Texture& t;
  Sprite bullet;
  Vector2f pos;
public:
  Bullet(Texture& texture);
  ~Bullet();
  void setPos(Vector2f pos);
  void update();
  Sprite getSprite();
  Vector2f getPos();
};

Bullet::Bullet(Texture& texture)
  : t(texture)
{
  bullet.setTexture(t);
}

The added advantage here is that you'll be using less memory (you really don't need to have the same image multiple times in memory).


This is based on the code you supplied on your other answer. I have uploaded the relevant changes to a github repository.

\$\endgroup\$

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .