Window::getSize() incorrectly returns m_size when using handle-based Window

[Theshooter7]

Hi,

Following the guidelines of the Issues page, I'm creating a quick forum thread for this (and I'll likely be posting a couple others over the next couple of days, hence why this text is quoted).

It appears that calling Window::getSize() will always return m_size, even when the window is instantiated as a child of another window (by passing a handle in the constructor, for instance). This bug was mentioned once in some buried thread somewhere, but I cannot seem to find it at the moment.

While a minimal code example is hard to produce (since it requires setting up some kind of platform-specific application like a Windows Forms app, or some external libs like wxWidgets), I cannot necessarily provide one, but the fix is really, really simple.

Just change line 230 (the body of Window::getSize()) in Window.cpp from:

return m_size;

to

return m_impl ? m_impl->getSize() : m_size;

I figure this is simple enough of a change that a pull request would be unnecessary, but I could put one together if really needed (and perhaps get around to making an example as well; although it would require dependencies that would potentially be painful to download).

EDIT: Forgot to mention, but I encountered this bug in SFML 2.2, although supposedly it's been around for a little while now, and it is still present in the current git revision, as well as 2.3.2 (obviously enough). Less relevant, but still following the guidelines, I'm on Windows 10 Home using Visual Studio 2013.

[Nexus]

Do we even need the Window::m_size member variable? It's currently only an optimization. And if we can't rely on it being up-to-date and have to query the PImpl anyway, then it's useless.

The code

return m_impl ? m_impl->getSize() : m_size;

is not particularly meaningful, as there won't be a window when m_impl is a null pointer. Returning the last valid size is not correct.

[zsbzsb]

Do we even need the Window::m_size member variable? It's currently only an optimization. And if we can't rely on it being up-to-date and have to query the PImpl anyway, then it's useless.

There was a thread on this some time ago and the result was that the window size is now cached. IIRC there was some bad performance when we would query the OS every time for the window size.

[Nexus]

Okay. Then I suggest that sf::Window tracks in another member variable whether the window was created through a handle (and is thus out of SFML's control) or not. Based on that, getSize() can then meet a decision: to either return the cached value or to recompute it.

[Laurent]

It appears that calling Window::getSize() will always return m_size, even when the window is instantiated as a child of another window (by passing a handle in the constructor, for instance). This bug [...]

I'm sorry if I missed something, but... I can't find any description of a bug in this discussion. What's wrong? Why is it a "bug" that Window::getSize() returns m_size? The implementation is supposed to make this work correctly, even when sf::Window wraps a native window.

[Theshooter7]

Do we even need the Window::m_size member variable? It's currently only an optimization. And if we can't rely on it being up-to-date and have to query the PImpl anyway, then it's useless.

The size not being up-to-date could be part of the issue at hand here, hence why requesting the size from m_impl seems to be fixing the problem.

I'm sorry if I missed something, but... I can't find any description of a bug in this discussion. What's wrong? Why is it a "bug" that Window::getSize() returns m_size? The implementation is supposed to make this work correctly, even when sf::Window wraps a native window.

I suppose it wouldn't be considered a "bug" per-say, but what exactly would you call it otherwise? An oversight? An error?
In any case, I primarily found this "issue" when adding a RenderWindow to a wxWidgets application. When I obtained the RenderWindow's size to use in an sf::View, it would return incorrect results causing things that I drew to it to be placed incorrectly and/or scaled improperly. I initially found the fix through searching in this thread (although the author mentions SFML 1.6, he was working with 2.0. His intent might be lost as the post isn't the clearest). Once I added this in to my own copy of SFML, everything worked just fine.

I admit that a potential work around might have been to use a RenderTexture, but even then, I would say reporting the wrong size is a problem. :x

[zsbzsb]

@Theshooter7 while I am thinking about it, are you sure you are still polling the SFML window events when embedded in another window? I remember there being a few issues that would happen if SFML's events were not polled.

[Theshooter7]

@Theshooter7 while I am thinking about it, are you sure you are still polling the SFML window events when embedded in another window? I remember there being a few issues that would happen if SFML's events were not polled.

Indeed I do not, as I delegated all events to wxWidgets. Turns out you're actually right! At least, glancing over the SFML source, it looks like m_size gets cached when filterEvent() is called from within pollEvent()/waitEvent().

Thank you for pointing this out. Perhaps a small change to the Documentation could be made to mention this for anyone in the future? Since no tutorial or otherwise mentions at this point that the event queue of a window must be processed in order for m_size to stay correct (and m_size is the only value that seems to be subject to this as well)

[eXpl0it3r]

Thank you for pointing this out. Perhaps a small change to the Documentation could be made to mention this for anyone in the future? Since no tutorial or otherwise mentions at this point that the event queue of a window must be processed in order for m_size to stay correct (and m_size is the only value that seems to be subject to this as well)

It's an internal detail and nothing that should be documented. Maybe the tutorial could point out even more that processing events is a must. Even though we already have this red box in the tutorials:

A mistake that people often make is to forget the event loop, simply because they don't yet care about handling events (they use real-time inputs instead). Without an event loop, the window will become unresponsive. It is important to note that the event loop has two roles: in addition to providing events to the user, it gives the window a chance to process its internal events too, which is required so that it can react to move or resize user actions.

[Theshooter7]

It's an internal detail and nothing that should be documented. Maybe the tutorial could point out even more that processing events is a must. Even though we already have this red box in the tutorials:

A mistake that people often make is to forget the event loop, simply because they don't yet care about handling events (they use real-time inputs instead). Without an event loop, the window will become unresponsive. It is important to note that the event loop has two roles: in addition to providing events to the user, it gives the window a chance to process its internal events too, which is required so that it can react to move or resize user actions.

Fair enough. I looked through the Events Explained tutorial and didn't see it; did not realize it was in Opening and Managing a Window.

[Laurent]

When I obtained the RenderWindow's size to use in an sf::View, it would return incorrect results

After 7 posts of searching an irrelevant solution, we finally have a description of the problem

You see now why I asked this: I know how sf::Window works internally, and I knew it was supposed to report the correct size if you had an event loop ; and I would probably have directed you to the solution if you had described your problem in the first post, instead of focusing on what you thought had to be fixed.

[Theshooter7]

You see now why I asked this: I know how sf::Window works internally, and I knew it was supposed to report the correct size if you had an event loop ; and I would probably have directed you to the solution if you had described your problem in the first post, instead of focusing on what you thought had to be fixed.

You're right and I probably should have made it a bit more clear from the start. Sorry about that. :/