Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Window::getSize() incorrectly returns m_size when using handle-based Window  (Read 6484 times)

0 Members and 1 Guest are viewing this topic.

Theshooter7

  • Newbie
  • *
  • Posts: 21
    • View Profile
Quote from: 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.
« Last Edit: October 06, 2015, 06:53:57 pm by Theshooter7 »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

zsbzsb

  • Hero Member
  • *****
  • Posts: 1409
  • Active Maintainer of CSFML/SFML.NET
    • View Profile
    • My little corner...
    • Email
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.
Motion / MotionNET - Complete video / audio playback for SFML / SFML.NET

NetEXT - An SFML.NET Extension Library based on Thor

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Quote
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.
« Last Edit: October 06, 2015, 10:47:28 pm by Laurent »
Laurent Gomila - SFML developer

Theshooter7

  • Newbie
  • *
  • Posts: 21
    • View Profile
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

  • Hero Member
  • *****
  • Posts: 1409
  • Active Maintainer of CSFML/SFML.NET
    • View Profile
    • My little corner...
    • Email
@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.
Motion / MotionNET - Complete video / audio playback for SFML / SFML.NET

NetEXT - An SFML.NET Extension Library based on Thor

Theshooter7

  • Newbie
  • *
  • Posts: 21
    • View Profile
@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)
« Last Edit: October 07, 2015, 12:38:32 am by Theshooter7 »

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 11034
    • View Profile
    • development blog
    • Email
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:

Quote from: Tutorial
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.
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

Theshooter7

  • Newbie
  • *
  • Posts: 21
    • View Profile
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:

Quote from: Tutorial
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

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Quote
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.
Laurent Gomila - SFML developer

Theshooter7

  • Newbie
  • *
  • Posts: 21
    • View Profile
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. :/

 

anything