1
\$\begingroup\$

I am trying to load a SDL texture through my class's constructor. All I'm doing is calling a function from the constructor to load the texture. As mentioned in the title, this does not seem to work. However, if I call the function directly then it loads fine and I can see it on the screen. I'm fairly new to C++ so please let me know what I'm doing wrong.

Game.h

#include "Player.h"

class Game {
  private:
   Player player;

  public:
   Game();
   ~Game();

   void update();
   void draw();
}

Game.cpp

#include "Game.h"

Game::Game() {
  this->player = Player("somefile.png", 0, 0, 10, 10);  // Doesn't draw
  this->player.load("somefile.png", 0, 0, 10, 10) // Will draw
}

void Game::update() { /* ... */ }

void Game::draw() { /* ... */ }

Player.h

class Player {
  private:
    SDL_Texture* texture;
    SDL_Rect source, destination;

  public:
    Player();
    Player(const std::string& filepath, const int x, const int y, const int w, const int h);
    ~Player();

    void load(const std::string& filepath, const int x, const int y, const int w, const int h);

    void update();
    void draw();
  }

Player.cpp

#include "Player.h"

Player::Player() {}

Player::Player(const std::string& filepath, const int x, const int y, const int w, const int h) {
  this->load(filepath, x, y, w, h);
}

void Player::load(const std::string& filepath, const int x, const int y, const int w, const int h) {
  this->texture = SDL_CreateTextureFromSurface(renderer, IMG_Load(filepath.c_str());

  if (this->texture != nullptr) {
    this->source = { x, y, w, h};
    this->destination = { 0, 0, w, h};
  }
  else {
    std::cout << "Could not load image." << std::endl;
  }
}

void Player::update() { /* ... */ }

void Player::draw() { /* ... */ }

In Game.cpp, if I use the line with the constructor then nothing draws in the window. If I use the line that has load(), then it works fine. I never get the Could not load image. error in either cases.

\$\endgroup\$
  • \$\begingroup\$ is the constructor running before you call SDL_INit maybe? \$\endgroup\$ – Jimmy Oct 23 '17 at 22:38
  • \$\begingroup\$ I call SDL_Init before I do any setup for the player, so that shouldn't be the case. \$\endgroup\$ – o.o Oct 23 '17 at 22:52
3
\$\begingroup\$

It seems that you implemented ~Player() to deallocate your texture.

Anyway, it is rather bad style to copy objects which contain free able resources like here:

this->player = Player("somefile.png", 0, 0, 10, 10);

What here happens is:

  • A new temporary Player object is constructed with given parameters. Here the constructor calls Player::load(...)
  • The object is binary copied (field by field) into this->player (notice that the texture pointer value is copied, no new texture is created)
  • The destructor ~Player() is called on the temporary (presumably deallocating the texture)

To fix this you have a few options:

You could use generally just the Player::load(...) function for the image loading. And never do a create-copy assignment as above.

Or using the right constructor when you actually construct it:

class Game {
  Player player("somefile.png", 0, 0, 10, 10);
  ...
}

Or if you need arguments using the initializer list:

Game::Game(const std::string& filepath) : player(filepath, 0, 0, 10, 10) {

}

Here in both snippets is no temporary object involved which would cause an unwanted destructor call.

Alternatively if you have at least C++11 support, using move semantics. But like Quentin pointed out, you have to implement the move constructor yourself. It might be easier to just disable the copy-constructor.

The difference with move semantics is that the temporary object and this->player would be swapped so the destructor is called on the old swapped out content.

BTW since C++11 you could also disable the copy semantics on a class which should not be copyable: (Quentin noted that in C++03 you might also just declare the copy-constructor private)

class Player {
  Player(const Player&) = delete;
  Player& operator=(const Player&) = delete;
  ...
}
\$\endgroup\$
  • \$\begingroup\$ I was destroying the texture in ~Player(). I should have mentioned that, sorry. Thank you for the detailed response, it was really helpful! \$\endgroup\$ – o.o Oct 24 '17 at 0:14
  • 1
    \$\begingroup\$ std::move is superfluous here, since we're talking about a temporary. You do have to implement the move constructor yourself though. In C++03, declaring the copy-constructor private and not defining it is a good enough emulation of a non-copyable type. \$\endgroup\$ – Quentin Oct 24 '17 at 8:31
  • \$\begingroup\$ @Quentin good point, it's sometime since I worked with move semantics in C++. I updated my answer. \$\endgroup\$ – Cryptjar Oct 24 '17 at 12:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.