Threaded server/display mode (for real this time) #174

publixsubfan · 2021-06-05T01:02:02Z

Summary

Uses threads instead of fork() to open new windows for an incoming session. Each thread is associated with a single window and problem state, and handles scene operations and rendering to OpenGL.
- As a stopgap pending Refactoring to remove global state #170, much of the global scene state has been made thread_local, and is set by a session worker thread upon a call to GLVisInitVis()/GLVisStartVis().
- This resolves the issue seen in __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() #141, where Cocoa doesn't play nice with fork().
- Also potentially provides a solution for How to close all GLVis windows? #128, since closing the server process should close all opened windows.
Adds an SdlMainThread class to deal with operations that need to be handled on the main thread, such as event handling and window operations. This class is instantiated once for a program, and essentially operates as a "server" which accepts commands from session threads.
- This is required for Windows and Mac OS X; both systems only expect a single thread to handle events. Worse, Cocoa expects this to be the main program thread only.
Splits server code into a new function, GLVisServer(), which runs on its own thread.
Enables preliminary support for building natively on Windows using CMake.

TODO

Get glvis-js working with these changes
Testing for issues

…le thread

…rver mode

- Don't build logo when we're on Windows - Use imported SDL2 targets when possible - needed for correct DLL handling - Don't use M_PI constant

publixsubfan · 2021-06-29T21:04:17Z

Never mind, it was a missing define in the makefile. It should work now.

pazner · 2021-06-29T21:18:08Z

Never mind, it was a missing define in the makefile. It should work now.

Works now, thanks!

camierjs · 2021-07-06T16:51:07Z

The overall behavior looks really good - thanks @kanye-quest !

I have just encountered the following exit/Ctrl-C difference: after hitting the q key in a window, I'm not able to Ctrl-C the server in the terminal anymore.

However, if I click on the dock icon, then everything shuts down normally.

tzanio · 2021-07-06T16:58:22Z

I still see issues with the lor-transfer (cf. #174 (comment)). While the windows are arranged correctly now:

some of them have scaled viewport which can be seen in the caption (letters have scaled width) and when you zoom in:

Can you reproduce?

tzanio · 2021-07-06T17:05:55Z

I have just encountered the following exit/Ctrl-C difference: after hitting the q key in a window, I'm not able to Ctrl-C the server in the terminal anymore.

I will also like to be able to close the server with Ctrl-C in the terminal if possible

tzanio · 2021-07-06T17:21:46Z

Not sure if this is a change in this PR, but the handling of ( and ) is incorrect -- they should only resize the window without affecting the centering of the image. This affects for example the minimal-surface miniapp in miniapps/meshing

tzanio

Please document in CHANGELOG

lib/sdl_main.cpp

share/windows.manifest

share/CMakeLists.txt

makefile

…ads.cpp

…event)

tzanio · 2021-07-07T20:23:31Z

I can confirm that e380892 fixed #174 (comment) and #174 (comment) for me.

tzanio

This is a pretty major overhaul of the GLVis server internals, that I'm pretty sure would not have happened without your help @kanye-quest -- Thank you! 🎉 🎉 🎉

All my testing has been good for a while now, so I am approving with the expectation that continued use may uncover some issues in corner cases. 👍

@pazner and @camierjs -- Thanks for reviewing!

pazner

Very nice quality-of-life improvements! Makes GLVis behave a lot more like a standard Mac app, and works much more reliably than before. Dock icon and Ctrl-C are both working properly for me.

I haven't looked at the code in the PR, but from a user's perspective it's a big improvement!

tomstitt · 2021-07-07T22:00:34Z

lib/gl/types.cpp

   float theta = atan2 (vy, vx);

+#ifndef M_PI
+   const double M_PI = glm::pi<double>();


(minor) can this be constexpr?

tomstitt · 2021-07-07T22:08:03Z

lib/sdl_x11.cpp

+   }
+   if (XISelectEvents_(disp, wnd, &event_mask, 1) != Success)
+   {
+      cerr << "Failed to disable XInput on the current window!" << endl;


should these error blocks also exit(EXIT_FAILURE) ? Or is XInput being enabled pretty minor? edit: i noticed glvis just has a lot of cases where errors are reported but they aren't necessarily fatal, it probably keeps things running in more cases which is good

Yeah, I think if we can't disable XInput extension events, it just means our event loop gets woken up more.

tomstitt

This is awesome, thanks @kanye-quest !

camierjs · 2021-07-07T22:34:28Z

Looks good to me too with Laghos.
Thank you again @kanye-quest for this PR!

publixsubfan added 13 commits May 28, 2021 13:34

Make a bunch of variables thread_local

4254f18

Move glvis_command multiplexing to IdleFunc

4bd9748

Add an SdlWindow::MainThread class to handle SDL operations on a sing…

95e97f8

…le thread

Get things to display

1c324e4

Consolidate SDL main thread commands from worker threads

7f15708

Fixing window exits and resizes; miscellaneous cleanup

b135df2

Make SdlWindow::Handle a move-only type, hold it directly as a value

d42bf14

Initial commit of a thread-based server mode

b762139

Re-enable -mac command for server mode, use Session object for non-se…

ed10443

…rver mode

Fixes for X11, split syswm code into new file to avoid macro conflicts

8dfef72

Fixing build issue on gcc 4.9.3

12a24f7

Fix save file opening; remove global input_streams variable

d6c75f3

Show prompt for terminating server thread

4c5cdb7

publixsubfan added Windows Mac labels Jun 5, 2021

publixsubfan requested review from tomstitt, tzanio and v-dobrev June 5, 2021 01:02

publixsubfan added this to the glvis-4.1 milestone Jun 5, 2021

publixsubfan mentioned this pull request Jun 5, 2021

[WIP] Multithreaded refactoring #149

Closed

4 tasks

publixsubfan and others added 10 commits June 7, 2021 11:03

Use vector<unique_ptr<T>> to manage input stream ownership

238bfc5

Remove last of the POSIX dependencies

b3df197

Fix case where platform is nullptr

e43155c

Only prompt for exit in server mode

8e9a8d4

Final build fixups for Windows builds

1c7eebc

- Don't build logo when we're on Windows - Use imported SDL2 targets when possible - needed for correct DLL handling - Don't use M_PI constant

Add HiDPI support for Windows

edc7424

Add #ifdef guards for our custom M_PI

fc3ad6d

Fix build on gcc 4.9.3 + clean up warnings

2fb40e9

Fix js build by enabling single-threaded mode

2c286e7

Split SDL main thread class into its own file

5888635

tzanio added the ready-for-review label Jul 6, 2021

tzanio reviewed Jul 6, 2021

View reviewed changes

lib/sdl_main.cpp Outdated Show resolved Hide resolved

share/windows.manifest Show resolved Hide resolved

share/CMakeLists.txt Show resolved Hide resolved

makefile Outdated Show resolved Hide resolved

publixsubfan added 3 commits July 7, 2021 10:36

Call [NSOpenGLContext update] after window geometry changes from thre…

37bdedb

…ads.cpp

Ensure context is updated when updateLayer is called as well (expose …

e380892

…event)

Fix qutting glvis server with Ctrl-C

8b27bd8

publixsubfan added 4 commits July 7, 2021 13:24

Fix version check for workaround

57fbf46

Update CHANGELOG

2b84347

Only print SDL version in debug mode

2e0e0a7

Wrap makefile to 80 cols

92f7f4c

tzanio approved these changes Jul 7, 2021

View reviewed changes

pazner approved these changes Jul 7, 2021

View reviewed changes

minor js build fix

f002d95

tomstitt reviewed Jul 7, 2021

View reviewed changes

tomstitt approved these changes Jul 7, 2021

View reviewed changes

camierjs approved these changes Jul 7, 2021

View reviewed changes

Better workaround for M_PI on windows

b8e0861

tzanio merged commit b513773 into master Jul 9, 2021

tzanio deleted the threaded-server-dev branch July 9, 2021 20:20

tomstitt mentioned this pull request Aug 9, 2021

The web version at glvis.org/live does not detect hi-DPI display #182

Closed

pazner mentioned this pull request Aug 10, 2021

Create Mac application bundle #184

Merged

najlkin mentioned this pull request Jul 24, 2024

CI misc improvements #296

Merged

Threaded server/display mode (for real this time) #174

Threaded server/display mode (for real this time) #174

Uh oh!

Conversation

publixsubfan commented Jun 5, 2021 • edited by tzanio Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

TODO

Uh oh!

publixsubfan commented Jun 29, 2021

Uh oh!

pazner commented Jun 29, 2021

Uh oh!

camierjs commented Jul 6, 2021

Uh oh!

tzanio commented Jul 6, 2021

Uh oh!

tzanio commented Jul 6, 2021

Uh oh!

tzanio commented Jul 6, 2021

Uh oh!

tzanio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tzanio commented Jul 7, 2021

Uh oh!

tzanio left a comment

Choose a reason for hiding this comment

Uh oh!

pazner left a comment

Choose a reason for hiding this comment

Uh oh!

tomstitt Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

tomstitt Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

publixsubfan Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

tomstitt left a comment

Choose a reason for hiding this comment

Uh oh!

camierjs commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

publixsubfan commented Jun 5, 2021 •

edited by tzanio

Loading

tomstitt Jul 7, 2021 •

edited

Loading

camierjs commented Jul 7, 2021 •

edited

Loading