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

[Morgan McGuire]

I like the currentGApp model.  Although...why multiple GApps in a single program?  Why not one app with multiple RenderDevices?

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

RenderDevice shouldn't. I don't think that GApp should either...this has to be the client code's responsibility because it is slow to set the context and you have to understand the threading model of the entire application.  I could be convinced otherwise by a good design.

-m

Prof. Morgan McGuire
Computer Science Department
Williams College
http://cs.williams.edu/~morgan

On Sun, Apr 7, 2013 at 8:39 AM, Mike Roberts <mikerob...@gmail.com> wrote:

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. My G3DWidget class integrates with the G3D event system, forwarding all including all keyboard and mouse events, drag and drop events, and window focus events. 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. As far as I can tell, all user input functionality in these embedded sample apps works behaves identically to the functionality in their standalone counterparts. 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 its design.

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

This is the more important of the two changes I made. I say this because I can't think of any purely client-code workarounds. Anyway, 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)?

--
You received this message because you are subscribed to the Google Groups "G3D Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to g3d-users+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Mike Roberts]

That's a great question. All things considered, I think that allowing multiple GApp instances results in simpler client-level code and simpler library-level code.

For example, the current constructor of GApp takes a single OSWindow as input. Suppose instead you wanted to pass in multiple OSWindows, or multiple RenderDevices. You could certainly do this, but you would need to reimplement a lot of the functionality in the GApp base class. You'd need to create multiple instances of WidgetManager, UserInput, and FirstPersonManipulator somewhere. Each of these classes take an OSWindow as input during initialization. Of course, this is easily accomplished, but it means changing GApp::m_widgetManager to be a list of WidgetManagers, GApp::m_debugController to be a list of FirstPersonManipulators, etc. Quite an intrusive change in GApp.

Alternatively, you could make these changes in a derived class (e.g., GMultipleWindowApp derived from GApp). But since GApp::m_widgetManager is protected, any derived classes would have access to m_WidgetManager (which would be NULL all the time because you're not using it any more) and the new GMultipleWindowApp::m_widgetManagers list. This might lead to confusion in client code. You'd also need to reimplement any code from GApp that refers to GApp::m_WidgetManager with new code that handles this new list of WidgetManagers. Repeat for UserInput and FirstPersonManipulator.

In addition, you'd need to reimplement GApp::show(...), GApp::onRun(...), GApp::oneFrame(...), GApp::endRun(...), and GApp::processEventQueue(...). These methods assume there is a single OSWindow associated with the GApp. You'd need to change all of this code to handle multiple OSWindows.

But suppose you did all that. Now you have a GMultipleWindowApp class that is working correctly and is fully debugged. What do you do in your MyMultipleWindowApp::onGraphics3D method? You'd probably like some way of encapsulating all the client-specific rendering functionality associated with a single window. That way, you could call the doRendering(...) method for each window in a loop inside your MyMultipleWindowApp::onGraphics3D method. Suppose you'd like to do the same thing for user input, so you can call the doUserInput(...) method for each window in a loop inside your MyMultipleWindowApp::onUserInput method. At this point, the interface for the single-window functionality starts to look a lot like the original GApp interface.

In fact, you there is a strong argument in favor of putting the code for individual windows in classes derived from GApp. This design allows you to easily create a stand-alone app that runs a single one of your windows. For example, you could imagine some kind of game level designer tool with a bunch of different windows. The shipping game would just be the main window from the level designer tool running in full-screen with a few extra systems turned on. In support of this modular design, it makes sense to have the code for each window live in a class derived from GApp. But now you're back to the original problem of debug rendering in multiple concurrent GApps  :)

So, with all of these design considerations in mind, I think supporting multiple GApps is the correct approach.

[Mike Roberts]

Also, I agree that RenderDevice should not be responsible for making the window current. It is too low-level to know the appropriate times to do this. To be consistent, RenderDevice would need to call makeCurrent at the top of each of its entry points, which seems like an excessive amount of interface chatter.

I think the argument against GApp calling makeCurrent at the top of its entry points is less clear. GApp is high-level enough to know when the rendering loop is beginning and ending, so it would only be calling makeCurrent only once per frame. And, there is already a state caching mechanism in OSWindow to ensure that redundant to calls to OSWindow::makeCurrent() do not call the actual driver.

Since this state caching mechanism is already in place in OSWindow, I would actually be in favor of GApp taking responsibility for calling makeCurrent at the top of its entry points. This contract would make the client code even cleaner and even harder to mess up.

On Sunday, April 7, 2013 6:56:17 AM UTC-7, Morgan McGuire wrote:

[Morgan McGuire]

Actually, why use GApp at all if you have some overarching application?   Most of what it does for you will already be handled by your window manager.  GApp's primary value is event delivery (i.e., all of the onX methods) and initializing the window system.  If you're using a non-G3D event system, then using G3D's timers and keyboard and mouse handling might not be as useful.

I can imagine still wanting to use FirstPersonManupulator and ThirdPersonManipulator, although you could just inject the mouse events for those directly.

We're also rethinking GApp in light of the fact that all of the sample projects have really similar code.  On one hand, we'd like to abstract that shared code.  On the other hand, it is exposed right now because the programmer needs to be able to get right into the middle of it and make fundamental modifications for most real applications.

-m

Prof. Morgan McGuire
Computer Science Department
Williams College
http://cs.williams.edu/~morgan

[Mike Roberts]

Point taken. In my view, the major advantages to using the GApp interface, even in overarching applications, are as follows:

- Inheriting from GApp means that in some builds, your code can run in an overarching application (e.g., a level designer tool for a game). In other builds, the same code can run as a standalone app (e.g., the actual game running in full screen). Having the flexibility to do this is quite compelling.

- The GApp base class gives you well-defined and intuitive GLUT-style entry points out of the box, which are nice to work with. These "game-centric" entry points often make more sense in graphics applications, as compared to the "GUI-centric" entry points you get in windowing libraries like Qt.

- The GApp base class gives you a lot of nice code that works out of the box. For example, it would be a shame to have to reimplement the developer HUD code, the debug-rendering-from-anywhere code, the GEvent processing code, etc in client code. Some of the logic in GApp.cpp is quite nuanced. At least it appears that way to me. If you reimplemented all that logic yourself, it would be harder to guarantee the same behavior in your standalone game, as compared to your level designer tool. Especially as new versions of G3D come out.

Question: why bother with processing GEvents when you're getting native events from a windowing system?

Answer: G3D Widgets. You need to convert native events to GEvents to hook into a G3D::WidgetManager. So, if you give up on GEvents, you also give up on G3D Widgets, which is a big negative. In principle, you could reimplement all the G3D Widgets you think are useful (e.g., the texture viewer widget, the video recorder widget, etc.) in whatever windowing library you're using. If you were building a souped up really industrial-strength tool, you'd probably want to do this. But if you're on a small team of hobbyists, and the existing G3D Widgets meet your needs, it would be nice to avoid all the extra effort.

So yeah, +1 for using GApps in overarching applications  :)

Cheers,

Mike

[Morgan McGuire]

That makes a lot of sense, thanks.

-m

Prof. Morgan McGuire
Computer Science Department
Williams College
http://cs.williams.edu/~morgan

[Mike Roberts]

Hi Prof. McGuire,

It sounds like you agree that there are some compelling use cases for multiple GApps in the same application. If that is true, I would like to suggest the above patch (the one with the currentGApp static variable to support debug rendering from multiple GApps in the same application) for inclusion in G3D.

The reason I'm interested in this change is because I'd like to post my G3DWidget demo application on GitHub (the one with multiple GApps in the same Qt application). But my demo application depends on the above patch, which means that anyone downloading my code would need to manually patch G3D to get debug rendering working. I'd like to avoid imposing this burden on developers that might use my code.

Cheers,

Mike

[Morgan McGuire]

Hi Mike,

Following your suggestions, we plan to add support for a currentRenderDevice and currentGApp, but that support won't come until late June on our development schedule.

-m

Prof. Morgan McGuire
Computer Science Department
Williams College
http://cs.williams.edu/~morgan

[Mike Roberts]

Great! I'll keep my eyes peeled  :)

[Mike Roberts]

Out of curiosity, did this feature make it into G3D9?

Cheers,

Mike