New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault with OpenGL on linux if DISPLAY env variable is not set. #508

Closed
Darkz opened this Issue Dec 15, 2013 · 36 comments

Comments

Projects
None yet
8 participants
@Darkz

Darkz commented Dec 15, 2013

The problem is on SFML/src/SFML/Window/Unix/GlxContext.cpp
On line 51.

    // Open a connection with the X server
    m_display = OpenDisplay();

    // Create a dummy window (disabled and hidden)
    int screen = DefaultScreen(m_display);

DefaultScreen(m_display); triggers a segfault because there is no error checking for Display* m_display.

Testing it like this shows the problem.

    // Open a connection with the X server
    m_display = OpenDisplay();

    assert(m_display != NULL);

    // Create a dummy window (disabled and hidden)
    int screen = DefaultScreen(m_display);
SFML/src/SFML/Window/Unix/GlxContext.cpp:51: sf::priv::GlxContext::GlxContext(sf::priv::GlxContext*): Assertion `m_display != __null' failed.
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 15, 2013

Member

In which conditions would the DISPLAY environment variable not be defined?

Member

LaurentGomila commented Dec 15, 2013

In which conditions would the DISPLAY environment variable not be defined?

@Darkz

This comment has been minimized.

Show comment
Hide comment
@Darkz

Darkz Dec 15, 2013

Well it should be correctly defined by X11 at startup, but it could be wrongly set by the user, or incorrectly set by trying to use X11 over SSH ( not the best example as Opengl will most likely fail anyway ).
Still any kind of message would be better than a segfault.

Darkz commented Dec 15, 2013

Well it should be correctly defined by X11 at startup, but it could be wrongly set by the user, or incorrectly set by trying to use X11 over SSH ( not the best example as Opengl will most likely fail anyway ).
Still any kind of message would be better than a segfault.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila
Member

LaurentGomila commented Dec 15, 2013

Yep.

@ghost ghost assigned LaurentGomila Jan 28, 2014

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 23, 2014

Member

SFML's OpenDisplay() if function is called in several places if you look here. For example, WindowImplX11.cpp or InputImpl.cpp have the same problem as GlxContext.cpp. However, in VideoModeImpl.cpp, a null pointer check is performed.

I guess we should cover all those cases? I'm wondering what a good error message would be. Outputting an error message already in OpenDisplay() might not be specific enough, and client code would still need to check for null pointers...

Member

Bromeon commented Feb 23, 2014

SFML's OpenDisplay() if function is called in several places if you look here. For example, WindowImplX11.cpp or InputImpl.cpp have the same problem as GlxContext.cpp. However, in VideoModeImpl.cpp, a null pointer check is performed.

I guess we should cover all those cases? I'm wondering what a good error message would be. Outputting an error message already in OpenDisplay() might not be specific enough, and client code would still need to check for null pointers...

@abodelot

This comment has been minimized.

Show comment
Hide comment
@abodelot

abodelot Apr 8, 2014

Contributor

@Bromeon

I guess we should cover all those cases?

There are another 5 calls to OpenDisplay() hidden in your github search, for a total of 13 calls.

What about checking the returned value of the unique XOpenDisplay() call instead of the SFML wrapper OpenDisplay(), and simply write "Couldn't open X11 display" on the error stream if sharedDisplay was set to NULL ?

If the user messed up with environment variables, we can't do much. Moreover, the call to XOpenDisplay could fail for other reasons than a missing DISPLAY environment variable.

Contributor

abodelot commented Apr 8, 2014

@Bromeon

I guess we should cover all those cases?

There are another 5 calls to OpenDisplay() hidden in your github search, for a total of 13 calls.

What about checking the returned value of the unique XOpenDisplay() call instead of the SFML wrapper OpenDisplay(), and simply write "Couldn't open X11 display" on the error stream if sharedDisplay was set to NULL ?

If the user messed up with environment variables, we can't do much. Moreover, the call to XOpenDisplay could fail for other reasons than a missing DISPLAY environment variable.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 19, 2014

Member

What about checking the returned value of the unique XOpenDisplay() call instead of the SFML wrapper OpenDisplay(), and simply write "Couldn't open X11 display" on the error stream if sharedDisplay was set to NULL ?

The error stream sounds good, but I'm concerned about returning a null pointer that would lead to undefined behavior if it's not checked by the client code (it could even mess up the just written error output, even though unlikely). On the other hand, the SFML APIs for windows and contexts provide no means of communicating failure at creation, so what would be an appropriate reaction?

Member

Bromeon commented Apr 19, 2014

What about checking the returned value of the unique XOpenDisplay() call instead of the SFML wrapper OpenDisplay(), and simply write "Couldn't open X11 display" on the error stream if sharedDisplay was set to NULL ?

The error stream sounds good, but I'm concerned about returning a null pointer that would lead to undefined behavior if it's not checked by the client code (it could even mess up the just written error output, even though unlikely). On the other hand, the SFML APIs for windows and contexts provide no means of communicating failure at creation, so what would be an appropriate reaction?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Apr 23, 2014

Member

A proper reaction would be throwing an exception, IMHO. Since SFML isn't using exceptions, this would probably lead to some bigger changes... ;)

Member

TankOs commented Apr 23, 2014

A proper reaction would be throwing an exception, IMHO. Since SFML isn't using exceptions, this would probably lead to some bigger changes... ;)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 23, 2014

Member

Yes, the use of exceptions is definitely something to consider for SFML 3.

Here we are dealing with a more fundamental problem: sf::Window's constructor and create() functions are supposed to always succeed, therefore there is no error reporting mechanism (be it return codes, object state or exceptions). I think the error message is the best we can do at the moment, so people will at least know why their program crashed.

Member

Bromeon commented Apr 23, 2014

Yes, the use of exceptions is definitely something to consider for SFML 3.

Here we are dealing with a more fundamental problem: sf::Window's constructor and create() functions are supposed to always succeed, therefore there is no error reporting mechanism (be it return codes, object state or exceptions). I think the error message is the best we can do at the moment, so people will at least know why their program crashed.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Apr 24, 2014

Member

I think the error message is the best we can do at the moment, so people will at least know why their program crashed.

Yep, I agree.

In the long run we should think about if calling code that can fail is a good idea in the constructor. To fully eliminate the problem, the constructor must be private and be "replaced" by a builder function (just like sf::Window::create, but static). What do you think? (This is of course SFML 3 babble, since it's breaking the API)

Member

TankOs commented Apr 24, 2014

I think the error message is the best we can do at the moment, so people will at least know why their program crashed.

Yep, I agree.

In the long run we should think about if calling code that can fail is a good idea in the constructor. To fully eliminate the problem, the constructor must be private and be "replaced" by a builder function (just like sf::Window::create, but static). What do you think? (This is of course SFML 3 babble, since it's breaking the API)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 24, 2014

Member

Wouldn't an exception in the constructor be the cleanest way? In my opinion, allowing to abort a construction is one of the advantages of exceptions. It would have to be documented, of course.

Member

Bromeon commented Apr 24, 2014

Wouldn't an exception in the constructor be the cleanest way? In my opinion, allowing to abort a construction is one of the advantages of exceptions. It would have to be documented, of course.

@Bromeon Bromeon assigned Bromeon and unassigned LaurentGomila Apr 24, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 24, 2014

Member

Wouldn't an exception in the constructor be the cleanest way?

It seems fine for many people. But then it all depends on how we want the API to look like, I guess.

Member

mantognini commented Apr 24, 2014

Wouldn't an exception in the constructor be the cleanest way?

It seems fine for many people. But then it all depends on how we want the API to look like, I guess.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

I'm wondering, how do you handle an exception thrown by a constructor? This is not a problem if you handle this kind of exception at a high level, but let's say I have a very simple code and want to process the possible failure directly:

try
{
    sf::Window window(...);
}
catch (...)
{
    std::err << "failed to create window";

    // possibly try other creation settings, or exit because it is a critical error
}

// /!\ problem: using window is not possible since it's already out of scope

Dynamic allocation could be a workaround, but it's ugly. I guess in C++11 you can use std::move as well.

Member

LaurentGomila commented Apr 24, 2014

I'm wondering, how do you handle an exception thrown by a constructor? This is not a problem if you handle this kind of exception at a high level, but let's say I have a very simple code and want to process the possible failure directly:

try
{
    sf::Window window(...);
}
catch (...)
{
    std::err << "failed to create window";

    // possibly try other creation settings, or exit because it is a critical error
}

// /!\ problem: using window is not possible since it's already out of scope

Dynamic allocation could be a workaround, but it's ugly. I guess in C++11 you can use std::move as well.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 24, 2014

Member

Yes, the object falls out of scope when its constructor throws; but this is deliberate, since you have no object that you can use (it's not allowed to do anything meaningful with an object that has not been constructed).

In this case, if you needed a local variable, you could do as follows -- maybe we could then even remove the create() method:

sf::Window window;
try
{
    window = sf::Window(...); // C++11 move
}
catch (...)
{
    std::err << "failed to create window";
}

But this application of try/catch is rather rare in C++, compared to Java for example, where it's the standard approach. Often, you want to clean up, which is done automatically with RAII, and the error is handled at a higher level. It depends if the local code is able to repair.

Member

Bromeon commented Apr 24, 2014

Yes, the object falls out of scope when its constructor throws; but this is deliberate, since you have no object that you can use (it's not allowed to do anything meaningful with an object that has not been constructed).

In this case, if you needed a local variable, you could do as follows -- maybe we could then even remove the create() method:

sf::Window window;
try
{
    window = sf::Window(...); // C++11 move
}
catch (...)
{
    std::err << "failed to create window";
}

But this application of try/catch is rather rare in C++, compared to Java for example, where it's the standard approach. Often, you want to clean up, which is done automatically with RAII, and the error is handled at a higher level. It depends if the local code is able to repair.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 24, 2014

Member

I agree. It might not be suited for the SFML API we have in mind. Maybe it's not appropriate for all classes – but then we have a consistency issue.

But we're completely hijacking this thread! =P

Member

mantognini commented Apr 24, 2014

I agree. It might not be suited for the SFML API we have in mind. Maybe it's not appropriate for all classes – but then we have a consistency issue.

But we're completely hijacking this thread! =P

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Apr 24, 2014

Member

Wouldn't an exception in the constructor be the cleanest way?

I'm alright with that as well -- I've mixed "do not throw in destructors" with "constructors", to be honest. Your proposal looks good.

maybe we could then even remove the create() method

...together with the default constructor. Is there really a need to be able to define an sf::Window object without actually creating the window? Are there real use-cases? (Except being able to do the try/catch from the example above)

Member

TankOs commented Apr 24, 2014

Wouldn't an exception in the constructor be the cleanest way?

I'm alright with that as well -- I've mixed "do not throw in destructors" with "constructors", to be honest. Your proposal looks good.

maybe we could then even remove the create() method

...together with the default constructor. Is there really a need to be able to define an sf::Window object without actually creating the window? Are there real use-cases? (Except being able to do the try/catch from the example above)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 24, 2014

Member

...together with the default constructor. Is there really a need to be able to define an sf::Window object without actually creating the window? Are there real use-cases?

IMO it would force the user to code thing the SFML way. E.g. the following code structure would no more be possible.

class App
{
    sf::Window wnd;
public:
    App(std::string cfgPath)
    : wnd{} // cfg not loaded... no default ctor -> outch!
    {
        auto cfg = loadCfg(cfgPath);
        wnd = sf::Window(cfg.wnd.mode, cfg.wnd.title, cfg.wnd.style);
    }
};

The default ctor should be kept. Otherwise the user would always need to find a workaround in his design.

Member

mantognini commented Apr 24, 2014

...together with the default constructor. Is there really a need to be able to define an sf::Window object without actually creating the window? Are there real use-cases?

IMO it would force the user to code thing the SFML way. E.g. the following code structure would no more be possible.

class App
{
    sf::Window wnd;
public:
    App(std::string cfgPath)
    : wnd{} // cfg not loaded... no default ctor -> outch!
    {
        auto cfg = loadCfg(cfgPath);
        wnd = sf::Window(cfg.wnd.mode, cfg.wnd.title, cfg.wnd.style);
    }
};

The default ctor should be kept. Otherwise the user would always need to find a workaround in his design.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Apr 24, 2014

Member

The default ctor should be kept. Otherwise the user would always need to find a workaround in his design.

I agree that my proposal is less practical than providing a default constructor, but on the other hand I find it weird to have a window object where there's no window..

My understanding of constructing objects is that they should always be in a valid state -- not doing so may lead to weird behavior, and even logic issues ("When I default-construct a window, what do I get then?").

IMO it would force the user to code thing the SFML way

It would only force the user not to construct an invalid object state. C++ provides no other mechanisms than using some kind of pointer and allocation of the object during runtime (in languages like C# and Java it's done exactly the same way as I proposed, the only difference is that everything is a reference, and ergo you can use null as the initial value). I wouldn't say that it's an SFML-way of doing things (just like I wouldn't say that default-constructing a window with an invalid state + being able to create() it later is an SFML way).

Additionally there is a lot of code in the Window class that does check for a valid internal state, and some code even returns questionable results. Examples:

  • Window::getSettings(): Returns "empty" context settings ("What are empty settings?"). A call to this function should technically not be possible on an object that does not have a context, especially because a context is not optional, it's required.
  • Window::getPosition(): Returns (0, 0) if the window hasn't been created yet. It's wrong, because there is no position.
  • Window::getSize(): Same as getPosition().
  • Window:set*(): Nearly all setters do check for a valid state -- calls to these functions should not be possible without a window -- I'd even declare this as an exceptional situation.

Normally when you want to express a "I will create/initialize this object later" in C++, you use a pointer.

What do others think about this?

Member

TankOs commented Apr 24, 2014

The default ctor should be kept. Otherwise the user would always need to find a workaround in his design.

I agree that my proposal is less practical than providing a default constructor, but on the other hand I find it weird to have a window object where there's no window..

My understanding of constructing objects is that they should always be in a valid state -- not doing so may lead to weird behavior, and even logic issues ("When I default-construct a window, what do I get then?").

IMO it would force the user to code thing the SFML way

It would only force the user not to construct an invalid object state. C++ provides no other mechanisms than using some kind of pointer and allocation of the object during runtime (in languages like C# and Java it's done exactly the same way as I proposed, the only difference is that everything is a reference, and ergo you can use null as the initial value). I wouldn't say that it's an SFML-way of doing things (just like I wouldn't say that default-constructing a window with an invalid state + being able to create() it later is an SFML way).

Additionally there is a lot of code in the Window class that does check for a valid internal state, and some code even returns questionable results. Examples:

  • Window::getSettings(): Returns "empty" context settings ("What are empty settings?"). A call to this function should technically not be possible on an object that does not have a context, especially because a context is not optional, it's required.
  • Window::getPosition(): Returns (0, 0) if the window hasn't been created yet. It's wrong, because there is no position.
  • Window::getSize(): Same as getPosition().
  • Window:set*(): Nearly all setters do check for a valid state -- calls to these functions should not be possible without a window -- I'd even declare this as an exceptional situation.

Normally when you want to express a "I will create/initialize this object later" in C++, you use a pointer.

What do others think about this?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 24, 2014

Member

While I agree with you – it would definitely a more logical way of doing things – I have a concern: what about integration with other tools like Qt, Cocoa, Win32, ...? How would they cohabit with SFML? (I'm wondering, I've not actually looked into it.)

Member

mantognini commented Apr 24, 2014

While I agree with you – it would definitely a more logical way of doing things – I have a concern: what about integration with other tools like Qt, Cocoa, Win32, ...? How would they cohabit with SFML? (I'm wondering, I've not actually looked into it.)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 24, 2014

Member

Unless you have to do something special between default constructing a Window and actually calling .create() on it, it wouldn't change anything. As it is now, Window already automatically calls .create() in its non-default constructors, so if you don't want to have to separately call .create() you could do so now already.

Since Window doesn't have any "interesting" data members, default constructing it does nothing else than to initialize them with some values. You really can't do anything useful in between its default construction and calling .create() so if you want to combine them into a single operation it won't be too big of a change, and as such integration with Qt, Cocoa, Win32 etc. really won't change at all. All it does is prevent leaving Window in a "unusable" state, which can prevent coding errors if .create() is forgotten (no idea how often this might happen).

Member

binary1248 commented Apr 24, 2014

Unless you have to do something special between default constructing a Window and actually calling .create() on it, it wouldn't change anything. As it is now, Window already automatically calls .create() in its non-default constructors, so if you don't want to have to separately call .create() you could do so now already.

Since Window doesn't have any "interesting" data members, default constructing it does nothing else than to initialize them with some values. You really can't do anything useful in between its default construction and calling .create() so if you want to combine them into a single operation it won't be too big of a change, and as such integration with Qt, Cocoa, Win32 etc. really won't change at all. All it does is prevent leaving Window in a "unusable" state, which can prevent coding errors if .create() is forgotten (no idea how often this might happen).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

Everyone agrees that a default-constructed sf::Window is useless, and that the best thing to do is to prevent it, but still, how do you solve the use case shown above, which is very common? With a std::unique_ptr<sf::Window>? :/

Even std::thread has a default constructor, and it's not more useful than sf::Window's one.

Member

LaurentGomila commented Apr 24, 2014

Everyone agrees that a default-constructed sf::Window is useless, and that the best thing to do is to prevent it, but still, how do you solve the use case shown above, which is very common? With a std::unique_ptr<sf::Window>? :/

Even std::thread has a default constructor, and it's not more useful than sf::Window's one.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 24, 2014

Member

how do you solve the use case shown above, which is very common?

I see two options but I'm not comfortable with any of them; having a create() function is simpler for the user in those cases.

a) any kind of indirection (smart pointers or custom class that encapsulate a sf::Window). In this specific case, using another constructor could work too (but it's a local solution that won't work everywhere):

class App
{
    sf::Window wnd;
public:
    App(std::string cfgPath)
    : App(loadCfg(cfgPath))
    { }

    App(CFG cfg)
    : wnd{cfg.wnd.mode, cfg.wnd.title, cfg.wnd.style}
    { }
};

b) Having a special NilWindow object (or with any better name!). This is a common practice in other language (e.g. Nil in Scala is a special List that has no element).

class App
{
    sf::Window wnd;
public:
    App(std::string cfgPath)
    : wnd{sf::NilWindow}
    {
        auto cfg = loadCfg(cfgPath);
        wnd = sf::Window(cfg.wnd.mode, cfg.wnd.title, cfg.wnd.style);
    }
};
Member

mantognini commented Apr 24, 2014

how do you solve the use case shown above, which is very common?

I see two options but I'm not comfortable with any of them; having a create() function is simpler for the user in those cases.

a) any kind of indirection (smart pointers or custom class that encapsulate a sf::Window). In this specific case, using another constructor could work too (but it's a local solution that won't work everywhere):

class App
{
    sf::Window wnd;
public:
    App(std::string cfgPath)
    : App(loadCfg(cfgPath))
    { }

    App(CFG cfg)
    : wnd{cfg.wnd.mode, cfg.wnd.title, cfg.wnd.style}
    { }
};

b) Having a special NilWindow object (or with any better name!). This is a common practice in other language (e.g. Nil in Scala is a special List that has no element).

class App
{
    sf::Window wnd;
public:
    App(std::string cfgPath)
    : wnd{sf::NilWindow}
    {
        auto cfg = loadCfg(cfgPath);
        wnd = sf::Window(cfg.wnd.mode, cfg.wnd.title, cfg.wnd.style);
    }
};
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

Having a special NilWindow object

I don't think it is much different from using the default constructor ;)

Member

LaurentGomila commented Apr 24, 2014

Having a special NilWindow object

I don't think it is much different from using the default constructor ;)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 24, 2014

Member

In analogy to std::thread, the only situation where I can imagine an "empty" sf::Window to be useful is if one were to move assign something into it at some later point such as:

sf::Window window;
// Do a lot of stuff here;
window = sf::Window(sf::VideoMode(800, 600), "WindowName");

This is first of all not yet possible because moves are not yet supported in non-copyable classes. And second of all, I can't think of any use cases where this might make sense... multiple windows stored in an STL container and move assigned at a later point?

Member

binary1248 commented Apr 24, 2014

In analogy to std::thread, the only situation where I can imagine an "empty" sf::Window to be useful is if one were to move assign something into it at some later point such as:

sf::Window window;
// Do a lot of stuff here;
window = sf::Window(sf::VideoMode(800, 600), "WindowName");

This is first of all not yet possible because moves are not yet supported in non-copyable classes. And second of all, I can't think of any use cases where this might make sense... multiple windows stored in an STL container and move assigned at a later point?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

This makes sense in this use case, where you know the window settings after it has been constructed.

Member

LaurentGomila commented Apr 24, 2014

This makes sense in this use case, where you know the window settings after it has been constructed.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 24, 2014

Member

Then the question has to be asked: why even construct it earlier if it serves no purpose (i.e. there is no useful information you can get from it)? ;)

Member

binary1248 commented Apr 24, 2014

Then the question has to be asked: why even construct it earlier if it serves no purpose (i.e. there is no useful information you can get from it)? ;)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

Because it's a member of its owner class and you have no choice but having it constructed in the owner's constructor. The only way to delay its construction would be to allocate it dynamically, which is not an ideal solution.

Member

LaurentGomila commented Apr 24, 2014

Because it's a member of its owner class and you have no choice but having it constructed in the owner's constructor. The only way to delay its construction would be to allocate it dynamically, which is not an ideal solution.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 24, 2014

Member

But even in that case, isn't it a better idea to completely construct it when the owner is constructed as well? If it was a private member (without accessors/mutators) then the owner would have to take care of its initialization itself anyway. If the initialization of the sf::Window is to be performed by another class at a later point, shouldn't it be considered as a "shared resource" and stuck into e.g. a shared_ptr? I feel uneasy when an object that is solely owned by a class is used more outside of it than in the class itself, I would categorize this under the "feature envy" category when it comes to refactoring.

Maybe I'm just misunderstanding something. It is hard for me to picture without a use case written in code.

Member

binary1248 commented Apr 24, 2014

But even in that case, isn't it a better idea to completely construct it when the owner is constructed as well? If it was a private member (without accessors/mutators) then the owner would have to take care of its initialization itself anyway. If the initialization of the sf::Window is to be performed by another class at a later point, shouldn't it be considered as a "shared resource" and stuck into e.g. a shared_ptr? I feel uneasy when an object that is solely owned by a class is used more outside of it than in the class itself, I would categorize this under the "feature envy" category when it comes to refactoring.

Maybe I'm just misunderstanding something. It is hard for me to picture without a use case written in code.

@germinolegrand

This comment has been minimized.

Show comment
Hide comment
@germinolegrand

germinolegrand Apr 24, 2014

sf::Window is a resource holding class. As such, the analogy with
std::thread is really good, and it is the standard way of dealing with
your problem in C++11 : a default constructor, a move constructor, some
other constructors that may throw, a way to know if your object holds a
resource or not.

I don't understand why you could even think of sf::Window as a shared
resource ? It's definitely not the SFML job to decide who actually owns
the sf::Window object, and it's quite a non-sense to make it a shared
resource : between what ? between who ? You don't even know, and that's
logic : that's the user's choice, not SFML's. Also : it's bad design in
C++11 to make the user deal with pointers, whether naked or smart (cf. a
lot of Bjarne Stroustrup talks on basic modern C++11 knowledge). If you
have pointers, just encapsulate it, it's none of the user's business.
The user wants to use a Window, give him a Window.

Le 24/04/2014 14:27, binary1248 a écrit :

But even in that case, isn't it a better idea to completely construct
it when the owner is constructed as well? If it was a private member
(without accessors/mutators) then the owner would have to take care of
its initialization itself anyway. If the initialization of the
sf::Window is to be performed by another class at a later point,
shouldn't it be considered as a "shared resource" and stuck into e.g.
a shared_ptr? I feel uneasy when an object that is solely owned by a
class is used more outside of it than in the class itself, I would
categorize this under the "feature envy" category when it comes to
refactoring.

Maybe I'm just misunderstanding something. It is hard for me to
picture without a use case written in code.


Reply to this email directly or view it on GitHub
#508 (comment).

germinolegrand commented Apr 24, 2014

sf::Window is a resource holding class. As such, the analogy with
std::thread is really good, and it is the standard way of dealing with
your problem in C++11 : a default constructor, a move constructor, some
other constructors that may throw, a way to know if your object holds a
resource or not.

I don't understand why you could even think of sf::Window as a shared
resource ? It's definitely not the SFML job to decide who actually owns
the sf::Window object, and it's quite a non-sense to make it a shared
resource : between what ? between who ? You don't even know, and that's
logic : that's the user's choice, not SFML's. Also : it's bad design in
C++11 to make the user deal with pointers, whether naked or smart (cf. a
lot of Bjarne Stroustrup talks on basic modern C++11 knowledge). If you
have pointers, just encapsulate it, it's none of the user's business.
The user wants to use a Window, give him a Window.

Le 24/04/2014 14:27, binary1248 a écrit :

But even in that case, isn't it a better idea to completely construct
it when the owner is constructed as well? If it was a private member
(without accessors/mutators) then the owner would have to take care of
its initialization itself anyway. If the initialization of the
sf::Window is to be performed by another class at a later point,
shouldn't it be considered as a "shared resource" and stuck into e.g.
a shared_ptr? I feel uneasy when an object that is solely owned by a
class is used more outside of it than in the class itself, I would
categorize this under the "feature envy" category when it comes to
refactoring.

Maybe I'm just misunderstanding something. It is hard for me to
picture without a use case written in code.


Reply to this email directly or view it on GitHub
#508 (comment).

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 24, 2014

Member

@binary1248: What if the window is a member of an application class that cannot be initialized immediately? Maybe the developer wants to load resources first, or at least configuration (also to know how to construct the window). Providing no default constructor forces the use of the constructor initializer list, which is good in many cases, but in all others one must use unique_ptrs with the sole intent of delaying initialization.

Don't forget that even if sf::Window's constructor enforced a valid state, this alone wouldn't guarantee the invariant of object validity -- not only because there is close() (which would obviously have to disappear), but also because you can move from a window, and the leftover window is then "empty". This depends on whether such an empty state is considered accessible, or the only allowed operation on it is destruction.

That brings me to the question, are there meaningful scenarios of functions being called on closed windows (apart from possibly checking if it's opened)? Should the user be able to initialize some properties like VSync etc. before opening the window? In that case, create() would then rather act as an open(), without completely resetting the state. I personally don't think this is a good way of doing things, as it introduces more side effects and state one has to be aware of, without a true advantage.

Generally, I agree with TankOs that the practice of functions called in the closed state and returning arbitrary results is not a good one. If we don't technically prevent it, we should at least document the results (as well as possible side effects) as undefined and maybe check in debug mode with assertions. This would discourage users from calling functions on closed windows.

Apparently, no one cares about the off topic anymore. I'll hopefully fix this issue soon and we can close it forever :D

Member

Bromeon commented Apr 24, 2014

@binary1248: What if the window is a member of an application class that cannot be initialized immediately? Maybe the developer wants to load resources first, or at least configuration (also to know how to construct the window). Providing no default constructor forces the use of the constructor initializer list, which is good in many cases, but in all others one must use unique_ptrs with the sole intent of delaying initialization.

Don't forget that even if sf::Window's constructor enforced a valid state, this alone wouldn't guarantee the invariant of object validity -- not only because there is close() (which would obviously have to disappear), but also because you can move from a window, and the leftover window is then "empty". This depends on whether such an empty state is considered accessible, or the only allowed operation on it is destruction.

That brings me to the question, are there meaningful scenarios of functions being called on closed windows (apart from possibly checking if it's opened)? Should the user be able to initialize some properties like VSync etc. before opening the window? In that case, create() would then rather act as an open(), without completely resetting the state. I personally don't think this is a good way of doing things, as it introduces more side effects and state one has to be aware of, without a true advantage.

Generally, I agree with TankOs that the practice of functions called in the closed state and returning arbitrary results is not a good one. If we don't technically prevent it, we should at least document the results (as well as possible side effects) as undefined and maybe check in debug mode with assertions. This would discourage users from calling functions on closed windows.

Apparently, no one cares about the off topic anymore. I'll hopefully fix this issue soon and we can close it forever :D

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 24, 2014

Member

I guess because sf::Window has a .close() method, it has to still have a valid state even after its underlying resource is destroyed... so to maintain symmetry it makes sense to have .create() methods as well...

That and the fact that in order to be a bit more move-friendly by being left in a usable state after being moved from (which is not required) makes me favour leaving .create() in now :).

Member

binary1248 commented Apr 24, 2014

I guess because sf::Window has a .close() method, it has to still have a valid state even after its underlying resource is destroyed... so to maintain symmetry it makes sense to have .create() methods as well...

That and the fact that in order to be a bit more move-friendly by being left in a usable state after being moved from (which is not required) makes me favour leaving .create() in now :).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

That brings me to the question, are there meaningful scenarios of functions being called on closed windows (apart from possibly checking if it's opened)?

There is. An often requested feature is to be able to set some of the window's properties before opening it (generally its position), so that there's no "jump" at startup between whatever-default-position-sfml-chooses and the one you want the window to appear to. Therefore, an idea that I had would be to be able to call the various setters of sf::Window before opening it, in which case they would just be cached, and then when apply them when the window is actually opened.

This is a much simpler and elegant solution than adding more and more arguments to the constructor/create function to handle all the possible initial states of the window.

Member

LaurentGomila commented Apr 24, 2014

That brings me to the question, are there meaningful scenarios of functions being called on closed windows (apart from possibly checking if it's opened)?

There is. An often requested feature is to be able to set some of the window's properties before opening it (generally its position), so that there's no "jump" at startup between whatever-default-position-sfml-chooses and the one you want the window to appear to. Therefore, an idea that I had would be to be able to call the various setters of sf::Window before opening it, in which case they would just be cached, and then when apply them when the window is actually opened.

This is a much simpler and elegant solution than adding more and more arguments to the constructor/create function to handle all the possible initial states of the window.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 24, 2014

Member

That brings me to the question, are there meaningful scenarios of functions being called on closed windows (apart from possibly checking if it's opened)?

Yes, there is one case that everybody has in his code: closing the window after an event and then drawing stuff on it (like here).

Member

mantognini commented Apr 24, 2014

That brings me to the question, are there meaningful scenarios of functions being called on closed windows (apart from possibly checking if it's opened)?

Yes, there is one case that everybody has in his code: closing the window after an event and then drawing stuff on it (like here).

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 24, 2014

Member

This is a much simpler and elegant solution than adding more and more arguments to the constructor/create function to handle all the possible initial states of the window.

It depends... Now some parameters are set before, while some are set at creation. It looks like the ones specified in create() are mandatory for the window creation and cannot be changed later; while mostly true, I'm not sure if this applies to all settings, for example we already discussed whether style could be changed once the window has been created. And as stated, I would then support open() rather than create(), also to be a counterpart to close(). And we would also have to keep all the settings when the window is closed.

Yes, there is one case that everybody has in his code: closing the window after an event and then drawing stuff on it (like here).

That's why I'm always using an infinite loop and return statements. It's more clean in my opinion than to rely on certain functions doing nothing. But yes, it would confuse everybody if draw() led to UB or an assertion in this case -- after all, the official tutorials have advertised that code for half a decade.

Member

Bromeon commented Apr 24, 2014

This is a much simpler and elegant solution than adding more and more arguments to the constructor/create function to handle all the possible initial states of the window.

It depends... Now some parameters are set before, while some are set at creation. It looks like the ones specified in create() are mandatory for the window creation and cannot be changed later; while mostly true, I'm not sure if this applies to all settings, for example we already discussed whether style could be changed once the window has been created. And as stated, I would then support open() rather than create(), also to be a counterpart to close(). And we would also have to keep all the settings when the window is closed.

Yes, there is one case that everybody has in his code: closing the window after an event and then drawing stuff on it (like here).

That's why I'm always using an infinite loop and return statements. It's more clean in my opinion than to rely on certain functions doing nothing. But yes, it would confuse everybody if draw() led to UB or an assertion in this case -- after all, the official tutorials have advertised that code for half a decade.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 24, 2014

Member

I also like open instead of create. And it's also true that there should be a major review of window attributes and how they are handled. Also keep in mind that it goes beyond the few things mentioned here: ideally we should also try to adapt to the mobile world, where things are a lot different when it comes to windowing ;)

So yes, let's stop this endless discussion here and close this issue quickly.

Member

LaurentGomila commented Apr 24, 2014

I also like open instead of create. And it's also true that there should be a major review of window attributes and how they are handled. Also keep in mind that it goes beyond the few things mentioned here: ideally we should also try to adapt to the mobile world, where things are a lot different when it comes to windowing ;)

So yes, let's stop this endless discussion here and close this issue quickly.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Apr 24, 2014

Member

We can simply differentiate between a window in an invalid state and a window with predefined default properties and hidden. The default constructor could therefore use minimum defaults (e.g. desktop resolution etc.) but hide the window initially -- this has to be documented of course. create() would be misleading and should probably named recreate(). close() would be removed entirely (use setVisible() to set the window's visibility).

Regarding the arguments that one wants to load a config before initializing a window etc.: One can simply argue against that by saying that as long as all required info is not present, constructing a window is simply not possible. Like binary1248 said: Where is the point in holding a "dummy" window object? It's the same as using a pointer (delayed initialization), just more dangerous.

@germinolegrand

and it is the standard way of dealing with
your problem in C++11 : a default constructor, a move constructor, some
other constructors that may throw, a way to know if your object holds a
resource or not.

Who declares that it's the standard way of dealing with the problem in C++11? This is a very specific problem, and I guess even Bjarne would agree that leaving objects in an invalid state is not a very good idea. ;) (one of his principles is "make it hard to use your code wrong").

It's definitely not the SFML job to decide who actually owns
the sf::Window object, and it's quite a non-sense to make it a shared
resource : between what ? between who ? You don't even know, and that's
logic : that's the user's choice, not SFML's.

Exactly, it's the user's choice: If he decides to share the window (to render from several places in the code, for example), then he should be able to do so. Nobody wants to remove this possibility. And no one forces the user to use pointers.

Pointers, ergo delayed construction, is only needed when the user's code design requires that. E.g. when someone loads a config in the same class that constructs the window (this can be done differently!). But it's not required technically -- nobody prevents you from using plain objects.

The user wants to use a Window, give him a Window.

A pointer to a window is still a window in the end... However this discussion is not about these kind of things.

Member

TankOs commented Apr 24, 2014

We can simply differentiate between a window in an invalid state and a window with predefined default properties and hidden. The default constructor could therefore use minimum defaults (e.g. desktop resolution etc.) but hide the window initially -- this has to be documented of course. create() would be misleading and should probably named recreate(). close() would be removed entirely (use setVisible() to set the window's visibility).

Regarding the arguments that one wants to load a config before initializing a window etc.: One can simply argue against that by saying that as long as all required info is not present, constructing a window is simply not possible. Like binary1248 said: Where is the point in holding a "dummy" window object? It's the same as using a pointer (delayed initialization), just more dangerous.

@germinolegrand

and it is the standard way of dealing with
your problem in C++11 : a default constructor, a move constructor, some
other constructors that may throw, a way to know if your object holds a
resource or not.

Who declares that it's the standard way of dealing with the problem in C++11? This is a very specific problem, and I guess even Bjarne would agree that leaving objects in an invalid state is not a very good idea. ;) (one of his principles is "make it hard to use your code wrong").

It's definitely not the SFML job to decide who actually owns
the sf::Window object, and it's quite a non-sense to make it a shared
resource : between what ? between who ? You don't even know, and that's
logic : that's the user's choice, not SFML's.

Exactly, it's the user's choice: If he decides to share the window (to render from several places in the code, for example), then he should be able to do so. Nobody wants to remove this possibility. And no one forces the user to use pointers.

Pointers, ergo delayed construction, is only needed when the user's code design requires that. E.g. when someone loads a config in the same class that constructs the window (this can be done differently!). But it's not required technically -- nobody prevents you from using plain objects.

The user wants to use a Window, give him a Window.

A pointer to a window is still a window in the end... However this discussion is not about these kind of things.

Bromeon added a commit that referenced this issue Apr 27, 2014

Bromeon added a commit that referenced this issue May 6, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 6, 2014

Member

Merged in dev branch.

Member

TankOs commented May 6, 2014

Merged in dev branch.

@TankOs TankOs closed this May 6, 2014

@Bromeon Bromeon added the resolved label May 6, 2014

Bromeon added a commit that referenced this issue May 26, 2014

Bromeon added a commit that referenced this issue May 28, 2014

Output error message and abort program when XOpenDisplay() fails
Fixes issue #508.

When the X11 display could not be opened, the application crashed without notice. Now, a meaningful error message is output to std::err() and std::abort() is called, causing immediate program termination.

Bromeon added a commit that referenced this issue May 28, 2014

Output error message and abort program when XOpenDisplay() fails
Fixes issue #508.

When the X11 display could not be opened, the application crashed without notice. Now, a meaningful error message is output to sf::err() and std::abort() is called, causing immediate program termination.

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this issue Jun 13, 2014

Output error message and abort program when XOpenDisplay() fails
Fixes issue SFML#508.

When the X11 display could not be opened, the application crashed without notice. Now, a meaningful error message is output to std::err() and std::abort() is called, causing immediate program termination.

jcowgill added a commit to jcowgill/SFML that referenced this issue Sep 22, 2014

Output error message and abort program when XOpenDisplay() fails
Fixes issue SFML#508.

When the X11 display could not be opened, the application crashed without notice. Now, a meaningful error message is output to std::err() and std::abort() is called, causing immediate program termination.

(cherry picked from commit bc1127d)

Conflicts:
	src/SFML/Window/Linux/Display.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment