static variable in GApp.cpp breaks debug rendering when multiple G3D::GApp instances are present in the same application. fix included.

[Mike Roberts]

Hi friends,

I've made some more progress on my G3DWidget class for use in Qt applications.

At this point, my G3DWidget class implements the full G3D::OSWindow interface. To test my implementation, I have been running the starter sample app and the pixelShader starter app concurrently in a single Qt application, with each sample app running in its own G3DWidget. Screenshot attached. To get all the details of this Qt application working, I had to make a few lightweight changes to GApp.cpp. I'd like to request feedback on these changes, because I might be missing important nuances regarding the design of GApp.cpp.

Main Issue: Static variable in GApp.cpp breaks debug rendering when multiple G3D::GApp instances are present in the same application.

There is a static variable in GApp.cpp that is used for debug rendering. It makes a lot of sense to have a static variable for this. Debug rendering is important cross-cutting functionality that you would like to be able to access easily from anywhere in an application. However, this static variable is only set in the G3D::GApp constuctor and is never updated. This breaks debug rendering when there are multiple G3D::GApp instances in the same application.

To address this issue, I simply update the static variable in each of the top-level entry points (i.e., beginRun, endRun, and oneFrame) in the G3D::GApp class. This solution is simple, lightweight, and effective in enabling debug rendering when there are multiple G3D::GApp instances in the same application. On the other hand, I admit that having multiple G3D::GApp instances repeatedly clobbering this shared state is aesthetically unpleasant. See the diff below.

Index: G3D9/GLG3D.lib/source/GApp.cpp

===================================================================

--- G3D9/GLG3D.lib/source/GApp.cpp (revision 3061)

+++ G3D9/GLG3D.lib/source/GApp.cpp (working copy)

@@ -40,7 +40,7 @@

 

 namespace G3D {

 

-static GApp* lastGApp = NULL;

+static GApp* currentGApp = NULL;

 

 /** Framerate when the app does not have focus.  Should be low, e.g., 4fps */

 static const float BACKGROUND_FRAME_RATE = 4.0; // fps

@@ -69,8 +69,8 @@

 void screenPrintf(const char* fmt ...) {

     va_list argList;

     va_start(argList, fmt);

-    if (lastGApp) {

-        lastGApp->vscreenPrintf(fmt, argList);

+    if (currentGApp) {

+        currentGApp->vscreenPrintf(fmt, argList);

     }

     va_end(argList);

 }

@@ -94,9 +94,9 @@

  const Color4&     wireColor, 

  const CFrame&     frame) {

 

-    if (lastGApp) {

+    if (currentGApp) {

         debugAssert(shape);

-        GApp::DebugShape& s = lastGApp->debugShapeArray.next();

+        GApp::DebugShape& s = currentGApp->debugShapeArray.next();

         s.shape             = shape;

         s.solidColor        = solidColor;

         s.wireColor         = wireColor;

@@ -106,7 +106,7 @@

         } else {

             s.endTime       = System::time() + displayTime;

         }

-        s.id                = lastGApp->m_lastDebugID++;

+        s.id                = currentGApp->m_lastDebugID++;

         return s.id;

     } else {

         return 0;

@@ -147,7 +147,7 @@

     m_realTime(0), 

     m_simTime(0) {

 

-    lastGApp = this;

+    currentGApp = this;

 

 #   ifdef G3D_DEBUG

         // Let the debugger catch them

@@ -446,9 +446,7 @@

 

 

 GApp::~GApp() {

-    if (lastGApp == this) {

-        lastGApp = NULL;

-    }

+    currentGApp = NULL;

 

     // Drop pointers to all OpenGL resources before shutting down the RenderDevice

     m_cameraManipulator.reset();

@@ -774,6 +772,9 @@

 

 

 void GApp::oneFrame() {

+

+    currentGApp = this;

+

     for (int repeat = 0; repeat < max(1, m_renderPeriod); ++repeat) {

         m_lastTime = m_now;

         m_now = System::time();

@@ -997,6 +998,8 @@

 

 void GApp::beginRun() {

 

+    currentGApp = this;

+

     m_endProgram = false;

     m_exitCode = 0;

 

@@ -1012,6 +1015,8 @@

 

 

 void GApp::endRun() {

+    currentGApp = this;

+

     onCleanup();

 

     Log::common()->section("Files Used");

@@ -1024,7 +1029,6 @@

     }

     Log::common()->println("");

 

-

     if (window()->requiresMainLoop() && m_endProgram) {

         ::exit(m_exitCode);

     }

Side Issue: Should G3D::GApp ever be responsible for making a window current?

G3D::GApp currently calls G3D::OSWindow::makeCurrent() in its constructor. However, if the window passed into G3D::GApp::GApp(...) is not already current, then fatal errors occur in G3D::RenderDevice::init(...), which assumes the input window is already current.

Of course, this issue is easily worked around in client code by calling G3D::OSWindow::makeCurrent() on any G3D::OSWindow used to construct a G3D::GApp instance. But is that the intended contract? As a client of G3D::GApp, should I ever rely on a G3D::GApp instance to make its window current? Am I solely responsible for doing so? If I am solely responsible, then is the call to G3D::OSWindow::makeCurrent() in G3D::GApp::GApp(...) redundant? If I am not solely responsible, then should G3D::GApp call G3D::OSWindow::makeCurrent() earlier in its constructor, and again in each of its top-level entry points (i.e., beginRun, endRun, and oneFrame)?