Ogre's Singleton global access optional

[Klaim]

First let me explain the context in which I am:

I'm setting up a concurrent-tasks-based engine, built over tbb, and I have this separate thread with the Ogre-related stuffs running.
So far so good, everything works as expected as far as no code is using Ogre outside of this thread.

The thing is, with Ogre classes being accessible globally because of the static accessors in the Singleton class, it is too easy to get in trouble. It should not be a problem most of the time, but it's better if the compiler can check that you are not accessing data that you shouldn't, as much as it can.

So I was considering this: making Ogre::Singleton class static accessors optional, removed by a simple CMake option for users who don't want global access.

First it would allow me to provide the Ogre::Root instance only to systems/code/functions that are supposed to modify something in the graphic rendering. This would be very very useful to me as it would make impossible for most of my code to try to access Ogre systems outside the graphic thread. Inside the graphic thread, I have a loop wich execute tasks (extended functors basically) which then could take the Ogre::Root instance as parameter:

Code: Select all

void my_graphic_function( Ogre::Root& ogre )
{
   // ... do something that can only be executed in the graphic thread
}
 
void GraphicEngine::main_loop() // GraphicEngine is the only object that owns the Ogre::Root instance, and is singleton itself (but not global access)
{
      while( is_running() )
      {
            task_list.execute( ogre_root ); // this will execute a list of graphic tasks that are objects which have a execute( Ogre::Root& ) function.
      }
}

I don't know for other users but I think it would be a great first enhancement for multithreaded context as it basically would allow users to make sure graphic data are not shared.

Second, from what I understand, it is possible to do this without impacting concurrent loading. However, I couldn't try this (because of an obscure problem I have with tbb+Ogre, but I'll get to this another time).

Third, I tried a first simple implementation of this idea:

Code: Select all

diff -r 1a6e4cfcb9b9 CMake/ConfigureBuild.cmake
--- a/CMake/ConfigureBuild.cmake	Fri Aug 17 21:21:32 2012 +0200
+++ b/CMake/ConfigureBuild.cmake	Sun Sep 23 01:48:57 2012 +0200
@@ -85,6 +85,8 @@
 set(OGRE_STATIC_LIB 0)
 set(OGRE_SET_USE_BOOST 0)
 set(OGRE_SET_PROFILING 0)
+set(OGRE_SET_SINGLETON_GLOBAL_ACCESS 1)
+
 if (OGRE_CONFIG_DOUBLE)
   set(OGRE_SET_DOUBLE 1)
 endif()
@@ -133,6 +135,9 @@
 if (OGRE_PROFILING)
   set(OGRE_SET_PROFILING 1)
 endif()
+if (NOT OGRE_CONFIG_SINGLETON_GLOBAL_ACCESS)
+  set(OGRE_SET_SINGLETON_GLOBAL_ACCESS 0)
+endif()
 
 if (OGRE_TEST_BIG_ENDIAN)
   set(OGRE_CONFIG_BIG_ENDIAN 1)
diff -r 1a6e4cfcb9b9 CMake/Templates/OgreBuildSettings.h.in
--- a/CMake/Templates/OgreBuildSettings.h.in	Fri Aug 17 21:21:32 2012 +0200
+++ b/CMake/Templates/OgreBuildSettings.h.in	Sun Sep 23 01:48:57 2012 +0200
@@ -55,6 +55,8 @@
 
 #define OGRE_PROFILING @OGRE_SET_PROFILING@
 
+#define OGRE_SINGLETON_GLOBAL_ACCESS @OGRE_SET_SINGLETON_GLOBAL_ACCESS@
+
 #cmakedefine RTSHADER_SYSTEM_BUILD_CORE_SHADERS
 
 #cmakedefine RTSHADER_SYSTEM_BUILD_EXT_SHADERS
diff -r 1a6e4cfcb9b9 CMakeLists.txt
--- a/CMakeLists.txt	Fri Aug 17 21:21:32 2012 +0200
+++ b/CMakeLists.txt	Sun Sep 23 01:48:57 2012 +0200
@@ -329,6 +329,9 @@
 option(OGRE_UNITY_BUILD "Enable unity build for Ogre." FALSE)
 set(OGRE_UNITY_FILES_PER_UNIT "50" CACHE STRING "Number of files per compilation unit in Unity build.")
 
+# Utility options
+option(OGRE_CONFIG_SINGLETON_GLOBAL_ACCESS "Ogre singletons are available globally for the client code (using getSingleton() static functions)." TRUE)
+
 # hide advanced options
 mark_as_advanced(
   OGRE_BUILD_RTSHADERSYSTEM_CORE_SHADERS 
diff -r 1a6e4cfcb9b9 OgreMain/include/OgreSingleton.h
--- a/OgreMain/include/OgreSingleton.h	Fri Aug 17 21:21:32 2012 +0200
+++ b/OgreMain/include/OgreSingleton.h	Sun Sep 23 01:48:57 2012 +0200
@@ -87,10 +87,14 @@
         }
         ~Singleton( void )
             {  assert( msSingleton );  msSingleton = 0;  }
+
+		// Provide global access functions only if requested or if it's Ogre code that is compiled.
+#if OGRE_SINGLETON_GLOBAL_ACCESS == 1 || defined( OGRE_NONCLIENT_BUILD ) 
         static T& getSingleton( void )
 		{	assert( msSingleton );  return ( *msSingleton ); }
         static T* getSingletonPtr( void )
 		{ return msSingleton; }
+#endif
     };
 	/** @} */
 	/** @} */

(this is based on Ogre 1.8.1)
This basically add an option to set or not global access of Ogre's singleton. It's set to true by default, like the current implementation.
If you look in details, you'll see that Ogre's code will compile with the accessors enabled, which makes it really a simple change.

So I tried to compile this and in my setup only the OpenGL rendersystem doe snot compile. I don't have all projects enabled so I don't know if DX rendersystem does not compile but I guess that yes as Cabalistic pointed before that rendersystems needs to access Ogre's root too. Here is a screenshot of all the Ogre related projects I'm compiling with:



Only RenderSystem_GL don't compile because of Ogre::Singleton accesses.
To my surprise, all other projects compiles fine, including Ogre Procedural.

Now, I wanted to see what you think guys about this?

To solve the plugins or component's or addons systems compilation to be backward compatible, a simple simple solution would be to add another define that would have to be used by any Ogre's extension source, like OGRE_EXTENSION_SOURCE, that would be checked in the same way and would automatically enable access to the global accessors. I think it's ok that Ogre-specific code have access globally to the singletons, it's only to isolate them from the client code that it's useful to disable the global access.
This solution seems good to me but it's 2 in the morning so maybe not.

Maybe there is a better solution?

What do you think?

Is a patch that would provide this option and also add the OGRE_EXTENSION_SOURCE to Ogre's plugins and components project definitions would be interesting for Ogre team? If yes, then I'll provide it ASAP and the only additional thing would be to test the patch and also notify addon maker about the necessary define to setup on ogre addon projects.

[CABAListic]

Why don't you just not use the accessors in your code? Just because they're there doesn't mean you have to use them. You can still access and share via pointer to Ogre::Root, like you are doing now. If you are consistent with that rule, it shouldn't be hard to remember and thus get in trouble...

I don't think adding a CMake option for this is a good idea. For most people it will be useless which means it won't get much attention, and that in turn means it can break easily. (I am surprised that only the RenderSystems are making trouble in the first place.)

[bstone]

Every class derived from Singleton redefines it's getSingleton() and getSingletonPtr() methods. That's most likely why everything compiles except the GL render system. So that doesn't really change much

If you can't trust yourself about not using singleton access methods then just add somewhere in your global (precompiled) project header file:

Code: Select all

#define getSingleton    __DONT_YOU_EVER_DARE_TO_CALL_THIS
#define getSingletonPtr __DONT_YOU_EVER_DARE_TO_CALL_THIS_2

Any direct use will then trigger a not defined method error at compilation stage.

[Klaim]

Why don't you just not use the accessors in your code? Just because they're there doesn't mean you have to use them. You can still access and share via pointer to Ogre::Root, like you are doing now. If you are consistent with that rule, it shouldn't be hard to remember and thus get in trouble...

That's what I'm doing right now, but I fear the point where I'll need other developers. Nothing in the code explicitly force the programmer to use Ogre only in specific contexts, while for example, for inputs (and actually all other systems in my engine), programmers have access to the input state only if they provide a functor wich takes a reference to an input state and only if they push this functor into a specific function that require that signature. Behind the scene, the functor is called only in a context where there is no need for synchronization (aka mutex) of the input state.
It really helps a lot because just at looking at the code you have strictly no way to access the input state other than by providing this functor executed in the right context with no lock.

I'm just trying to get the same kind of constraints for Ogre access.

I don't believe at all that someone in the future will not forget about this and try to do something using Ogre managers into code that will be executed in a different thread. I have no way to prevent this and in my current code it's really hard to see the constraint. I don't like the idea that I have to remember people "manually" each time they do this mistake.

Obviously I can live with the current state of Ogre, but to me some kind of solution in this way would be an enhancement. I wouldn't have been bothered in a non-multithreaded context though.

On the option maintenance side, that's why I was looking for a minimal change that would not impact Ogre's code itself, only client code.

Every class derived from Singleton redefines it's getSingleton() and getSingletonPtr() methods. That's most likely why everything compiles except the GL render system. So that doesn't really change much

Argh I totally forgot about this! So as though, this patch is a bit naive.

[s]One other very simple idea, without providing CMake option would be to have some kind of function to turn off the global access, but it would work only at runtime. :/[/s] Wouldn't work for the same reasons.

[CABAListic]

I'm not sure I really get your problem, though. In a multi-threaded system, Ogre must be restricted to a single thread and, as a consequence, I think your code should make it very obvious which parts can call Ogre and which parts cannot. If it doesn't, maybe you need to rethink your encapsulation?

If you think it's possible someone would access Ogre from the wrong parts in code, then why do you think he'd even be tempted? What would he need Ogre in those parts of the code for, anyway? Ogre should strictly be dealing with graphics, any part of your system not running in the Ogre thread should therefore not be dealing with graphics. Or at least not with Ogre directly.

You'll have to explain the basics of your code base to new developers, anyway. That, and setting good examples in the code written by you, should go a long way to noone making this kind of mistake.

[bstone]

That's what I'm doing right now, but I fear the point where I'll need other developers. Nothing in the code explicitly force the programmer to use Ogre only in specific contexts

And what about entities that might access the scene manager or cameras behind the scene? What about texture/material pointers? Hiding just singletons would change nothing really.

If you're into the defensive coding then just build a set of interfaces around Ogre that suit your needs. Expose only what can be exposed. Imho the only safe method yet with multiple issues ahead since you don't have any control of the deep Ogre internals that might still access something you'd think they wouldn't and thus break your little glass cube of safety. You'll have to foresee cases like that and wrap your code around them to ensure thread safety.

[Klaim]

I'm not sure I really get your problem, though. In a multi-threaded system, Ogre must be restricted to a single thread and, as a consequence, I think your code should make it very obvious which parts can call Ogre and which parts cannot. If it doesn't, maybe you need to rethink your encapsulation?

If you think it's possible someone would access Ogre from the wrong parts in code, then why do you think he'd even be tempted? What would he need Ogre in those parts of the code for, anyway? Ogre should strictly be dealing with graphics, any part of your system not running in the Ogre thread should therefore not be dealing with graphics. Or at least not with Ogre directly.

Ok I see that I'm not clear at all on what I'm doing, so I'll try to explain more.

What I'm trying to do is not to avoid any access to Ogre from different parts of the code, but to avoid executing code accessing Ogre in different threads than the graphic one. I'm sorry I was really not clear on this.
The difference is that, in my case, I still need more than half of the code to do graphic things (because of game design, I'll get back to it soon) but when they do, I want to inject the code into the graphic thread. I'm heavily using lambdas by the way, it really helps in this context. Let me explain schematically how things goes:

There is one library responsible for output and input systems, so in this library there is a class like that:

Code: Select all

// This class is the owner of Ogre and initialize it syncrhounsly in a thread that will be 
// the only thread to manipulate Ogre systems.
class GraphicEngine
{
public:
      GraphicEngine(); // Initialize the thread, initialize Ogre, then run in a loop which execute tasks in a "chain"
      ~GraphicEngine(); // Request the thread's end, then wait for it's end. (wait for the end of the current loop folllowed by Ogre destruction)

      /* Here is the function that inject tasks objects (augmented functors) into
          the task chain. */
      void schedule_before_rendering( Task<Ogre::Root&> ); 
      void schedule_after_rendering( Task<Ogre::Root&> ); 

private:

      // This is basically a thread-safe list of tasks. 
      // It provide a execute() function that will simply loop through registered tasks
      // (after having done some synchronization if the list have changed) 
      // and will take the template argument as argument, like all the tasks.
      TaskChain< Ogre::Root& > m_task_chain; 

      void main_loop(); // basically just call m_task_chain.execute( ogre_root ); in loop until some flag is set to make it end.
};

So with this, I can do this from any library that needs to access Ogre:

Code: Select all

void do_something( GraphicEngine& graphic_engine )
{
      graphic_engine.schedule_before_rendering( []( Ogre::Root& ogre_root ){ // this is a lambda, it will be implicitely converted to Task<Ogre::Root&>
             // here the code to be executed in the graphic thread, but before the call to render_one_frame()
      });
     // then maybe do the some other stuffs with some other systems.
};

Now, why do I even need such setup? I could have used some kind of messaging system to isolate the library manipulating Ogre, right?
Then we enter into the game's design. The design of my game requires different modules (dynamic libraries loaded onthefly) to inject some graphic, input and audio instructions that I really cannot predict by setting up a set of messages. This is obviously a very very rare case and most games shouldn't be built like this but this one needs to.
To be more clear, the game runs by presenting an interractive audio-visual representation of some game state. The design requires that both that game state and the representation can be modified by modules in an unpredictable way, that is for exemple, replacing a material by another, or even replace a mesh by another, or add or remove some graphic elements (same thing for input and audio - even the GUI!).

I'm trying to avoid boring you with game-specific design, but the point is that I need most code to be able to, if necessary, link with Ogre and manipulate it directly.
However, I still need Ogre's manipulations to happen in that specific thread. I just want to enforce this.


I'm using this pattern for all basic systems, as they have update tasks pushed into tbb, Ogre being the only one with a specific thread. It's ok as it is hidden behind the interface I just described up there. The point of doing it like this is to avoid mutex locks where not necessary, by just transfering code to be executed in the right thread (or task), at the right time. The only locks that should happen are with TaskChains (but then they can be optimized later, they already are partially).

Is it more clear what I'm trying to achieve?

And what about entities that might access the scene manager or cameras behind the scene? What about texture/material pointers? Hiding just singletons would change nothing really.

You are right, that's not enough. I think it would have avoided at least a bit part of the problem, but it's far from being perfect, I agree.
You see, I can hide the access to these elements into functions that only accept functors which can take these elements as attributes, but it's still possible to copy a pointer to an entity into code that shouldn't manipulate it, yes.
However, you still can't access it without accessing specific functions first, which make easiest to find such problems.

But it's far from perfect. ;__;

If you're into the defensive coding then just build a set of interfaces around Ogre that suit your needs. Expose only what can be exposed. Imho the only safe method yet with multiple issues ahead since you don't have any control of the deep Ogre internals that might still access something you'd think they wouldn't and thus break your little glass cube of safety. You'll have to foresee cases like that and wrap your code around them to ensure thread safety.

Yes, I would have done that in a context where I know exactly what graphic manipulation will be done by my game, but as described above, I'm in an opposite context.
I'm tempted to say "don't do this at home!" because I'm very aware that I'm in a very rare case of design and it should be avoided most of the time.

I guess I'll just have to live with the current state and just make sure it's heavily documented somewhere how to work with the engine. Thanks for feedbacks anyway.

[bstone]

You're tackling a very deep issue and unfortunately there are no simple solutions here. Lambdas are a bliss but they are merely a syntax sugar and don't solve any related problems in the field of concurrent processing.

I think you will realize relatively soon that you can't access all things you need from Ogre merely through Ogre::Root. So that's another problem.

If I'd been building a task based concurrency around Ogre then I'd have taken the following approach: have my engine/game logic code completely isolated, then have the scheduler to queue tasks into the render queue as soon as that can be done to transform whatever game state changes have been generated by the engine/game logic tasks into related Ogre calls. This way you have complete isolation, safety, and control over the threading issues. Lots of work but can't see any other way around.

[Klaim]

I think you will realize relatively soon that you can't access all things you need from Ogre merely through Ogre::Root. So that's another problem.

If I'd been building a task based concurrency around Ogre then I'd have taken the following approach: have my engine/game logic code completely isolated, then have the scheduler to queue tasks into the render queue as soon as that can be done to transform whatever game state changes have been generated by the engine/game logic tasks into related Ogre calls. This way you have complete isolation, safety, and control over the threading issues. Lots of work but can't see any other way around.

I strongly agree, but unfortunately I can't do that in this particular case, just because it would require me to make messages that reflect exactly the whole Ogre interface.
Maybe at some point I'll be able to isolate some patterns and be able to setup some message system for better isolation but I highly doubt it.

Anyway, thanks both for your help. I think I'll have to show the game for you to see why I'm in such volatile context.