sf::RenderWindow::close() sometimes keeps process running?

[netrick]

I encountered a problem with close() function of RenderWindow class. Basically, sometimes it destroys window and  destroys process, but sometimes it just destroys window but process is still running eating 50% of cpu.
Of course it may be something with my code, so here it is:

Main.cpp (main loop)

Code: [Select]

cRender render;
cLogger logger;

int main()
{
logger.start();
sf::RenderWindow window(sf::VideoMode(600, 500), "TDS Editor", sf::Style::Titlebar | sf::Style::Close);
render.init(window);
while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
            if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Right))
                cameraX++;
            if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Left))
                cameraX--;
            if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Down))
                cameraY++;
            if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Up))
                cameraY--;

        }
       window.clear(sf::Color::White);
       render.drawMap(window, myMap, cameraX, cameraY);
       window.display();
    }

    logger.end();
    return 0;
}

And while I'm here, exiting by clicking 'X' works as expected. However, i have one function:

Code: [Select]

void cLogger::error(sf::RenderWindow & window, string text)
{
    cLogger logger;
    sf::Font openSans;
    if(!openSans.loadFromFile("data/fonts/SourceSansPro-Regular.ttf"))
    {
        logger.write("Logger only - couldn't load font");
        logger.write(text);
        window.close();
    }
    else
    {
    logger.write(text);
    window.clear(sf::Color::White);
    sf::Text temp(text);
    temp.setFont(openSans);
    temp.setColor(sf::Color::Black);
    window.draw(temp);
    sf::Text temp2("Press any key to exit");
    temp2.setFont(openSans);
    temp2.setColor(sf::Color::Black);
    temp2.setPosition(0, 100);
    window.draw(temp2);
    window.display();
    sf::Event event;
    bool running = true;
    while (running)
    {
        window.clear(sf::Color::White);
        window.draw(temp);
        window.draw(temp2);
        window.display();
        while(window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
            {
                running = false;
            }

            if (event.type == sf::Event::KeyPressed)
            {
                running = false;
            }
        }

    }
    window.close();
    }

}
I use it only for critical errors to avoid crash of application. Well it pops up a screen with error and waits for pressing a key. It always exits when I press any key/click 'X', however sometimes it destroys process (and it goes to the end of main loop in main.cpp because it logs logger.end() message and sometimes it just destroys window, but keeps the process running and never reaches the end of main loop.
For example when that error triggers in render.init(), it always works perfectly. However if error happens in drawMap() it always keeps the process running. I don't fully understand how that window.close() works and I think it may be something with my main loop design but I have no clue. I could just make global variable bool appRunning that I can acces from every file, but I'd like to avoid it.

Basically using logger.error() anywhere outside while (window.isOpen()) loop works perfectly. However using it inside it always keeps the process running after destroying the window. Why? I don't have any clue.

Of course error() function always gets the same window argument (by reference). And its proper window because when error happens in drawMap the error gets displayed etc.

Most likely the problem is that event loop ignores the fact that window is closed. So i don't know how to exit application properly. Using C++ exit (int) doesn't call destructors of automatic objects or something like this, I just heard it is some kind of memory leak but I'm not 100% sure.

Thanks

[eXpl0it3r]

Please make use of the code=cpp tag!

Unfortunatly you don't give a complete and minimal example that reproduces the problem, so we can't really test it.
As for the provided code you're doing several things very wrong in the error() functions.

• You shouldn't use draw outside of a game/draw loop since it'll show then aways for just a single frame.
• You shouldn't use a boolean variable for exiting the while loop if you're not certain that there's a way to exit the loop. And if you want to exit the loop when the window closes why not just use isOpen().
• Event processing should happen before the draw calls.
• It's often adviced to keep the logic and the draw part seperated and not mix them.
• Since a logger is mostly for development, it's better just to use the console output for such things, instead of displaying some text on the window.

So if you want to find out where the application is stuck when the process doesn't close, then simply step into the application with a debuger and run it step by step to see what is going on.

[Andrei15193]

Hello,

 I am getting an error with sf::RenderWindow::close() as well. I'm just running the test code with Visual C++ 2010 and every time I close the window it gives the "has stopped working" error. Here's the code:

#define WINDOWS

#include <SFML/Graphics.hpp>

#ifdef WINDOWS
    // Something I use to hide the console,
    // however even with this line commented it still gives the error
    #pragma comment(linker, "/SUBSYSTEM:windows /ENTRY:mainCRTStartup")
#endif

int main()
{
    sf::RenderWindow window(sf::VideoMode(300, 200), "SFML works!");
    sf::Text text("Hello SFML");

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        window.draw(text);
        window.display();
    }

    return 0;
}
 

Also the title is displayed weird. Sometimes it has some odd characters before "SFML works!" and sometimes it is just made out of odd characters
If I break where it crashes it gives an access violation error and goes to "crt0dat.c"/"crtexe.c" (in debug/release configuration). Also in release configuration the title is displayed correctly.

[EDIT]
A small test I just ran. Calling the RenderWindow instance destructor explicitly fixes the issue. Code:

#define WINDOWS

#include <SFML/Graphics.hpp>

#ifdef WINDOWS
    // Something I use to hide the console,
    // however even with this line commented it still gives the error
    #pragma comment(linker, "/SUBSYSTEM:windows /ENTRY:mainCRTStartup")
#endif

int main()
{
    sf::RenderWindow window(sf::VideoMode(300, 200), "SFML works!");
    sf::Text text("Hello SFML");

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        window.draw(text);
        window.display();
    }
    window.~window();

    return 0;
}
 

I don't know if it will work for others but it did the job for me.
[/EDIT]

[eXpl0it3r]

Are you using release libraries but compile as debug?
Also what version of SFML are you using?

[Andrei15193]

I am using SFML 2.0. I was using release libraries on debug configuration now changed to match libraries accordingly. That fixed the title issue however it still gives the "has stopped working" error if the destructor isn't called explicitly (on both debug and release configurations, with or without the #pragma line commented).

[Nexus]

Try with the latest Git revision. Load a font for the text, there is no more default font.

Calling destructors explicitly is certainly not a fix -- on the contrary, it results in undefined behavior. Don't do that.

[Andrei15193]

Calling destructors explicitly is certainly not a fix -- on the contrary, it results in undefined behavior. Don't do that.

Thanks, captain obvious. To add more to your phrase, it may result in undefined behavior because an empty destructor can be called countless of times and get the wanted result without any undefined behavior (it's just a method). It all depends on what that destructor does, if it has a delete inside, yea I agree it's a bad idea, but if there's nothing like that in it there's nothing to worry about.

I haven't looked into SFML sources to say "Yes, it's 100% safe to do this from my point of view", it worked for me and I may or may not use this "fix" until a proper fix is released when it will be released, if it will be released.

[Nexus]

To add more to your phrase, it may result in undefined behavior

No, it does result in undefined behavior.

Undefined behavior includes the case where everything works without problems. But it is undefined, so you can never be sure. A C++ program where objects are destroyed multiple times is ill-formed, just because the C++ standard says so. Arguments like "it may be safe depending on the destructor code" are completely irrelevant. Compiler and standard libraries are allowed to use code basing on the assumption that objects are only destroyed once, so you'll certainly run in to problems sooner or later.

[Andrei15193]

To add more to your phrase, it may result in undefined behavior

No, it does result in undefined behavior.

Undefined behavior includes the case where everything works without problems. But it is undefined, so you can never be sure. A C++ program where objects are destroyed multiple times is ill-formed, just because the C++ standard says so. Arguments like "it may be safe depending on the destructor code" are completely irrelevant. Compiler and standard libraries are allowed to use code basing on the assumption that objects are only destroyed once, so you'll certainly run in to problems sooner or later.

If it's so wrong to call destructors explicitly then the standard should never allow one to use such methods. Checks for calling destructors explicitly can be done at compile time so there are no run time costs.

Destructors are methods that are called implicitly when an object is destroyed. That mean that when an object is destroyed, when it's memory space is about the be considered "free" it makes a call to the destructor and "frees" the memory to be used by something else. Therefore a destructor that is empty, has no dynamic allocation relations or references to other objects not contained with the current object will result in defined behavior (with the condition that base classes have their destructors in the same meaner). It's just a method! Calling it multiple times is like calling any other method multiple times. It's benefit comes from the implicit call upon destruction.

You can make a method called destroy() and put your destructor code there and never use the ~ClassName() destructor. Every time, before you destroy an object, just make the call to destroy(). It has the same result, it's just a method. Yea, it is an unorthodox way for doing things. Yea, it is not a long term solution.

The standard gives you implicitly called destructors so you don't have to track objects and when they get destroyed to call a specific method.


I don't say "Yea go call destructors explicitly! IT'S AWESOME TO DO SO!". All I'm saying is that in some situations you can call destructors and in others you can't. It fixed the "has stopped working" error for me, I'm looking for a better solution however I haven't came across any, yet.

[eXpl0it3r]

Well C++ was never a language that disallowed things just because it's bad to do so, so I guess under some circumstance you can call the destructor explicitly, but you won't find any author of any good C++ book that would advice you to do so, thus it's quite useless to argue about it.
It's just a matter of code design and in that context I would disallow it in any case. It's a bit the same argument, why I'd disallow manual delete (and new) calls, because it can introduce memory leaks and there are better ways to do it.
Say you create a class and call the destructor on your own, now someone else takes your code and puts a delete into the destructor, now you have undefined behaviour and sure you can say it's the other guys fault, but if you hadn't called the destructor on your own, such a problem could've never occured in the first place.

You should never do things where you have to say to yourself: "I know what I'm doing, so the code is okay."

I don't say "Yea go call destructors explicitly! IT'S AWESOME TO DO SO!". All I'm saying is that in some situations you can call destructors and in others you can't. It fixed the "has stopped working" error for me, I'm looking for a better solution however I haven't came across any, yet.

Well it's no secret that SFML has issues with its global OpenGL context which can lead to crashes at clean up.
The code example you've provided above work perfectly on my system, so it's kind of hard to figure out where the error originates from. You could make use of your debugger and find out on your own.

Is your graphics card driver uptodate?

Btw: The use of pragma comment is also a very bad code design, because project settings do not have anything to do with coding, so they should be set in the project settings!

[Andrei15193]

Say you create a class and call the destructor on your own, now someone else takes your code and puts a delete into the destructor, now you have undefined behaviour and sure you can say it's the other guys fault, but if you hadn't called the destructor on your own, such a problem could've never occured in the first place.

There goes that:

Therefore a destructor that is empty, has no dynamic allocation relations or references to other objects not contained with the current object will result in defined behavior (with the condition that base classes have their destructors in the same meaner).

I'm not saying to use destructors as any other method. I don't really call destructors to call myself interesting or whatever, I just thought "what if I do that" I did that and it fixed the issue for now. If I'm not pleased with this solution I'll find a different one.

You should never do things where you have to say to yourself: "I know what I'm doing, so the code is okay"

I should or I shouldn't. I think I know what things to do, thanks anyway.

Yes I am running the latest graphics drivers (what does that have to do with anything in the first place anyway?)

The use of pragma comment is a bad code design? Just a sec, LOL, back. Your subjectiveness is cool, keep it up. Ok, so loading a library with #pragma comment is a bad idea and not code related at all. In fact I'm loading the library just to have it loaded, I don't use it to code with it, why would I do that?

Another scenario, you download some sources and then you read "you need to go to projects settings and do this crap" while you could of just downloaded one archive, with 2 folders and a readme. The readme says "folder a goes there", "folder b has sources", "open whatEverNameYouLike.sln from folder b with visual studio 2005 or later". Less time wasted easier to deal with.

Also when looking over a source code, wouldn't you like to know, from the source code you're reading, what external dependencies does the compilation and final application require? I know I would.

Because you don't agree with some techniques it doesn't mean they are bad. I personally don't like the break thingy, but I don't say "if you're using break in your code, you're writing bad code". Each codes how he or she wants. It's code efficiency and making it work is what matters in the end. Like it or not we're not in an ideal world to have ideal solutions that everyone likes, we can tend to be ideal, but we will never be ideal. I am not saying I call destructors with the programming technique that I currently apply.

[Nexus]

If it's so wrong to call destructors explicitly then the standard should never allow one to use such methods.

First, this isn't a valid conclusion in general. Just because things work, they needn't be correct.

Second, there is a rare use case for explicit destructor calls: When you split the delete operator into destruction and raw deallocation. This works as counterpart to raw allocation and placement new.

T* ptr = static_cast<T*>( operator new(sizeof(T)) );
new (ptr) T(...);

ptr->~T();
operator delete(ptr);

Therefore a destructor that is [...] will result in defined behavior [...]. It's just a method! Calling it multiple times is like calling any other method multiple times.

No. Again, you make the mistake to assume a specific implementation and to generalize it in order to prove your claim.

As already stated, the C++ standard says the behavior is undefined. This allows compilers and libraries to exploit that fact for optimizations or debug assistance. I searched for the corresponding paragraph:

Once a destructor is invoked for an object, the object no longer exists; the behavior is undefined if the destructor is invoked for an object whose lifetime has ended (3.8). [Example: if the destructor for an automatic object is explicitly invoked, and the block is subsequently left in a manner that would ordinarily invoke implicit destruction of the object, the behavior is undefined. ]

[eXpl0it3r]

The use of pragma comment is a bad code design? Just a sec, LOL, back. Your subjectiveness is cool, keep it up. Ok, so loading a library with #pragma comment is a bad idea and not code related at all. In fact I'm loading the library just to have it loaded, I don't use it to code with it, why would I do that?

As for the pragma comment you miss understood me. Of course linking has todo with coding, but the codes task is not to tell the linker which library to link, this is again my opinion and you might keep yours, but you'll also have to accept that I don't like your way and will call it bad no matter how you 'argue' in favor for it.
The one thing you can't argue around is that using pragma comment will make your code automatically none portable. Since I'm trying to stay portable as much as possible this is a no go for me. But again you can keep it and stick to VS only.

Another scenario, you download some sources and then you read "you need to go to projects settings and do this crap" while you could of just downloaded one archive, with 2 folders and a readme. The readme says "folder a goes there", "folder b has sources", "open whatEverNameYouLike.sln from folder b with visual studio 2005 or later". Less time wasted easier to deal with.

I guess you're talking more about build scripts, etc. Well I might be easier for the end user to have the wanted project files directly shipped with the application it's also extremly unpractical for the developer. Because he'd have to generate all those project files newly evertime he changes something and if he wants to offer as much as felexibility he'd have to provide a few dozens of them (e.g. for VS10 debug dynamic, VS10 release dynamic, VS10 MinSize dynamic, VS10 debug static, VS10 release static, VS10 MinSize static, Unix Makefile debug dynamic, Unix Makefile release dynamic, Unix Makefile debug static, ....)
And if the dependencies grow the more complicated the generating of this goes and thus it's waaay easier to provide a a build script, which lets the user choose what options he wants and then generated the specific project file and thus it's easier to maintain by the developers.
Again there are always people that fight against such things, but then again most of the devs will tell you that build scripts are better.

Also when looking over a source code, wouldn't you like to know, from the source code you're reading, what external dependencies does the compilation and final application require? I know I would.

When reading sourcecode one knows from the header inclusions which library was used, thus there's no need to specifiy this with pragma comment.

Because you don't agree with some techniques it doesn't mean they are bad. I personally don't like the break thingy, but I don't say "if you're using break in your code, you're writing bad code". Each codes how he or she wants. It's code efficiency and making it work is what matters in the end. Like it or not we're not in an ideal world to have ideal solutions that everyone likes, we can tend to be ideal, but we will never be ideal. I am not saying I call destructors with the programming technique that I currently apply.

Hmmm yes, I was always talking about code design which is to some point always a bit personal, but from that perspective I can very well declare something as bad. But since it's a perspective I share with many others it's not just my opinion but the opinion of a collective, which to some parts includes the 'inventor' of C++ itself (Bjarne Stroustup) and other well-known programmers (Herb Sutter...).

Yes I am running the latest graphics drivers (what does that have to do with anything in the first place anyway?)

Everything and nothing. If the graphics driver are not uptodate you can experience a lot of strange stuff, so it's always good to check this point.

[Andrei15193]

I'll resume my phrases to not exceed in off-topic.

@Nexus: okay so calling destructors lead to undefined behavior (says the 2003 standard) even though it may work.

@eXpl0it3r: I accept that people disagree with me, but opinions should remain opinions and not made into rules or judge upon them if one thing is bad or not (no matter how many think the same way). One knows from the source code what library was used but does not know what external dependencies to the application (.dll files for example) are required. #pragma comment can be encapsulated between #ifdef/#ifndef... #endif statements and use of macros can handle build scripts to some point.


I've tried a few things and came to the conclusion that not declaring any instances of sf::Text doesn't make the application crash on exit for me. I have no idea how sf::Text manages that.

[eXpl0it3r]

@Nexus: okay so calling destructors lead to undefined behavior (says the 2003 standard) even though it may work.

This sounds like you doubt that the standard is legit or so...
'Undefined' doesn't mean that the application has to crash or anything, it just means that the standard doesn't say what should happen and thus, anything can happen, from nothing to atomic war.
It's not only in the 2003 standard but also in the new C++11 standard:

Once a destructor is invoked for an object, the object no longer exists; the behavior is undefined if the destructor is invoked for an object whose lifetime has ended

@eXpl0it3r: I accept that people disagree with me, but opinions should remain opinions and not made into rules or judge upon them if one thing is bad or not (no matter how many think the same way). One knows from the source code what library was used but does not know what external dependencies to the application (.dll files for example) are required. #pragma comment can be encapsulated between #ifdef/#ifndef... #endif statements and use of macros can handle build scripts to some point.

That's your opinion. So are you judging me because I have a diffrent one?
The whole opinion discussion is quite useless, because there's always disagreements which opinion is 'the right' one (even that is again an opinion one can disagree with), so quickly gets into some sort of loop which doesn't really have an end.
The ideas of discussion are then actually to get other people to agree with ones opinions and if both parties don't want to give up their opinion, there's no need for further discussion. But if you want I still can go on and tell you where I've a diffrent opinion and why.

I've tried a few things and came to the conclusion that not declaring any instances of sf::Text doesn't make the application crash on exit for me. I have no idea how sf::Text manages that.

Oh well I've overlooked the 'obvious' part, I think it's once again connected to the ATI/default font bug. There are three posibilities:

• The best solution would be to compile the latest SFML version on your own, because the latest version doesn't provide a default font anymore and you'd be forced to load one on your own.
• Otherwise you can stick to the SFML 2rc but you will have to load your own font directly through the constructor of the sf::Text.
• And if you want to keep the default font, then you'll have to link statically.