Separate Window.Display from event polling

[l0calh05t]

Would it be possible to split Window.Display from event polling (and expose polling in the public interface)? This could be done via an auto-poll flag (which should be true by default, as not to break the api).

Furthermore it would be *really* nice if I could unbind the current rendercontext, to rebind it again in a different thread.

The first point would separate rendering&input more, and it would become possible to pause rendering (including unnecessary buffer clearing & swapping) when the window is minimized, without automatically also stopping input from being polled (basically resulting in a crash/unresponsive window)

The second point (combined with the first) would make it possible to separate Rendering into a different Thread which make far more sense than having those in the same thread in a multi threading environment (which is something we honestly can't ignore anymore), because rendering has nothing to do with input handling and this way the events could be polled directly in the thread that actually uses the events (which otherwise would have to be sent to the other thread first, possibly requiring extra synchronization).

I really hope you make this possible, as I really like this library, but also need these features.

For a small win32 example of what I want to do, see below:

Code: [Select]


#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <GL/gl.h>

LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);
void EnableOpenGL(HDC hDC, HGLRC hRC);
void DisableOpenGL(HDC hDC, HGLRC hRC);

volatile bool quit = false;
volatile bool paused = false;

struct GlThreadData
{
HDC hDC;
HGLRC hRC;
};

DWORD WINAPI GlThread(LPVOID arg1)
{
GlThreadData d = *reinterpret_cast<GlThreadData*>(arg1);
HDC hDC = d.hDC;
HGLRC hRC = d.hRC;
float theta = 0.0f;

EnableOpenGL( d.hDC, d.hRC );

glClearColor( 0.0f, 0.0f, 0.0f, 0.0f );

while(!quit)
{
glClear( GL_COLOR_BUFFER_BIT );

glPushMatrix();
glScalef(0.9f,0.9f,0.9f);
glRotatef( theta, 0.0f, 0.0f, 1.0f );
glBegin( GL_TRIANGLES );
glColor3f( 1.0f, 0.0f, 0.0f ); glVertex2f( 0.0f, 1.0f );
glColor3f( 0.0f, 1.0f, 0.0f ); glVertex2f( 0.87f, -0.5f );
glColor3f( 0.0f, 0.0f, 1.0f ); glVertex2f( -0.87f, -0.5f );
glEnd();
glPopMatrix();

SwapBuffers( d.hDC );

if(!paused)
theta += 1.0f;
}

DisableOpenGL( d.hDC, d.hRC );

return 0;
}

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance,
  LPSTR lpCmdLine, int iCmdShow)
{
WNDCLASS wc;
HWND hWnd;
MSG msg;

wc.style = CS_OWNDC;
wc.lpfnWndProc = WndProc;
wc.cbClsExtra = 0;
wc.cbWndExtra = 0;
wc.hInstance = hInstance;
wc.hIcon = LoadIcon( NULL, IDI_APPLICATION );
wc.hCursor = LoadCursor( NULL, IDC_ARROW );
wc.hbrBackground = (HBRUSH)GetStockObject( BLACK_BRUSH );
wc.lpszMenuName = NULL;
wc.lpszClassName = L"MsgPumpThread";
RegisterClass(&wc);

hWnd = CreateWindow(
L"MsgPumpThread", L"Threaded OpenGL / Windows Message Pump",
WS_CAPTION | WS_POPUPWINDOW | WS_VISIBLE,
0, 0, 300, 300,
NULL, NULL, hInstance, NULL );

PIXELFORMATDESCRIPTOR pfd;
int format;

HDC hDC = GetDC( hWnd );

ZeroMemory( &pfd, sizeof( pfd ) );
pfd.nSize = sizeof( pfd );
pfd.nVersion = 1;
pfd.dwFlags = PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL | PFD_DOUBLEBUFFER;
pfd.iPixelType = PFD_TYPE_RGBA;
pfd.cColorBits = 24;
pfd.cAlphaBits = 8;
pfd.cDepthBits = 24;
pfd.cStencilBits = 8;
pfd.iLayerType = PFD_MAIN_PLANE;
format = ChoosePixelFormat( hDC, &pfd );
SetPixelFormat( hDC, format, &pfd );

HGLRC hRC = wglCreateContext( hDC );

GlThreadData gtd = { hDC, hRC };

HANDLE hGlThread;
hGlThread=CreateThread(0,0,&GlThread,&gtd,0,0);

while(!quit)
{
if(!GetMessage(&msg, NULL, 0, 0))
{
quit = true;
}
else
{
TranslateMessage(&msg);
DispatchMessage(&msg);
}
}

WaitForSingleObject(hGlThread,INFINITE);

wglDeleteContext( hRC );
ReleaseDC( hWnd, hDC );

DestroyWindow( hWnd );

return msg.wParam;

}

LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
{

switch (message)
{

case WM_CREATE:
return 0;

case WM_CLOSE:
PostQuitMessage( 0 );
return 0;

case WM_DESTROY:
return 0;

case WM_KEYDOWN:
switch ( wParam )
{
case VK_SPACE:
paused = !paused;
return 0;

case VK_ESCAPE:
PostQuitMessage(0);
return 0;

}
return 0;

default:
return DefWindowProc( hWnd, message, wParam, lParam );
}

}

void EnableOpenGL(HDC hDC, HGLRC hRC)
{
wglMakeCurrent( hDC, hRC );
}

void DisableOpenGL(HDC hDC, HGLRC hRC)
{
wglMakeCurrent( NULL, NULL );
}


[T.T.H.]

Valid point, nevertheless some thoughts from me:

In case you have a GUI, mouse event handling would be directly depending on the graphics and making two threads (events, graphics) to access the GUI data in a thread safe way (mutexes) might actually slow down your two threads again.

I'm not sure about but it might be that OpenGL must handled in the main thread of the process because otherwise it might produce very "wicked" results on some graphic card drivers.

[l0calh05t]

Valid point, nevertheless some thoughts from me:

In case you have a GUI, mouse event handling would be directly depending on the graphics and making two threads (events, graphics) to access the GUI data in a thread safe way (mutexes) might actually slow down your two threads again.

I'm not sure about but it might be that OpenGL must handled in the main thread of the process because otherwise it might produce very "wicked" results on some graphic card drivers.

Mouse event handling does *not* depend on graphics, even mouse cursor display only depends on graphics if you use a sprite as cursor, the standard windows cursor (and i believe also X11 and OSX) is independent from rendering. You can try this with the small example i posted by having the window cleared 10000 times every frame, which will slow down rendering to ~1fps, but the cursor will still move correctly.

And OpenGL does not have to be handled in the main thread (just try the example), what you do have to do is make the context current in the rendering thread (and only there, so if it was current in another thread before, you'll have to call wglMakeCurrent( NULL, NULL ) (for Win32) in the other thread first)

EDIT:
And why would event handling and graphics both have to access gui data? Only the gui manager needs to access gui data, the event handler only has to tell the manager what happened (if necessary) and this could be done via a lockless queue (basically forwarding preprocessed events to all listeners of a certain event type, so the renderer for example only gets resize events)

[Laurent]

That's interesting.

I guess this kind of feature won't be used by a lot of people, but you're right, that would make things much more flexible.

I'll try to integrate this to the sf::Window interface, I'll keep you informed

[l0calh05t]

That's interesting.

I guess this kind of feature won't be used by a lot of people, but you're right, that would make things much more flexible.

Yeah, that's why I would suggest to provide a flag if the events are automatically polled (and that it should be set by default). That way, those who don't use the added flexibility don't have to care.

I'll try to integrate this to the sf::Window interface, I'll keep you informed

Thank you very much, I hope this feature will be in SFML in the near future, because I'd really like to continue using it.

[Laurent]

I've just finished the modifications.

The event handling is now automatically independant from Display() (you don't have to do anything).

The window's context can be unbound with SetActive(false) (SetActive(true) replaces SetCurrent).

You can get the modifications with SVN.

[l0calh05t]

I've just finished the modifications.

The event handling is now automatically independant from Display() (you don't have to do anything).

The window's context can be unbound with SetActive(false) (SetActive(true) replaces SetCurrent).

You can get the modifications with SVN.

Wow, that was fast! I checked out the newest revision, and it seems to work as intended, but I am unsure if one spot in the code is entirely safe on all platforms (and technically it's not thread safe either), I am talking about these lines in sf::Window::Display():

Code: [Select]


    // Make sure events have been processed
    if (myWindow && !myEventsProcessed)
    {
        myWindow->DoEvents();
    }
    myEventsProcessed = false;


On windows myWindow->DoEvents(); will do nothing if called from the wrong thread, dunno how it is with other platforms, you should probably check if it is the same.
Furthermore, setting myEventsProcessed to false at this spot, is technically not threadsafe, as you might be reading/writing from two threads. (Rendering thread & input thread)

But in any case, it does work (at least on Win32), so: Merci beaucoup :-)

[Laurent]

On windows myWindow->DoEvents(); will do nothing if called from the wrong thread, dunno how it is with other platforms, you should probably check if it is the same.

Yep, that's what I was planning to do. I don't know if it has the same behavior, but I can't see any reason it would do something bad.

Furthermore, setting myEventsProcessed to false at this spot, is technically not threadsafe, as you might be reading/writing from two threads. (Rendering thread & input thread)

You're right again, the code is not multithread-safe. That will probably be the next step to make sf::Window robust with this new feature.

Merci beaucoup

De rien

[l0calh05t]

Yep, that's what I was planning to do. I don't know if it has the same behavior, but I can't see any reason it would do something bad.

Assuming it does poll events on some platform, this would cause multiple threads to write to one std::queue, which would be very bad. So, it is worth checking imo.

You're right again, the code is not multithread-safe. That will probably be the next step to make sf::Window robust with this new feature.

Using the architecture I suggested, this would be solved in the following way:

1) User calls Window.setAutoPollEvents(false);
2) Now, in the main thread, events can by polled via Window.pollEvents() (maybe also Window.waitEvents() could be provided)
3) Window.Display doesn't touch events as long as auto poll is still false, so no thread safety issues there.

This is a little more work for people who want to use it the way I want to, but for those who do not use this feature, nothing changes. And it is thread safe. Only disadvantage: If autopoll is (accidentally) set to false, and the user forgets to poll events manually, events will not be handled.

PS: One more tiny little suggestion: Add the event types Iconified and Restored. (That way the game could for example be partially or fully suspended when the window is iconified)

[Laurent]

If the only thing is to remove possible event handling from Display(), it can already be done without changing the code. As you say, the only drawback is that events won't be polled if the user forgets to call GetEvent. But I don't get the point of adding functions such as SetAutoPollEvents or PollEvents.

I also need to check all the event handling code, to make sure everything else is thread-safe.

PS: One more tiny little suggestion: Add the event types Iconified and Restored. (That way the game could for example be partially or fully suspended when the window is iconified)

If all systems support these events, I'll add it

[l0calh05t]

If the only thing is to remove possible event handling from Display(), it can already be done without changing the code. As you say, the only drawback is that events won't be polled if the user forgets to call GetEvent. But I don't get the point of adding functions such as SetAutoPollEvents or PollEvents.

Ok, assume you remove event polling from Display (this is what you are suggesting, correct?), then how does GetEvent know if it already polled recently? (Hmm... maybe if events are polled whenever the Queue is empty... that should work) Assuming this problem is solved, what if someone only uses Asynchronous input? (IsKeyDown etc.) Eh... well, actually those have to do GetEvent anyways as they probably also need to know when the window is closed, so I guess that is no real problem.

Solving the problem with SetAutoPollEvents and PollEvents is merely what I would have done, but if you manage to solve this without making the API more complex  that would obviously be better.

EDIT:
I rewrote GetEvent to

Code: [Select]


bool Window::GetEvent(Event& EventReceived)
{
// Let the window implementation process incoming events
if (myWindow && myEvents.empty())
{
myWindow->DoEvents();
}

    // Pop first event of queue, if not empty
    if (!myEvents.empty())
    {
        EventReceived = myEvents.front();
        myEvents.pop();

        return true;
    }

    return false;
}


and removed

Code: [Select]


    // Make sure events have been processed
    if (myWindow && !myEventsProcessed)
    {
        myWindow->DoEvents();
    }
    myEventsProcessed = false;


from Display, and it seems to work and should be threadsafe (unless GetEvent is called from different threads but that would be pretty nonsensical anyway)

[Laurent]

That looks good

But I must admit I'm still not sure about giving up event handling in Display. That's the only function which I'm sure will be called each frame ; if I remove event handling from it, I can't be 100% sure that events will be polled. Even if, as you said, most people will use GetEvent at least to catch the Closed event.

[l0calh05t]

Understandable, but even if you do keep Event handling in Display, not calling GetEvent will still cause major problems. Why? Well, you'll have a queue to which you are consistently appending events at a fairly high rate, and if they are never removed from the queue this will cause one big memory leak...

[Laurent]

You're perfectly right. That finished to convince me, I just changed the code

[Aszarsha]

It's not actually a memory leak since it's the intended behavior.

Splitting event handling from window display is a great suggestion, I've thought of it a few times, but your model isn't really adequate.

Polling the system through system calls (even if highly optimized) every time GetEvent is called is all but efficient.
I understand backward compatibility issues, but I submitted another suggestion a few weeks ago which aimed at the same system of SFML, so :
There could be two functions, PollEvents() et WaitEvents(), which would poll the event loop of the system. The two would add events to SFML events queue, as it currently is. WaitEvents would act as PollEvents do, unless the first poll is empty, if so, WaitEvents would enter in passive wait from the system.