Rate my engine!

Started by
56 comments, last by thre3dee 16 years, 10 months ago
Many moons ago I created a mess using singletons. On paper it was great, but shall we say my experience was very educational in "how not to do it" [lol]

When designing and structuring your code, ESPECIALLY at the early stages, give some thought to maintenance and extensibility.

Late in the aforementioned project I was doing profiling and debugging - the whole "global singleton mess" made it very hard to reason about the system. I spent a lot of time just finding the route of execution to a problem (if everything can talk to everything then you've got a lot of possibilities!). Then once I found the source I'd often have to spend a lot of time reasoning about what else I'd break if I changed it (one route's bug might be another route's crutch). If everything can depend on everything else, it's a nightmare.

Extending the code-base in any significant way was hard because of all the strong binding and lack of organisation. It was ten times harder if we needed to modify/"improve" an existing part to make way for a new component to be dropped in.

At the end of the project it was about 90% bandage and 10% clean code. The number of unholy hacks and work-arounds was unbelievable. Yes, not entirely down to singletons, but it was a dominate design pattern from the outset and I do think it played a significant role in the eventual mess.


It's no bad thing to consider design in the context of you (or other developers) working with it. A good design tends to encourage good code and good code makes a developer happy [wink]


hth
Jack

<hr align="left" width="25%" />
Jack Hoxley <small>[</small><small> Forum FAQ | Revised FAQ | MVP Profile | Developer Journal ]</small>

Advertisement
Might be worth pointing out at the Games Dev course I'm on, half of one of my third year modules is "Singletons and why not to use them." Specifically for the reason mentioned above ;)
Quote:
If I understood your architecture right, your engine receives references to objects it needs to render, places them in a render queue, and various internal classes manage the how and when of the actual rendering based on what kind of object it is/treatment it requires. You also (mostly) abstract the API from the client and let your interfaces deal with that aspect.

The API is entirely abstracted -- clients of the rendering subsystem cannot access any D3D-specific objects or enumerations (at least not through any interface I expose), although they may be able to access my wrappers of said objects (BufferResource, for example). Other than that, yes, your understanding is correct.

Quote:
The project originally began in OpenGL (screenie) but for various reasons I decided to restart it.

Read this.

Quote:
So only the Engine class is a singleton and thus can be referenced anywhere, and the sub-systems can be gotten from that.

That being said, a class that uses a sub-system might have the Engine * passed in to its ctor so it can get any sub-system references it needs..

BTW, this is a very basic version of my proposed structure, and its pretty early on in terms of development so this change will be very easy since I haven't actually started coding the Engine super class yet.

Anything you can recommend?

"Engine" itself doesn't need to be a singleton. In making it one, you're still falling prey to all the pitfalls, even though it might be the only singleton in the system. This is a good example of how singletons introduce dependency rot.

Since the Engine is accessible globally, so are all its aggregate members -- FileSystem, Renderer and Input are essentially singletons too, because they will be accessible via Engine::GetInstance() or whatever, and it is thus impossible to tell who might be depending on what aspects of the Engine and where.

I would strongly reconsider the whole "Engine" aggregate itself. Unless Engine is responsible for setting up the application entry point and handling events, and only calls out to a stub, user-supplied entry point, there is little reason to bother with an "Engine" object. What I'm talking about are engines that are actually contained in an .exe and expect user code to be in a DLL loaded at startup, entirely abstracting away the application bootup and event chugging from the user code. From the organizational breakdown of your engine you've posted, this doesn't seem to be the case - it looks like user code creates the .exe and links to your engine DLLs and libs. Engine is consequently just a dumb container that doesn't really offer any advantages, but has the disadvantage of allowing you to bleed dependencies into unrelated code (see my comments below).

"Subsystems" is similarly stupid, since it is just a dumb container for the actual subsytems and serves no purpose -- it doesn't even create the subsystems when it itself is created, which means it is pathologically coupled to the Engine class unnecessarily.

If nothing else you need to roll these two classes together. And unless there is a compelling need (for example, complex creation logic and data you don't want exposed to the client), you can eliminate Engine and Subsystems completely and let the user create the parts directly.

Quote:
That being said, a class that uses a sub-system might have the Engine * passed in to its ctor so it can get any sub-system references it needs..

Why not just pass the subsystem, instead of the Engine itself, since that allows the class to potentially access more than just the subsystem it needs.

On another note, it's important to consider the difference between implementation of a singleton and what a singleton conceptually is. As above, your Engine basically implicitly makes each subsystem a singleton, especially if those subsystems cannot be created individually by client code. There is still a singularity restriction and there is still global, dependency-hemorrhaging access to them. Just because they don't fit the standard C++ idioms for singletons doesn't mean they aren't.

I've had some questions in PMs and on the IRC channel (#gamedev) about the distinction between implementation detail in other languages, too. For example, in C#, you can create static classes that can not be instantiated. They serve, essentially, as fake namespaces allowing you to write emulations of free functions. But they're also classes, so they can hold (static) state. What this means is that static classes are not necessarily singletons, although they could be. At that point the distinction becomes semantic -- if the object holds state and the various static class methods operate as a unified but single object, then yes, you might have a singleton and you might have a potential design flaw, depending on the nature of the class and its functionality role.

The same thing can happen with namespaces, in C++, and functions that operate on shared translation-unit-local data. This can act like, and suffer from the same problems as, a traditional singleton if you're careless. In practice, given the mindset we often switch to with procedural code (functions are procedures that may mutate state, but that state is not stored with the procedures and rather with the client, lifting the one-only restriction on state and thus rendering the entire thing not-a-singleton), this doesn't quite happen often enough to warrant strict warnings against the practice, fortunately.

Of course, that still leaves you with global accessibility -- but that alone is not necessarily a problem. After all, things like logging mechanisms should have some kind of global accessibility point -- it just shouldn't be "the one and only global accessible single shared-state log object."

[Edited by - jpetrie on May 24, 2007 8:47:47 AM]
Firstly, my VS project is setup with a number of projects. Its structure looks like this:

+ Other Projects
- external source libraries such as Squirrel

+ Projects
- Game Project (the actual game exe to build etc)
- Codecs or plugins for the engine (which build to *.plugin as DLLs)

+ SGE Components
- SgeCore
- SgeData
- etc...

Secondly:
At the moment, for testing whatever I'm adding to the codebase, I'm creating all the different subsystems myself anyway because I don't have some big core class to do it all.

As well, having to add FileSystem * fileSystem to every constructor on an object that may need a FileSystem is going to get really annoying if I don't have some sort of global access (either implicit through one big central class or explicitly through singletons (which I've consequently decided against)).

One thing I just thought about is plugins. If theres no global access to anything, I really don't want to pass in 8 pointers of systems into the plugin's pluginInit callback function which would then require refactoring of the PluginSystem class....

Either way, having NO global access is going to royally screw up coding efficiency, especially if I ever want to access a subsystem just once, and have to go through a whole load of interfaces and functions to get to the instance...
Quote:Original post by thre3dee
having NO global access is going to royally screw up coding efficiency, especially if I ever want to access a subsystem just once, and have to go through a whole load of interfaces and functions to get to the instance...


That is a design flaw, not a selling point. You should design components in such a way that coupling is minimized.
Quote:Original post by thre3dee
Either way, having NO global access is going to royally screw up coding efficiency, especially if I ever want to access a subsystem just once, and have to go through a whole load of interfaces and functions to get to the instance...


So why make it so hard to obtain an instance? If something ends up being buried really, really deep into your system, it's likely to be something you won't want exposed to the outside world. If it's supposed to be acquirable by something else, you should plan your design with that in mind, IMO.

Relying too much on global access can also be bad for modular code. Suppose you were to write an awesome particle system that relies heavily on your project's pool of global resources, you'll have a hard time picking it out of the project and into another one later one. This is just my opinion, but it sounds like passing a mere 8 pointers to some init function once is well worth doing if you can reuse that code elsewhere without having to refactor it everytime you do so. ;)
The one thing I don't get is why something like the FileSystem shouldn't be globally accessible?

Everything uses it. Its probably one of the most used general systems in a game engine.

I think that when you start getting specific with a system then it may need to be tucked away for use by only a few things. But having to dig around to simply open a file (which has very very few dependencies) is rather stupid.

I think if a system is going to be used by a lot of other systems frequently then its access should be simple and straightforward.

When coding in Win32 you don't get some LPFILESYSTEM pointer passed into WinMain to use files on a hard disk. Its just a set of global functions and objects that anything can use, and that makes it very easy to code with.

It just seems utterly useless to destroy accessibility to something like a file system wrapper for the mere sake of being apparently a 'good' designer.

Renderer would be a different story, as well as say, the sound system. You wouldn't have them globally accessible because they would be accessed implicitly through scene and resource functionality.
Quote:
The one thing I don't get is why something like the FileSystem shouldn't be globally accessible? Everything uses it. Its probably one of the most used general systems in a game engine.

Everything shouldn't use it, that's your problem.

When do you need to do file IO? When you load data, and when you save data. Only the objects dedicated to loading and saving data should need access to the filesystem. I'll ignore saving, because that usually happens with, like, game state. And I find game state boring. Let's talk pretty graphics.

A common graphics-related item to load is a 3D geometry mesh (model). You might think that because you have to load mesh data from a file, that your Mesh class needs to have access to the file system. But this is wrong.

Consider the responsibility of the Mesh class: to hold 3D geometry data in a fashion suitable for drawing by the primary rendering mechanism. In other words, it holds vertex buffers and index buffers, maybe provides some accessors to say how many vertices or indices are in the mesh, and provides either a mechanism to allow the main renderer to "pull" the geometry data from the Mesh, or a mechanism to "push" the geometry to the main render system (either is fine, and up to your sense of style; I prefer the pulling mechanism to further reduce coupling).

Now, consider what happens if we make Mesh able to load itself from a file, perhaps with a Mesh::LoadFromMD5() method (for Doom III models). Now the responsibility of the Mesh class has widened to encompass everything we just mentioned, plus the responsibility for performing file I/O and interpreting logical file formats and turning them into internal mesh data. How many responsibilities is that? Here's a hint: it ain't one.

Mesh should be able to construct itself from a pair of arrays or vectors containing the appropriate vertex or index data, or from some other intermediate object that models the native format of geometry in your renderer. This allows you to divorce the rendering and loading responsibilities; you place the loading responsibility with an object or free function that knows how to interpret whatever file format you want and turn it into an internal representation, which is then used to construct a new Mesh.

This has the extra advantage of allowing you to extend your engine to support additional model formats without modifying the Mesh class itself, which lends itself well to eventually constructing a robust plug-in-based content pipeline and toolchain for your engine (which are essential components of any nontrivial engine).

So you might end up with this:
typedef std::vector<MyVertexStruct> MeshVertexData;typedef std::vector<short> MeshIndexData;class Mesh{  public:    Mesh(const MeshVertexData &vertexData,const MeshIndexData &index);    //...}namespace MeshIO{  Mesh* LoadFromMD5(FileSystem *fileSystem,const std::string &fileName)    {     MeshVertexData vertexData;    MeshIndexData indexData;    // Do MD5 model loading, storing vertex and index data into the above,    // using the fileSystem routines. Whatever they are.    return new Mesh(vertexData,indexData);   }}
Well here's something to think about for an arguement for having two file systems. Say you have two classes that want to have two working directories, this is a lot easier to set up with two seperate file systems then having a single file system that's a singleton.
Quote:Original post by thre3dee
The one thing I don't get is why something like the FileSystem shouldn't be globally accessible?

Everything uses it. Its probably one of the most used general systems in a game engine.


Like jpetrie said, you have serious issues if your sound system, renderer, UI, AI, etc need access to your FileSystem.

My material class has no clue how to load. It requests the file from the resource system which returns a pointer to the properly loaded material OR logs an error. What handles the loading? A registered class for the file extension requested.

Also, do you not have a messaging system? Mine uses global functions contained within a namespace. Need a pointer to the FileSystem? Request it. The classes register themselves to handle messages.

"Those who would give up essential liberty to purchase a little temporary safety deserve neither liberty nor safety." --Benjamin Franklin

This topic is closed to new replies.

Advertisement