Overall Design - OOP

Everything about development and the OpenMW source code.
Locked
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

Overall Design - OOP

Post by lgromanowski »

Zini wrote: I just had another look at the code base as a whole and some things seem a bit strange, judged from my general understanding of OOP. I kinda have trouble wrapping my mind around working with the code base (beyond simple maintenance tasks). I hope I am not stepping on your toes when I criticise some of your design decisions.

What irritates me the most is the high number of dumb structures (e.g. the whole esm/load* business). This is at least an unusual way of coding. The more C++-ish way would be to use classes with proper encapsulation, consisting of the data and all functions related to it.

Regarding the example given above my understanding is that the most natural approach would be to make them classes instead with all ID-type specific functionality implemented as member functions. For example an ID could offer a render function, that works on an abstract cell interface, which then could be specialized into an OgreInteriorCell, OgreExteriorCell or even into SomeOtherRenderingEngineInteriorCell and so on.
nicolay wrote:
Zini wrote:I just had another look at the code base as a whole and some things seem a bit strange, judged from my general understanding of OOP. I kinda have trouble wrapping my mind around working with the code base (beyond simple maintenance tasks). I hope I am not stepping on your toes when I criticise some of your design decisions.
Not at all.
Zini wrote:What irritates me the most is the high number of dumb structures (e.g. the whole esm/load* business). This is at least an unusual way of coding. The more C++-ish way would be to use classes with proper encapsulation, consisting of the data and all functions related to it.
Yes, this part deviates a bit from textbook OOP and more towards pure C-like data structures. I figured though that since that is exactly what the whole ESM system is (one big data structure), which is loaded once and basically never changed (and always referenced though const pointers btw), I figured that writing getters and setters for all the fields (as an example) would be overkill. But maybe that isn't exactly what you're hinting at.
Zini wrote:Regarding the example given above my understanding is that the most natural approach would be to make them classes instead with all ID-type specific functionality implemented as member functions. For example an ID could offer a render function, that works on an abstract cell interface, which then could be specialized into an OgreInteriorCell, OgreExteriorCell or even into SomeOtherRenderingEngineInteriorCell and so on.
That's also a very valid approach. The current structure is built around the view that:
  • the ESM loader is a pure data structure
  • it is completely independent of everything else
  • it's the game layer / game state system's responsibility to make use of this data how it wishes. The ESM loader shouldn't know or care.
Since the ESM loader is not tied to the game code, we could scrap and redesign the entire game/ directory today and not have to rewrite a single line of the ESM code. We could also reuse it for an editor (perhaps add a writing component to it) or use it in a pure ESM analysis program with no render part.

Not to suggest that the code itself couldn't need use refactoring. Right now it more or less mirrors the messiness of the ESM format itself.
Zini wrote:
nicolay wrote:I figured that writing getters and setters for all the fields (as an example) would be overkill. But maybe that isn't exactly what you're hinting at.
It isn't. I am not a big fan of excessive getter/setter functions. That is the most primitive way of handling encapsulation.

If you are planning to re-use the ESM part for other purposes (editor code or other tools), it is obviously best keep as generic as possible. However it also means that you will have to duplicate all the structure in the high level part of the engine, i.e. for almost every type of ID (static, activator, ...) you will have to write special code, probably resulting in large if-then-cascades or something equally ugly.
I think you have noticed that already in your most recent commit. According to how the CS is handling it (and probably MW too) an ID without a mesh means an error situation, that probably means the ID should be ignored entirely. But for lights this is not the case because there are valid lights without a physical object attached to it. That is only one of many special cases. Another special case are the NPCs. And that is only about rendering. It becomes even more complex for using or activating.

If you want to go with this design, my proposal is probably not applicable, but I will describe it a bit more in detail anyway. The basic idea is to orthogonalise the whole system, i.e. decouple different hierarchies (it also uses conventional inheritance instead of meta-programming, which is more convenient and might be not less efficient).

Just a rough sketch:

Code: Select all

class CellBase
{
  public:
   virtual ~CellBase();
   virtual void beginAdd() = 0; // begin adding a new reference

   virtual void addMesh (const std::string& mesh, const CellRef& ref) = 0; // ref used for position, orientation and so on

   virtual void addLight (various parameter ...) = 0;

   virtual std::string endAdd() = 0; // finish adding a new reference and return a handle

};

Code: Select all

class ID
{
   public:
    virtual ~ID();
    virtual void load (ESMReader& esm) = 0;
    virtual void render (CellBase& cell, CellRef& ref) = 0; // add self to cell

    ... more functions to handle other aspects of the ID, each operating on a CellBase style interface
};

Code: Select all

class Activator : public ID
{
   std::string mesh;
   ... and other stuff

   public:
    virtual std::string render (CellBase& cell, CellRef& ref)
    {
      cell.beginAdd();
      cell.addMesh (mesh, ref)
      ref.handle = cell.endAdd();
    }
};
Note: In your current CellRef structure you don't store any handle to the screen node created in Ogre. But you will need to do that eventually.

Actual cell types would then derived from cell base:

Code: Select all

class OgreInteriorCell : public CellBase ...
class OgreExteriorCell : public CellBase ...
I don't know if you have already looked at adding references to exterior cells, but it will require a different algorithm than for the interior cells. With your current design that means you have to implement every ID-type dependent case twice (or add another layer of indirection).
Also, if you decide to add a component for a different renderer (irrlicht or whatever) you will need to reimplement all the ID-type dependend special cases in the new renderer code.

With the design described here you would contain all the ID type specific behaviour in the ID sub-classes, but the whole ESM component would still be independent of the remaining code. On the other hand you would indeed lose the ability to re-use the ESM structs for an editor.

Edit: To make sure, that there is no misunderstanding: The CellBase-hierarchy is meant for the rendered cell. It would represent a combination of game/mwrender/cell and game/mwrender/mwscene.
pogzy wrote: Hi,

A Design Pattern could take place such as a Factory that would be a loader.

Look at http://en.wikipedia.org/wiki/Factory_method_pattern for Factory, I think you already known that DP which one of the most common.

The Esm Loader could be between the usual factory and a loader http://weblogs.asp.net/fmarguerie/archi ... 86536.aspx, ok it'is not C++ but just to give the idea.
nicolay wrote: * the ESM loader is a pure data structure
* it is completely independent of everything else
* it's the game layer / game state system's responsibility to make use of this data how it wishes. The ESM loader shouldn't know or care.
I don't think ESM loader is a pure data structure, it could be better to see it a set of data that have a certain internal fonctionalities (i.e. ability to check their own contents, etc.) => smarts data structures, which is a good definition for classes :mrgreen: not structs. Let stay close to C++ and OOP benefits is a nice way to prepare easy future refactoring.

Just add some:
* ESM loader can be properly designed as OOP with classes,
* ESM loader can enforce several rules about the ESM structure (even if there are not so much for now) "wellformness" (that could be extended later, but the control can be done now)
* Always look at Design Patterns, so much cooking chiefs that did the work before us so that we could make best recipes without working too hard

And OK, getters and setters are not THE point we should take care in OOP, but it is still useful when we need to check consistancy on the fly when an accessor is called, other times we can make it without them

Sorry for my english, is it not my native language.
Locked