Destructor sometimes called on static object

[jaybee]

I was writing a music/sound manager class the other day and it has a destructor that first stops all music or sounds that are currently playing and deletes the objects that are stored in a std::map. Sometimes with my manager classes I like to do something like this:

Code: Select all

class Manager
{
public:
     static Manager ManagerControl;
     Manager();
     ~Manager();
public:
     void DoSomething();
     void DoSomethingElse();

private:
     int _x;
     int _y;
};

This way the class has a static instance of itself I can access from wherever I need to. Maybe this is a bad way of doing it but it seems to work. Originally I didn't have it set up like this but I decided to change it to that method. I assumed that this meant I needed a seperate clean up function because of the new static instance, but I noticed that about every other time that I exit, the destructor gets called anyway. When it doesn't get called the game hangs because (I think) sfml is still trying to loop the music.

Why would the destructor be called sometimes on a static instance?

[Falco Girgis]

It should ALWAYS be called. You sure there isn't some conditional logic in there that only causes it to hang sometimes? Try a breakpoint/printf/cout, and you should find the destructor being invoked consistently.

[dandymcgee]

If you're going to implement the Singleton Pattern at least do it correctly.

Or even better, don't do it at all [see: search.php?st=0&sk=t&sd=d&sr=posts&keywords=singleton for reasons]

[jaybee]

Hmmmm... it looks like you're right. It is getting called, it just seems to take quite a while sometimes for it to finish. That's the reason I thought it wasn't getting called. Sometimes it just finishes really quickly and sometimes it takes like 30 - 40 seconds.

Here is the code:

Code: Select all

SoundManager::~SoundManager()
{
    std::cout<<"SoundManager destructor called"<<std::endl;
    typedef std::map<std::string, sf::Music*>::iterator it;
    for(it i = _music.begin(); i != _music.end(); i++)
    {
        i->second->stop();
        std::cout<<i->first<<" stopped"<<std::endl;
    }
    std::for_each(_sounds.begin(),_sounds.end(), SoundDeallocator());
    std::for_each(_music.begin(),_music.end(), MusicDeallocator());
}

Am I missing something obvious?

[jaybee]

If you're going to implement the Singleton Pattern at least do it correctly.

Or even better, don't do it at all [see: search.php?st=0&sk=t&sd=d&sr=posts&keywords=singleton for reasons]

Yeah, I've heard that it isn't a good idea. I know nothing about correctly implementing ANY design patterns. I've looked into it a bit but a lot of times the examples are too complicated or so conceptual that I'm not able to figure out how to apply it to my projects.

What exactly is incorrect about an object with a static instance of itself?

[Nokurn]

If you're going to implement the Singleton Pattern at least do it correctly.

Or even better, don't do it at all [see: search.php?st=0&sk=t&sd=d&sr=posts&keywords=singleton for reasons]

Yeah, I've heard that it isn't a good idea. I know nothing about correctly implementing ANY design patterns. I've looked into it a bit but a lot of times the examples are too complicated or so conceptual that I'm not able to figure out how to apply it to my projects.

What exactly is incorrect about an object with a static instance of itself?

The order in which global objects (such as an instance static) are initialized is not guaranteed. You may have a singleton that depends on another singleton, but the dependent singleton is initialized first. By using lazy evaluated singletons, you ensure that singletons are initialized in the proper order (at least, in single-threaded code). It also guarantees destruction. If your code is single-threaded, the correct way to go about implementing a singleton goes something like this:

class Singleton {
public:
    static Singleton& instance();

private:
    Singleton();

    // These declarations ensure that the
    // Singleton class is non-copyable.
    // Attempting to copy through a
    // copy-constructor or assignment
    // operator will result in compiler errors
    // (due to the private declarations) for
    // client code and linker errors for
    // Singleton implementation code (due to
    // these declarations not being defined).
    //
    // In C++11, these would be marked =delete.
    Singleton(const Singleton&);
    Singleton& operator =(const Singleton&);
};

Singleton& Singleton::instance()
{
    static Singleton inst;
    return inst;
}

Singleton::Singleton()
{
    // By using a private constructor, you ensure that
    // the class may only be instantiated within itself.
    // In this case, it is not possible to instantiate a
    // Singleton in client code, so the only Singleton
    // instance in the entire program is the one
    // instantiated in the Singleton::instance() function.
}

As for your destructor problem, place debug prints around each for_each loop to help narrow it down. Unless sf::Music::stop() is the source of the issue, I suspect that your Deallocator functors are the problem.

Edit: I would like to include my recommendation against using a singleton. I would advise the use of individual sound managers that store related pools of sounds and directly passing the managers to their users. This would allow you to implement more fine-grained control of resources; you could have a pool of sounds that must be available for the duration of the program's execution and pools that exist only for a particular level's lifetime, which are destroyed when the level is completed.

[jaybee]

The order in which global objects (such as an instance static) are initialized is not guaranteed. You may have a singleton that depends on another singleton, but the dependent singleton is initialized first. By using lazy evaluated singletons, you ensure that singletons are initialized in the proper order (at least, in single-threaded code). It also guarantees destruction.

I didn't realize this. Makes sense.

If your code is single-threaded, the correct way to go about implementing a singleton goes something like this:

class Singleton {
public:
    static Singleton& instance();

private:
    Singleton();

    // These declarations ensure that the
    // Singleton class is non-copyable.
    // Attempting to copy through a
    // copy-constructor or assignment
    // operator will result in compiler errors
    // (due to the private declarations) for
    // client code and linker errors for
    // Singleton implementation code (due to
    // these declarations not being defined).
    //
    // In C++11, these would be marked =delete.
    Singleton(const Singleton&);
    Singleton& operator =(const Singleton&);
};

Singleton& Singleton::instance()
{
    static Singleton inst;
    return inst;
}

Singleton::Singleton()
{
    // By using a private constructor, you ensure that
    // the class may only be instantiated within itself.
    // In this case, it is not possible to instantiate a
    // Singleton in client code, so the only Singleton
    // instance in the entire program is the one
    // instantiated in the Singleton::instance() function.
}

I appreciate the example. That is really helpful. I've Implemented a manager similar to this in a shoot em up game I wrote a while back but in all honesty, I had no idea why I was doing it. I came across it reading someone elses code and it looked like an easy way to access something globally.

As for your destructor problem, place debug prints around each for_each loop to help narrow it down. Unless sf::Music::stop() is the source of the issue, I suspect that your Deallocator functors are the problem.

I did this and it is definitely the for loop. The functors are fine. I just ended up removing it and it seems to work. I'm not sure how sfml handles music internally and if it even really matters if I delete the sound without stopping it. I'm not getting any errors so I assume its ok.

Edit: I would like to include my recommendation against using a singleton. I would advise the use of individual sound managers that store related pools of sounds and directly passing the managers to their users. This would allow you to implement more fine-grained control of resources; you could have a pool of sounds that must be available for the duration of the program's execution and pools that exist only for a particular level's lifetime, which are destroyed when the level is completed.

So you mean, having maybe a sound manager for the game class and individual sound managers for each level or area? Then if I needed to have a sound when the character shoots or jumps or something like that, just pass a reference to character for the appropriate sound manager? I suppose that will end up making the dependencies more explicit.

I know there is a good reason for not using singletons and globally available objects but I am just missing it. I would imagine understanding that peice of the puzzle is the key for me to start designing better code. Can you point me in the direction of some design patterns that would be beneficial for me to start studying?

[Nokurn]

I'm not sure how sfml handles music internally and if it even really matters if I delete the sound without stopping it. I'm not getting any errors so I assume its ok.

This was my next guess.

I know there is a good reason for not using singletons and globally available objects but I am just missing it. I would imagine understanding that peice of the puzzle is the key for me to start designing better code. Can you point me in the direction of some design patterns that would be beneficial for me to start studying?

Off the top of my head, as someone who spends a lot of time writing libraries, I would recommend studying...

• Resource acquisition is initialization (RAII)
• The bridge pattern (also known as the Pimpl idiom)
• The observer pattern (known as signals/slots in Qt, events/delegates in C#)
• The curiously recurring template pattern (CRTP; I would strongly recommend reading this if you write any substantial amount of template code)
• The single responsibility principle (SRP; one of the most important)
• The "don't repeat yourself" principle (DRY; another important one)
• The "keep it simple, stupid" principle (KISS; also important)

General advice: Use the language to your advantage, watch your pointers, don't violate encapsulation, split things into specialized chunks, and don't prematurely optimize unless you're working on an embedded or legacy platform.

Design Patterns: Elements of Reusable Object-Oriented Software is an excellent book on exactly what its title states, I would advise getting it if you're serious about programming.

[Falco Girgis]

I know there is a good reason for not using singletons and globally available objects but I am just missing it. I would imagine understanding that peice of the puzzle is the key for me to start designing better code. Can you point me in the direction of some design patterns that would be beneficial for me to start studying?

It's an ongoing philosophical debate regarding the object-oriented programming paradigm.

One of the qualities of good object-oriented design is to avoid global state. State data should be encapsulated within an object. In the C language, globals were just a normal part of writing in the language. Within C++, they are frowned upon, as they make it harder to trace and register dependencies among functions and data.

A singleton is nothing more than an ugly global state wrapped around a pretty little object-oriented accessor method. It gives the illusion of good design, but you are actually committing an object-oriented sin by using this design pattern. Most of my colleagues (and myself included) consider the singleton an antipattern. At first it seems like an incredibly easy and versatile way to access data from anywhere, but once your code-base grows, being able to pull references to objects out of your asshole at an point during execution becomes an untraceable mess.

I discussed a pretty useful alternative to the Singleton design pattern that we use here:
http://elysianshadows.com/phpBB3/viewto ... ton#p82674

[dandymcgee]

At first it seems like an incredibly easy and versatile way to access data from anywhere, but once your code-base grows, being able to pull references to objects out of your asshole at an point during execution becomes an untraceable mess.

I actually had to do a global replace on "MainController.ActiveGroup" today in my codebase at work. I was using the MainController's group as a public global variable and making assumptions about in other parts of the application with independent states. No matter how good of an idea it seems at the time, globally accessible variables will often come back to bite you later.

[jaybee]

Thanks for the replies guys. That blog post with the engine code is really interesting. Looks like I've got a lot of studying up to do on design patterns.

Sometimes I've wondered if I should just go about my business coding like a dumbass if for no other reason than to fall flat on my face and understand how my implementation fails.

At any rate, thanks guys. Glad to see you're still kicking it on this board.

[Falco Girgis]

Sometimes I've wondered if I should just go about my business coding like a dumbass if for no other reason than to fall flat on my face and understand how my implementation fails.

Hey, it worked for me.

[jaybee]

I discussed a pretty useful alternative to the Singleton design pattern that we use here:
http://elysianshadows.com/phpBB3/viewto ... ton#p82674

Shit dude I just went back and read this thread one more time and I just got your point. That's pretty slick. In my case, dealing with a sound manager, almost everything needs to make sound, even things that are already inhereting from a base class. How do you keep things from getting out of hand with situations like that?

Lets say I've got a GameObject class and a Character class that inherits from it. Would I simply make GameObject inherit from SoundManager to avoid multiple inheritance hell?

[Falco Girgis]

I discussed a pretty useful alternative to the Singleton design pattern that we use here:
http://elysianshadows.com/phpBB3/viewto ... ton#p82674

Shit dude I just went back and read this thread one more time and I just got your point. That's pretty slick. In my case, dealing with a sound manager, almost everything needs to make sound, even things that are already inhereting from a base class. How do you keep things from getting out of hand with situations like that?

Lets say I've got a GameObject class and a Character class that inherits from it. Would I simply make GameObject inherit from SoundManager to avoid multiple inheritance hell?

Yeah, that's what I would do, provided almost every GameObject needs access to the SoundSystem pointer or whatever.

[MarauderIIC]

I discussed a pretty useful alternative to the Singleton design pattern that we use here:
http://elysianshadows.com/phpBB3/viewto ... ton#p82674

Shit dude I just went back and read this thread one more time and I just got your point. That's pretty slick. In my case, dealing with a sound manager, almost everything needs to make sound, even things that are already inhereting from a base class. How do you keep things from getting out of hand with situations like that?

Lets say I've got a GameObject class and a Character class that inherits from it. Would I simply make GameObject inherit from SoundManager to avoid multiple inheritance hell?

Yeah, that's what I would do, provided almost every GameObject needs access to the SoundSystem pointer or whatever.

You could do

Code: Select all

              GameObject
       |                         |
SoundObject : SoundManager   SilentObject

If you really wanted