Skip to content
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

OpenGL modernization #66

Merged
merged 230 commits into from
Dec 3, 2020
Merged

OpenGL modernization #66

merged 230 commits into from
Dec 3, 2020

Conversation

publixsubfan
Copy link
Contributor

@publixsubfan publixsubfan commented Jul 23, 2018

Modernizes the OpenGL API usage of GLVis. Highlights:

  • Prepare*() functions now load vertices into a gl3::GlDrawable container, which replaces the previously-defined display lists and wrap the vertex buffers to be provided to OpenGL
  • A gl3::GlBuilder object can be created from a gl3::GlDrawable to partially emulate fixed-function OpenGL calls and load the generated vertices into vertex buffers
  • Two OpenGL backends are defined: gl3::FFGLRenderer, for OpenGL implementations lacking shaders; and gl3::CoreGLRenderer, which uses shaders to draw vertex buffers
  • A gl3::MeshRenderer object accepts the GlDrawable objects to be rendered per-frame, and directs buffering operations, draw calls, and state changes to one of the two backend classes
  • To support native windows and modern OpenGL contexts on non-Linux platforms, X11 is replaced with SDL for window and event management, with operations on the application window and the event loop encapsulated by the SdlWindow class.

Miscellaneous improvements:

  • For print-to-PDF functionality, handle the parsing of transform feedback buffers ourselves; allows us to support printing in texture mode and for both OpenGL backends
  • Transparency for texture mode is supported by generating an alpha texture for the mesh values
  • Removed non-texture rendering mode, since both printing and transparency support textured rendering mode now
  • Text is printed to screen with a font atlas texture (hard dependency on FreeType)
  • Support building to a JavaScript library with Emscripten – requires MFEM to be built with Emscripten first
  • Various refactorings
PR Author Editor Reviewers Assignment Approval Merge
#66 @kanye-quest @tzanio @v-dobrev + @jakubcerveny + @jandrej + @camierjs 04/25/20 ⌛due 06/29/20 ⌛due 07/06/20

@v-dobrev
Copy link
Member

One issue that is still outstanding: #66 (comment). @kanye-quest, do you have a specific reason for those changes? Unless I'm missing something, I think we need to revert back to the explicit synchronous window re-drawing.

@publixsubfan
Copy link
Contributor Author

I can't actually remember why I made that change; I'll see if I can change the SendExposeEvent calls back to MyExpose.

@v-dobrev
Copy link
Member

The window content is not dynamically updated while resizing the window. It will be nice to fix that.

@v-dobrev
Copy link
Member

I can't actually remember why I made that change; I'll see if I can change the SendExposeEvent calls back to MyExpose.

Looking at the code of SendExposeEvent, I see that it's behavior is now quite different from what it used to be. Maybe renaming it to something like SingalExpose() or TriggerExpose() will be a better name. It looks like it triggers MyExpose calls directly from the SDL main loop (and between calls to the idle function and the glvis_command queue), so using it instead of MyExpose seems fine. It also triggers SDL_GL_SwapWindow which is probably necessary to actually update the window content?

@publixsubfan
Copy link
Contributor Author

If I understand correctly, the main issue is that when a screenshot is requested via a character key sequence, we only want the commands up to that point to be executed and displayed; but with SendExposeEvent, MyExpose is only called after the entire character key sequence is processed.

I think we can achieve this asynchronously by exiting out of the event loop in SdlWindow early if a screenshot command has been queued. Then the MyExpose command, window swap, and Screenshot will be executed immediately.

@publixsubfan
Copy link
Contributor Author

@v-dobrev, I made some changes to switch from SendExposeEvent() back to MyExpose() for the stuff in threads.cpp and glvis.cpp.

The original reason I changed it to SendExposeEvent() was because the back-to-front buffer swap was only called in the SDL window loop. With the new changes, calling MyExpose() will signal to the SdlWindow that the back buffer needs to be swapped in on the next iteration of the event loop. Screenshot() has been modified to be able to capture either the back or the front buffer: if the back buffer has been updated but not yet swapped in, the image data is pulled from the back buffer; if no swap is pending, the image data is pulled from the front buffer.

@tzanio
Copy link
Member

tzanio commented Dec 1, 2020

Thanks for the hi-res icon, @kanye-quest!

hi-res-glvis-icon

@v-dobrev
Copy link
Member

v-dobrev commented Dec 1, 2020

@v-dobrev, I made some changes to switch from SendExposeEvent() back to MyExpose() for the stuff in threads.cpp and glvis.cpp.

The original reason I changed it to SendExposeEvent() was because the back-to-front buffer swap was only called in the SDL window loop. With the new changes, calling MyExpose() will signal to the SdlWindow that the back buffer needs to be swapped in on the next iteration of the event loop. Screenshot() has been modified to be able to capture either the back or the front buffer: if the back buffer has been updated but not yet swapped in, the image data is pulled from the back buffer; if no swap is pending, the image data is pulled from the front buffer.

Is there a reason not to do the swap inside MyExpose? At that point the back buffer has the full image and the swap should be fast. This will also simplify the logic in a few places.

v-dobrev and others added 4 commits December 2, 2020 12:37
GLVis is idle -- only for X11, for now, with placeholders for
Cocoa on Mac.

When using non-X11 subsystem, use SDL_WaitEvent() in the event loop
instead of generating pauses with SDL_WaitEventTimeout(). In this
case the GLVis communication thread will "wake up" the main thread
by sending it a user-defined SDL event.

Re-enable the screensaver since SDL disables it by default.

When using the X11 subsystem, disable the XInput extension events
since they are generated even outside the GLVis window.
lib/sdl_mac.m Outdated
void GLVis_Cocoa_WaitEvent()
{ @autoreleasepool
{
[NSApp nextEventMatchingMask:NSEventMaskAny
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

@tzanio
Copy link
Member

tzanio commented Dec 3, 2020

@v-dobrev and @kanye-quest -- are we all set for merging today from your perspective?

{
SDL_Event e;
static bool useIdle = false;
if (SDL_PollEvent(&e))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@v-dobrev, I think we need to go back to while (SDL_PollEvent(&e)) here - there's noticeable response lag introduced when moving with mouse on a remote X11 connection to LC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should block today's merge though.

@termi-official
Copy link
Contributor

Hi @v-dobrev ,

with commit fc9276b the broke build for me on OpenSUSE and current MFEM master.

In file included from /home/termi/Repos/glvis/lib/sdl.cpp:15:
In file included from /home/termi/Repos/glvis/lib/visual.hpp:17:
In file included from /home/termi/Repos/glvis/lib/vsdata.hpp:18:
In file included from /home/termi/builds/mfem/mfem.hpp:3:
In file included from /home/termi/Repos/mfem/mfem.hpp:18:
In file included from /home/termi/Repos/mfem/general/device.hpp:15:
In file included from /home/termi/Repos/mfem/general/globals.hpp:19:
In file included from /home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/mpi.h:2693:
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/mpicxx.h:177:3: error: declaration of
      anonymous class must be a definition
  class Status;
  ^
In file included from /home/termi/Repos/glvis/lib/sdl.cpp:15:
In file included from /home/termi/Repos/glvis/lib/visual.hpp:17:
In file included from /home/termi/Repos/glvis/lib/vsdata.hpp:18:
In file included from /home/termi/builds/mfem/mfem.hpp:3:
In file included from /home/termi/Repos/mfem/mfem.hpp:18:
In file included from /home/termi/Repos/mfem/general/device.hpp:15:
In file included from /home/termi/Repos/mfem/general/globals.hpp:19:
In file included from /home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/mpi.h:2693:
In file included from /home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/mpicxx.h:219:
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/status.h:24:1: error: declaration of
      anonymous class must be a definition
class Status {
^
In file included from /home/termi/Repos/glvis/lib/sdl.cpp:15:
In file included from /home/termi/Repos/glvis/lib/visual.hpp:17:
In file included from /home/termi/Repos/glvis/lib/vsdata.hpp:18:
In file included from /home/termi/builds/mfem/mfem.hpp:3:
In file included from /home/termi/Repos/mfem/mfem.hpp:18:
In file included from /home/termi/Repos/mfem/general/device.hpp:15:
In file included from /home/termi/Repos/mfem/general/globals.hpp:19:
In file included from /home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/mpi.h:2693:
In file included from /home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/mpicxx.h:224:
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:166:11: error: expected
      unqualified-id
            MPI::Status& status);
                 ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:166:6: error: expected
      parameter declarator
            MPI::Status& status);
            ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:166:11: error: expected ')'
            MPI::Status& status);
                 ^
/usr/include/X11/Xlib.h:83:16: note: expanded from macro 'Status'
#define Status int
               ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:165:12: note: to match this
      '('
  void Read(void* buf, int count, const MPI::Datatype& datatype,
           ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:165:8: error: class member
      cannot be redeclared
  void Read(void* buf, int count, const MPI::Datatype& datatype,
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:163:8: note: previous
      declaration is here
  void Read(void* buf, int count, const MPI::Datatype& datatype);
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:171:8: error: expected
      unqualified-id
                MPI::Status& status);
                     ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:171:3: error: expected
      parameter declarator
                MPI::Status& status);
                ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:171:8: error: expected ')'
                MPI::Status& status);
                     ^
/usr/include/X11/Xlib.h:83:16: note: expanded from macro 'Status'
#define Status int
               ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:170:16: note: to match this
      '('
  void Read_all(void* buf, int count, const MPI::Datatype& datatype,
               ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:170:8: error: class member
      cannot be redeclared
  void Read_all(void* buf, int count, const MPI::Datatype& datatype,
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:168:8: note: previous
      declaration is here
  void Read_all(void* buf, int count, const MPI::Datatype& datatype);
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:178:37: error: expected
      unqualified-id
  void Read_all_end(void* buf, MPI::Status& status);
                                    ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:178:32: error: expected
      parameter declarator
  void Read_all_end(void* buf, MPI::Status& status);
                               ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:178:37: error: expected ')'
  void Read_all_end(void* buf, MPI::Status& status);
                                    ^
/usr/include/X11/Xlib.h:83:16: note: expanded from macro 'Status'
#define Status int
               ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:178:20: note: to match this
      '('
  void Read_all_end(void* buf, MPI::Status& status);
                   ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:178:8: error: class member
      cannot be redeclared
  void Read_all_end(void* buf, MPI::Status& status);
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:176:8: note: previous
      declaration is here
  void Read_all_end(void* buf);
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:184:45: error: expected
      unqualified-id
               const MPI::Datatype& datatype, MPI::Status& status);
                                                   ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:184:40: error: expected
      parameter declarator
               const MPI::Datatype& datatype, MPI::Status& status);
                                              ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:184:45: error: expected ')'
               const MPI::Datatype& datatype, MPI::Status& status);
                                                   ^
/usr/include/X11/Xlib.h:83:16: note: expanded from macro 'Status'
#define Status int
               ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:183:15: note: to match this
      '('
  void Read_at(MPI::Offset offset, void* buf, int count,
              ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:183:8: error: class member
      cannot be redeclared
  void Read_at(MPI::Offset offset, void* buf, int count,
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:180:8: note: previous
      declaration is here
  void Read_at(MPI::Offset offset,
       ^
/home/termi/Repos/spack/opt/spack/linux-opensuse_leap15-zen/clang-8.0.0/openmpi-3.1.4-6esuakacee44dvnr3af32nhn5auvqufg/include/openmpi/ompi/mpi/cxx/file.h:190:42: error: expected
      unqualified-id
                   const MPI::Datatype& datatype, MPI::Status& status);
                                                       ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

I think this error is due to

fc9276b#diff-65fa73f3701f40e95286291d439656a479415073bf5cb2bcb81c80e19d9b235bR16-R24 pulling in some X11 macros.

publixsubfan and others added 2 commits December 3, 2020 12:26
until the queue is empty or another type event is processed.

This helps improve mouse-drag lag over remote ssh connections.
@psocratis
Copy link

The latest pull seems to be working fine for me too

@publixsubfan publixsubfan merged commit 60f3761 into master Dec 3, 2020
@tzanio tzanio deleted the upstream/modern-ogl-dev branch December 3, 2020 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.