I get a Access violation reading location error every time I close my program.

[kong288]

Tower.h

#ifndef TOWER_H
#define TOWER_H

#include <SFML\Graphics.hpp>
#include <SFML\Graphics.hpp>
#include <vector>

class Tower
{
public:
	
	sf::Texture TowerTxtr;
	sf::Sprite TowerSpr;


	void Setup();

};

void TowerSetup();
void TowerCreate(sf::RenderWindow& Window);
void TowerRotate();
void TowerDraw(sf::RenderWindow& Window);
void TowerMain(sf::RenderWindow& Window);


#endif 

Tower.cpp

#include <SFML\Graphics.hpp>
#include <SFML\Audio.hpp>
#include <vector>

#include "Tower.h"

std::vector<Tower> Towers;



void TowerCreate(sf::RenderWindow& Window)
{
	bool clicked = false;
	sf::Texture TowerTxtr;

	if(sf::Mouse::isButtonPressed(sf::Mouse::Left) && clicked == false)
	{
		Tower tower;
		TowerTxtr.loadFromFile("assets//TowerTxtr.png");
		tower.TowerSpr.setPosition(sf::Mouse::getPosition(Window).x, sf::Mouse::getPosition(Window).y);
		tower.TowerSpr.setTexture(TowerTxtr);
		Towers.push_back(tower);
		clicked = true;
	}


}

void TowerDraw(sf::RenderWindow& Window)
{
	for(int i = 0; i < Towers.size(); i++)
	{
		Window.draw(Towers[i].TowerSpr);
	}
}

I think it's because of the global vector because after I started using it my program started giving the error. Any ideas?

[Disch]

You are correct. The global is the culprit. But it's also a semi-bug in SFML.

SFML will destroy the OpenGL context when the last render target is destroyed.
SFML will also destroy the OpenGL textures when the associated sf::Texture object is destroyed.

Both of those things are what you'd expect. The problem is.... the OpenGL context must exist for the textures to be destroyed. So if the context is destroyed first you get an access violation when the sf::Texture destructor runs, because it's trying to destroy a texture in a context which no longer exists.



Your global vector is a problem, because each element in the vector own a texture, and it's not destroyed until program shutdown. Your last sf::RenderTarget (and therefore the OpenGL context) is being destroyed earlier than that.

To solve this you need to make sure all your textures are destroyed before the render target is destroyed.

The easiest way to do this is to use a resource manager which owns all the textures (you should be doing this anyway... so that multiple towers can share the same texture). The manager needs to be little more than a vector<> or map<> of sf::Textures. Then before you destroy the last window, simply tell your manager to empty (yourmanager.clear();). This will ensure that all textures get destroyed.


If you don't want to rework this to add a resource manager, you can get away with just doing a clear() on your Towers vector prior to window shutdown. This will also wipe all those textures.

[kong288]

Thanks a lot, it worked! What's the difference betweem Towers.clear() and a resource manager?

Also, is a global vector good practice? Or should I put it somewhere else?

[Disch]

What's the difference betweem Towers.clear() and a resource manager?

Textures are very big. They consume a lot of memory.

Right now your Tower class owns its texture. This is a bad idea because it's very likely that you will have multiple towers, all of which have the same graphic. This means that you are loading the entire image into memory for each tower. As a result, the same image has 6 or 7 copies of it loaded. This is a huge waste.

The idea behind having a resource manager is that nothing in your code owns a texture. But when you need a texture in your code... you can just ask the resource manager for it.

The resource manager would keep track of which resources/textures are loaded, and make sure that one image is only ever loaded once. If multiple different areas of the code request the same image... it can give them all the same copy. That way it doesn't matter how many dozens/hundreds of similar towers you have, they all share the same texture -- the texture only is in memory once.

A simple resource manager is very easy to implement. Something along these lines works well:

#include <map>
#include <string>

class TextureManager
{
private:
  static std::map< std::string, sf::Texture >  textures;  // the map of all loaded textures

public:
  static sf::Texture& get(const std::string& filename)
  {
    if( /* texture isn't loaded yet */ )
    {
      // load texture.  Put it in textures[filename]
    }

    return textures[ filename ];    
  }

  // call this before the OpenGL context ends to avoid the crashing problem.
  static void clear()
  {
    textures.clear();
  }
};

Also, is a global vector good practice? Or should I put it somewhere else?

Global anything is almost always a bad idea. With very few exceptions --- one of which is resource managers (since the whole point is to only have one of them to avoid duplicate resources).

I typically would have a "Game" class which has the vector of towers as one of its members.

[kong288]

Hmm. I thought making the texture static would only make one instance of it. Like this:

class Tower
{
public:
	static sf::Texture * TowerTxtr;
	static sf::Texture * TowerBulletTxtr;
};

Also, I heard making a class that isn't going to be used more than once is a bad idea. (like only make a class if you're going to have more than 1 copy of it. like towers or bullets).

Damn, if you're correct I'll start making more classes.

As for the resource manager, that's genius. I'll totally have to use that next time I remake this.

Thanks a lot for all of this useful info!

[Disch]

Hmm. I thought making the texture static would only make one instance of it. Like this:

Yeah that would also work, but has other downsides... one of which being that now all your towers HAVE to use the same texture.

Also, I heard making a class that isn't going to be used more than once is a bad idea. (like only make a class if you're going to have more than 1 copy of it. like towers or bullets).

Whoever told you that is probably giving that as an extremely generalized (and simplified) guideline to try dissuade you from making lots of useless classes. Their heart is in the right place, but I can't say I agree with their approach.