Bug in GraphicsSystem exception handling

Discussion area about developing with Ogre-Next (2.1, 2.2 and beyond)


Post Reply
berserkerviking
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 63
Joined: Tue May 02, 2017 8:15 pm
x 16

Bug in GraphicsSystem exception handling

Post by berserkerviking »

There is a bug in the way that GraphicsSystem gets cleaned up when an exception is thrown.
Rather than printing out the information of the exception and exiting the program cleanly,
an assert occurs, and the exception info is NOT printed out. This causes a loss of valuable debug information.

For example, on the following Ogre v2-1 config on macOS I tried to run Sample_TutorialUav01_Setup.
changeset 11283:e04d99567c87

There is a bug in this program which causes OGRE_EXCEPT() to be called.
Here is the assert message:

Code: Select all

Assertion failed: (!mRoot && "deinitialize() not called!!!"), 
What should happen is the following:

Code: Select all

An exception has occured: OGRE EXCEPTION(5:ItemIdentityException): Cannot find best technique for material 'FillUav' in CompositorPassQuad::CompositorPassQuad at /Volumes/Home/home/ogfiles/src/ogdev/OgreMain/src/Compositor/Pass/PassQuad/OgreCompositorPassQuad.cpp (line 101)
Program ended with exit code: 255
From what I can tell, one can fix this problem by modifying TutorialUav01_Setup.cpp:114 to make sure that we deinitialize the graphics system before deleting it:

Code: Select all

graphicsSystem->deinitialize();  // <<<<<<< ADDED
delete graphicsSystem;
delete graphicsGameState;
The problem with this is that practically EVERY sample deletes the graphics system without first deinitializing it.
Thus, NOT calling deinitialize has become a de facto standard. Therefore, I propose that the proper fix is to delete the assertion in deinitialize().

Any comments?
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: Bug in GraphicsSystem exception handling

Post by dark_sylinc »

Yeah, this problem is very annoying (and in fact during debug I just comment out the assert to get the logs).

The reasons it works like this are several, some of them no longer apply, some still do:
  • Originally the GraphicsSystem from the demos object lived in the stack, making it painful to correctly deinitialize. This no longer applies however (got refactored into the current code you see now when the iOS port started)
  • In several platforms, graphics code needs to be called from the thread that created the Window. This means deinitialize() must be called on the render thread, while the destructor may happen from any thread (assuming it's already deinitialized)
  • The assert is there to signal the developer something wasn't correctly deinitialized (which is useful to spot silly mistakes in real apps), but it doesn't really make sense for the assert to be triggered when we're aborting because something went terribly wrong (in fact trying to correctly deinitialize could further aggravate program misbehavior).
The second point is basically the main reason simply calling deinit before deleting the pointer could be troublesome. However you do raise a very valid point that the current behavior is troublesome as well.

Maybe the assert should be changed to a logging routine with LML_CRITICAL flag set. I'm open to more suggestions.
berserkerviking
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 63
Joined: Tue May 02, 2017 8:15 pm
x 16

Re: Bug in GraphicsSystem exception handling

Post by berserkerviking »

I'm fine with doing LML_CRITICAL logging. How should we make this happen? I can't yet modify the code.
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: Bug in GraphicsSystem exception handling

Post by dark_sylinc »

Normally via pull request, but I'll give you direct access in 5 hours or so when I get back home
berserkerviking
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 63
Joined: Tue May 02, 2017 8:15 pm
x 16

Re: Bug in GraphicsSystem exception handling

Post by berserkerviking »

Okay. I've made the change locally. What is the process to request a code review?
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: Bug in GraphicsSystem exception handling

Post by dark_sylinc »

Unless you're posting to an already released branch (i.e. at the time of writing 1.9 or older) if the change is trivial just push it directly.

Otherwise you can create (or use an existing) temporary branch for testing and indicate us other devs either via forum or via Skype, or email for review.
berserkerviking
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 63
Joined: Tue May 02, 2017 8:15 pm
x 16

Re: Bug in GraphicsSystem exception handling

Post by berserkerviking »

Okay. I'll just post my diff here. Please review.

Code: Select all

diff -r 66c1f42b159e Samples/2.0/Common/src/GraphicsSystem.cpp
--- a/Samples/2.0/Common/src/GraphicsSystem.cpp Sat Jul 29 21:03:46 2017 -0300
+++ b/Samples/2.0/Common/src/GraphicsSystem.cpp Wed Aug 30 15:05:38 2017 -0700
@@ -25,6 +25,8 @@

 #include "OgreWindowEventUtilities.h"

+#include "OgreLogManager.h"
+
 #if OGRE_USE_SDL2
     #include <SDL_syswm.h>
 #endif
@@ -66,7 +68,10 @@
     //-----------------------------------------------------------------------------------
     GraphicsSystem::~GraphicsSystem()
     {
-        assert( !mRoot && "deinitialize() not called!!!" );
+        if (mRoot)
+        {
+            Ogre::LogManager::getSingleton().logMessage( "deinitialize() not called!!!", Ogre::LML_CRITICAL );
+        }
     }
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: Bug in GraphicsSystem exception handling

Post by dark_sylinc »

Looks fine but:

I'd change it to "WARNING: GraphicsSystem::deinitialize() not called!!!" since the assert previously made it obvious it was something that shouldn't happen, and the callstack would be obtained by hooking a debugger, now it's not.

Plus one more thing:
I try to keep line columns below 106 (106 is arbitrary but it's the reference I'm using), so break it in two:

Code: Select all

Ogre::LogManager::getSingleton().logMessage( "deinitialize() not called!!!",
                                                         Ogre::LML_CRITICAL );
Then just push directly.
berserkerviking
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 63
Joined: Tue May 02, 2017 8:15 pm
x 16

Re: Bug in GraphicsSystem exception handling

Post by berserkerviking »

Okay. It has been pushed.

hg log
changeset: 11401:e41a0aed6ab4
branch: v2-1
tag: tip
user: berkserkerviking
date: Thu Aug 31 13:49:26 2017 -0700
summary: Convert assertion into warning log message so info on causing exception gets printed
Post Reply