Code design

Started by
14 comments, last by ToohrVyk 15 years, 9 months ago
I think my "engine" project (its more like a framework..) is ...kinda messed , and im looking for a way to link all seperated modules together. I've been thinking to use polymorphism , but , im not sure if it will work well. Please tell me your opinion.. Example structure:

class Engine
{
public
Engine() ,~Engine();
virtual functions... eg :
virtual void proccessEvents() = 0;
virtual void present();
}

class Event : public Engine
{
	...
void proccessEvents();
}

class SDLCommon : public Engine
{
SDLCommon()
{
start sdl..etc
}
}

class GLRenderer : public Engine
{
setMatrix..
getMatrix..
present()
clearScreen()
putTexture( string& file... ,string name, unsigned int flags )
callTexture(string name)

}

class ConfigUtility : public Engine
{
	...........
}


Im not sure if that's the best way , that's why im posting here. So , what you guys think ? im in the right path...or...{..}?
Advertisement
Here are a few basic rules to get your design straight:

  • Keep everything as simple as possible, stay within the bounds of the language as much as possible.
  • Modules should interact through a small set of semantically rich patterns.
  • You should use modules to build other modules.
  • Always think in terms of usage: first determine how a given module will be used.


In your situation, the design seems too complex (everything inherits from Engine for no apparent reason), and the usage is not obvious at all. So, the first question you should be asking yourself is, how should your modules be used?. Write some example code that uses the modules (even if the features are not implemented yet). This will let you determine whether the interface of the modules is easy enough to use.
Quote:Original post by 3Dgonewild
I think my "engine" project (its more like a framework..) is ...kinda messed , and im looking for a way to link all seperated modules together.

Having your separate modules loosely coupled is a good thing. Of course at some point they're going to need to talk to one another and so there are a host of design patterns that allow them to do so whilst also maintaining that loose coupling.

Quote:I've been thinking to use polymorphism , but , im not sure if it will work well.

Is having a common base class for all those things useful to you? I would generally think it isn't.
Quote:
Write some example code that uses the modules (even if the features are not implemented yet).
This will let you determine whether the interface of the modules is easy enough to use.


Wow , great idea.


Hmm , well , lets see...


A simple demo :


//init..if(!Engine->Init("config_file.cfg")) DIE..;//how the audio module should work..Engine->Resource->Audio->Add(AUDIOTYPE_MUSIC,"sample","bgm.ogg");//texture cache..Engine->Resource->Textures->Add("cube","cube.tga",maybe some flags..);Engine->Start();while(Engine->Running()){		Engine->Renderer->Clear();	Engine->Renderer->Translate(..);	if Engine->Events->KeyDown(escape key)  Engine->Shutdown();	if Engine->Events->KeyDown(up key)  Engine->Resource->Audio->PlayByName("sample");	Engine->Resource->Textures->CallByName("cube");	Engine->Renderer->DrawQuad(..);		Engine->Renderer->Present();}Engine->Resource->FreeAll();



Now i will have to think how to implement this system........lol


@dmater:
I think im starting to understand how important is to have seperated modules.

Btw , that site looks EVIL ...lol
Small tip: You can cleanup the code a bit by using references (or pointers):

//init..if(!Engine->Init("config_file.cfg")) DIE..;// Save referencesRenderer &renderer = Engine->Renderer;Resource &resource = Engine->Resource;Audio &audio = resource.Audio;Textures &textures = resource.Textures;Events &events = Engine->Events;//how the audio module should work..audio.Add(AUDIOTYPE_MUSIC,"sample","bgm.ogg");//texture cache..textures.Add("cube","cube.tga",maybe some flags..);Engine->Start();while(Engine->Running()){		renderer.Clear();	renderer.Translate(..);	if events.KeyDown(escape key)  Engine->Shutdown();	if events.KeyDown(up key)  audio.PlayByName("sample");	textures.CallByName("cube");	renderer.DrawQuad(..);		renderer.Present();}resource.FreeAll();
Quote:Original post by 3Dgonewild
Now i will have to think how to implement this system........lol


Wait. Before you do that, don't forget to simplify your design to the core. Here's some examples:

Quote:
if(!Engine->Init("config_file.cfg")) DIE..;
Too complex. Initialization is already a language-provided feature (in terms of constructors). You can simplify this line to a constructor call (which will throw an appropriate exception if it fails). Also, you need to decouple configuration-loading functionality to a distinct module:
Engine engine( Config("config_file.cfg") );


Quote:
Engine->Resource->Audio->Add(AUDIOTYPE_MUSIC,"sample","bgm.ogg");Engine->Resource->Textures->Add("cube","cube.tga",maybe some flags..);
First, why pointers? Unless you want to be able to return "null" in any of these situations, you should be using references (or plain old values instead). Also, using a flag instead of a distinct function creates issues as soon as loading music requires different arguments than non-music. So, use a specific function for loading music. Again, separate filenames from actual filename data to account for potential path rewriting. Also, apply a hash function to your strings to make it faster to handle than plain old strings:
engine.resources.add_music( key["sample"], path("bgm.ogg") );engine.resources.add_texture( key["cube"], path("cube.tga") );


Quote:
Engine->Start();while(Engine->Running()){	Engine->Renderer->Clear();	Engine->Renderer->Translate(..);	Engine->Renderer->Present();}
I strongly suspect that the above will be happening in almost every single game using the engine. Therefore, it should be encapsulated away. I would suggest using a function that runs until the engine decides it should stop, and which calls a provided function on each render frame (and is responsible for setting up the next frame). To allow for time-based game management, I would further provide a time argument to that function. For now, let's suppose that this function is defined elsewhere. The end result is:
struct local{  bool game_logic(Engine &engine, float time) { /* Game logic here */ }};engine.run(local::game_logic);


Quote:
if Engine->Events->KeyDown(escape key)  Engine->Shutdown();if Engine->Events->KeyDown(up key)Engine->Resource->Audio->PlayByName("sample");
This is globally correct. However, in order to avoid shutting down the engine then still doing things, I will decide that instead of calling a shutdown function, simply having the game logic function return false will shut down the engine. As for the music, I would separate the "get music" and "play music" parts from one another, perhaps moving the "play music functionality" to an adapted module such as a playlist. Therefore, I would add to the game logic function:

if (engine.input.keydown(KEY_ESCAPE)) return false;if (engine.input.keydown(KEY_UP))  engine.playlist.start(    engine.resources.get_music( key["sample"] ) );/* Rendering code here */return true;


Quote:
Engine->Resource->Textures->CallByName("cube");Engine->Renderer->DrawQuad(..);
This is a bad idea, because it requires you to work in terms of state. Instead, you can perform the entire quad rendering in a single statement. That statement can of course delay the rendering to a later time, for instance to group quads by texture or to perform geometry instancing.

engine.render.draw_quad(   transform,   engine.resources.get_texture( key["cube"] ) );


Quote:
Engine->Resource->FreeAll();
Use a destructor.

The end result:
Engine engine( Config("config_file.cfg") );engine.resources.add_music( key["sample"], path("bgm.ogg") );engine.resources.add_texture( key["cube"], path("cube.tga") );struct local{  bool game_logic(Engine &engine, float time)   {     if (engine.input.keydown(KEY_ESCAPE)) return false;    if (engine.input.keydown(KEY_UP))      engine.playlist.start(        engine.resources.get_music( key["sample"] ) );    engine.render.draw_quad(       transform,       engine.resources.get_texture( key["cube"] ) );   return true;  }};engine.run(local::game_logic);

Quote:Original post by ToohrVyk
you need to decouple configuration-loading functionality to a distinct module:
Engine engine( Config("config_file.cfg") );


Small question - would it be worse if you just pass the file name and internally the engine's constructor created a Config object? This will make the constructor call shorter so I'm wandering why you chose to do it this way.
Quote:Original post by Gage64
Quote:Original post by ToohrVyk
you need to decouple configuration-loading functionality to a distinct module:
Engine engine( Config("config_file.cfg") );


Small question - would it be worse if you just pass the file name and internally the engine's constructor created a Config object? This will make the constructor call shorter so I'm wandering why you chose to do it this way.


When unit testing, I want to be able to build the Config object myself, instead of having to load it from a file. Also, it helps reduce coupling (now, the engine doesn't need to know how the configuration is loaded or created).

Also, I don't want to give my configuration an implicit constructor, which means I'd have a constructor like this:
Engine::Engine(const std::string &filename) :  resources( /* ? */ ), input( /* ? */ ), renderer( /* ? */ ){  Config config(filename);}


So the configuration object would only be created after I had made the calls to the constructors which need subsections of said configuration object! C++ takes the middle way between O'Caml and C# here: like C#, it isn't expressive enough to create things "before" the constructor call and, like O'Caml, it does not allow calling a constructor from another. In the end, it's annoying like hell.
Quote:Original post by ToohrVyk
When unit testing, I want to be able to build the Config object myself, instead of having to load it from a file. Also, it helps reduce coupling (now, the engine doesn't need to know how the configuration is loaded or created).


I assumed that the Config object will always be created from a file, but I guess that this is needlessly limiting my options?

Quote:Also, I don't want to give my configuration an implicit constructor, which means I'd have a constructor like this:
Engine::Engine(const std::string &filename) :  resources( /* ? */ ), input( /* ? */ ), renderer( /* ? */ ){  Config config(filename);}


So the configuration object would only be created after I had made the calls to the constructors which need subsections of said configuration object!


Ah I didn't think about that! [smile]

BTW, what do you mean by implicit constructor?
Quote:Original post by Gage64
BTW, what do you mean by implicit constructor?

An implicit constructor is a regular constructor taking a single argument which makes that class implicitly convertible from another type.

When implicit construction is not what you want then you can guard against it with the explicit modifier to make explicit constructors.

This topic is closed to new replies.

Advertisement