Sign in to follow this  
tomek_zielinski

Class/inheritance problem

Recommended Posts

I've got following classes in my program: CSceneObject |-CSceneCamera |-CScene......(particles,bullets etc) |-CSceneCharacter ```|-CSceneZombie_Base `````|-CSceneZombie_TypeXXXXX `````|-CSceneZombie_TypeYYYYY ```|-CSceneSoldier My problem is that some of the classes have unique methods(for example CSceneSoldier have 'OnStrafe' and other classes don't). Now - In a number of places in the code I need to call these unique methods. So I check the type of the object(I implementend pure virtual get_type() function in CSceneObject) and for example if it's soldier then I call: ((CSceneSoldier*)(*iterSoldier).get())->OnStrafe( dir, strafe ); It's so complex because I'm using STL::list and boost::shared_ptr, so first I need to dereference iterator, then get raw pointer and then cast it to proper type. But it's making my code a piece of sh.. so I figured out that I can put all these unique methods ito CSceneObject and make them throwing an exception or asserting by default, then override them in the proper class so I will avoid casting and assure that all calls will be valid. What do you think about that? Or maybe there's know solution to this problem? I'm confused how to do it in more clear way...

Share this post


Link to post
Share on other sites
There may be a better way, but I typically go with your first way. If I'm going to be making multiple calls on the child pointer, however, I tend to make a reference and use that.

CSceneSoldier*& pSoldier = reinterpret_cast<CSceneSoldier*&>(iterSoldier->get());
pSoldier->OnStrafe(dir, strafe);
pSoldier->SomethingElse();
pSoldier->WhateverElse();

That cleans things up at least somewhat.

Share this post


Link to post
Share on other sites
The idea behind virtual functions (which is what I think you're probably using now) is that you don't need to know the type because it is handled behind the scenes. HOWEVER, having lots of objects with 'null implementations' (that just throw) sounds like a very bad idea.

The idea is that when you're working with CSceneObjects you only operate on CSceneObjects. If you need to access data in CSceneSoldier from the code processing CSceneObjects, then you're doing something wrong probably.

I can't really say how to fix the design without a lot more information, but I'm not good at design anyways so I don't really know what info I'd need to make a decision.

Share this post


Link to post
Share on other sites
To make life a little easier for you, look at boost::dynamic_pointer_cast, which is built for just such an occaision. It'll make the code a little cleaner.

However, really take a look at how you do your object processing - you shouldn't need to do any downcasting, but it may be the simplest/quickest way to do it.

Share this post


Link to post
Share on other sites
I never use that approach, because I kinda consider it defeats the purpose of using virtual functions and base classes in the first place.

When an object has a property that its parent does not have, it either:

- Manages this property on its own (from the member functions it inherited from its parent) or
- Is added to another container as well, that knows its type and can then work on its properties

That is, if you store "soldier" as "scene object" somewhere, never assume it is a soldier (or even think about it). If you want to work with soldiers, get a soldier list, don't use the scene object list for that, you pervert ;)

Share this post


Link to post
Share on other sites
You already have SceneCharacter, so why not implement all the virtual onStrafe, onRun, onBite, onEtc functions in there.

This way, you'd have a class like:


class ISceneCharacter : public ISceneObject
{
public:
virtual int onRun() { return 0; }
virtual int onBite() { return 0; }
virtual int onStrafe() { return 0; }
};

class SceneSoldier
{
public:
int onRun() { // override & implement run }
// No bite method, don't override
int onStrafe() { // override strafe }
};

class SceneZombie
{
public:
// No run
int onBite() { // override and implement bite }
// no strafe
};




That's one way I see of doing it. Adding specific character methods to the character class allows you to customise these types without casting. It'd make little sense adding them to SceneObject as bullets, particles etc can't run, bite or strafe.

Share this post


Link to post
Share on other sites
Agony => txh, your way of doing this is visually much cleaner

Extrarius => Yes, I know that it isn't good idea, but at least I would avoid casts... My design is far from clean but I couldn't think any ways of imroving it. Besides my game is to be shipped soon - everything works fine and is very stable, but I if I'm using stl containers, shared ptr's, memory manager, exceptions and other things that protects me from pointer hell I would like to avoid casts that can ruin all these guards

ze_jackal => I've got to see it, it's the first time I've heard about dynamic_pointer_cast. Thx in advance! Refering to my design - yes I know that - I was thinking about virtual function CSceneObject::SendMessage derived in child classes, but I'm passing many different things that it would mess my code more than casting - I would need either to pass pointers(instead of this I prefer casting) or make another class hierarchy only to pass different messages...

Share this post


Link to post
Share on other sites
I agree with ToohrVyk above but if you must go your way then you must. The problem is described by Gamma et al in their book so you're not alone on the path you've chosen.

The path forks in the book to the solution Evolution mentioned above and to the following:

class CSceneObject {
public:
..
virtual CSceneSoldier *GetSceneSoldier() { return NULL; }
..
}

class CSceneSoldier {
public:
..
virtual CSceneSoldier *GetSceneSoldier() { return this; }
..
}

if (test=SceneObject->GetSceneSoldier()) {
test->strafe();
}

JD

Share this post


Link to post
Share on other sites
evolutional => you're right, but it won't allow to avoi catsing, because instead of CSceneObject->CSceneSoldier I will have CSceneObject->CSceneCharacter, so it's just 'swapping bottlenecks'?

ToohrVyk => I had a similar solution, but it was hard to manage - maintaining a number of connected lists.

After ToohrVyk's post I've come up to some idea. What about traversing on each frame SceneObject's list and making list of zombies, list of soldiers etc only for this one frame. It wouldn;t prevent casting but it would make it much more error-prone due to focusing all casts in one place?


[EDIT]

JesusDesoles => What should I say... It's a kind of brilliant trick - very easy and very helpful, error prone. BIG THX! I don't know what approach I will use but your solution is now my favorite.

Share this post


Link to post
Share on other sites
Quote:
Original post by tomek_zielinski
evolutional => you're right, but it won't allow to avoi catsing, because instead of CSceneObject->CSceneSoldier I will have CSceneObject->CSceneCharacter, so it's just 'swapping bottlenecks'?


True, but the idea is that you don't have to cast - you don't have to know if the Character is a soldier or a Zombie, it just has to know it's a character. Of course, you'll still get virtual functions (which may be the bottleneck you're talking about?). Then again, in my system I keep lists of (eg) Characters, not just the objects so I don't need to cast. I don't seem to have any problems with such a setup, but then again my requirements are probably lower or different than yours. I'll watch this thread to see if anyone posts anything interesting to this topic because it's probably something I'll need in the future.

Share this post


Link to post
Share on other sites
evolutional => I named it wrongly 'bottleneck', I meant that there would also be a cast. My only requirements are to clean up that mess with casting. I'm using all the safe things to protect from leaks, invalid pointers and references and one stupid cast can ruin it. I would also like to avoid separate list etc(if reasonable).

Now I think that modified JesusDesolese's approach will suit my needs, but feel free to write how are you dealing with this problem in your programs/games - maybe there are other easy-but-not-easy-to-be-seen:) solutions?? Or maybe there is a better desing out there?

Share this post


Link to post
Share on other sites
There's a good article in Game Programming Gems 3 that describes a design pattern known as the autolist. Whenever an object of a class that inherits from the autolist template is created, it is added to a static list of just those classes. When that same object is destroyed, it is automatically removed from the list. Due to the way that the list operates, adding and removing from the list is done automatically when the object is contructed and destroyed.

So, to walk the list of just CSceneSoldier , you'd use:

CSceneSoldier *soldier = TAutolists<CSceneSoldier *>::GetAutolistFirst();

while( soldier ) {
// do soldier specific stuff

soldier = TAutolists<CSceneSoldier *>::GetAutolistNext();
}



You'd do something similar to access all of the CSceneZombie's:

CSceneZombie *zombie = TAutolists<CSceneZombie *>::GetAutolistFirst();

while( zombie ) {
// do zombie specific stuff

zombie = TAutolists<CSceneZombie *>::GetAutolistNext();
}



There are some specific issues to watch for, such as when a classes parent also inherits from the autolist template, but they are fairly easy to manage as long as you are aware of them.


Share this post


Link to post
Share on other sites
Scrime => You're solutiona is very clever so that now I don't know which one is best for me:)


I've got one questin concerning what ze_jackal proposed: create CSceneObject and then do dynamic_pointer_cast down to CSceneZombie. How about creating shared_ptr<CSCeneZombie> and upcasting it to CSceneObject? Will dynamic_pointer_cast also work?

[EDIT]

lpWorld = boost::shared_ptr<CSceneWorldStatic>(new CSceneWorldStatic( this, mdlLevel ) );
lstSceneObjects.push_back( boost::dynamic_pointer_cast<CSceneObject>(lpWorld) );

Will it work well? Now list is only for common processing and rendering

[EDIT2]

I've got

enum SCENE_OBJECT_TYPE { sctCamera, sctSoldier, sctWorldStatic, sctZombie, sctGib, sctBolt, sctSmoke, sctDynLight, sctBlendEffect, sctEND };

So now I could do array of some kind type-aware lists:

ta_list<CSceneXXXXX> objects[sctEND]

I think that with such an approach list of CSceneObjects could be redunant but I've to think more about it??




Share this post


Link to post
Share on other sites
Quote:
Original post by tomek_zielinski
I've got one questin concerning what ze_jackal proposed: create CSceneObject and then do dynamic_pointer_cast down to CSceneZombie. How about creating shared_ptr<CSCeneZombie> and upcasting it to CSceneObject? Will dynamic_pointer_cast also work?

You should use static_cast<CSceneObject*> instead. CSceneZombie is-a CSceneObject, so you know it at compile time.

(EDIT--I just realized you're using boost; I thought you were just using standard C++, so now I'm not sure if my above statement applies).

static_cast is used to cast between types that can be resolved at compile time, and this is always true for up-casting.

dynamic_cast is used when you're not quite sure what the type might be, almost always for down-casting. (I think it's been said enough in this thread that if you have to rely on this method then your object design is incorrect, so I'll just say "ditto".)

Quote:

lpWorld = boost::shared_ptr<CSceneWorldStatic>(new CSceneWorldStatic( this, mdlLevel ) );
lstSceneObjects.push_back( boost::dynamic_pointer_cast<CSceneObject>(lpWorld) );

Will it work well? Now list is only for common processing and rendering

Not sure since I'm not familiar with boost (I know, it's the best library ever, but it just hasn't worked its way up my priority queue). I can say that if shared_ptr wasn't involved, you wouldn't even need to cast--you could just pass the pointer directly because CSceneWorldStatic is-a CSceneObject.

Quote:

[EDIT2]

I've got

enum SCENE_OBJECT_TYPE { sctCamera, sctSoldier, sctWorldStatic, sctZombie, sctGib, sctBolt, sctSmoke, sctDynLight, sctBlendEffect, sctEND };

So now I could do array of some kind type-aware lists:

ta_list<CSceneXXXXX> objects[sctEND]

I think that with such an approach list of CSceneObjects could be redunant but I've to think more about it??

Redundant, absolutely. You might as well just be programming in C without inheritence if you have a "type" enum.

If I can suggest a reference, Exceptional C++ (Sutter) has a chapter on the mis-uses of inheritence--which seems to be what's happening here--creep into code an how they can be avoided.

IMHO, as a person who went from knowing C to learning C++, this was the most difficult part of the language: developing a proper usage model for inheritence. I'm all for finishing what you start, so maybe a complete redesign isn't what you should do right now. But for your next project, maybe you should come to the forums and plot out the usage model first. (Then again, maybe you ARE up for a complete redesign, in which case we'll have to know how you use all of your containers with your objects.)

Share this post


Link to post
Share on other sites
Stoffel =>

Quote:
by tomek_zielinski
How about creating shared_ptr<CSCeneZombie> and upcasting it to CSceneObject? Will dynamic_pointer_cast also work?


Quote:
by tomek_zielinski
lpWorld = boost::shared_ptr<CSceneWorldStatic>(new CSceneWorldStatic( this, mdlLevel ) );
lstSceneObjects.push_back( boost::dynamic_pointer_cast<CSceneObject>(lpWorld) );


I figured out that simple lstSceneObjects.push_back( lpWorld ); is ok because implicit conversion take place and this is static cast so it's ok - I know it at compile time, it's safe. Then I can store shared_ptr's to soldiers and zombies in separate lists so I don't need to perform dynamic casting.

This way I've cleared most of my code - having list of CSceneObjects to perform updating and rendering and type-aware shared_ptrs to perform class-specific calls.

One thing I cannot clean is everything connected to Newton Game Dynamic physics engine I'm using. There are a number of callbacks where only void* is passed as parameter so there is no chance of avoiding heavy C-style casting. But I can stand this.

----------------------------------------------

But in general - how to cope with such things?

I (think I) need to have CSceneObject base class for all the entities because it provides many useful functions such as translations, rotations, common rendering, common processing and allows me to have single loop to iterate my entities in order to update/render them.

I cannot imagine how not to use class-specific functions, but have all needed stuff in CSceneObject?? It's possible but nonpractical I think - only few casts allows to keep CSceneObject and all derived classes much more readable.

BTW: As you probably realized I don't need a perfect design. I rate my code once a week and if I don't like it because of sth I'm not afraid of change/redesign/... it, but if sth isn't worth it I don't do that only because I would get bad mark at computer science class for such a piece of code;)

Share this post


Link to post
Share on other sites
Quote:
Original post by tomek_zielinski
One thing I cannot clean is everything connected to Newton Game Dynamic physics engine I'm using. There are a number of callbacks where only void* is passed as parameter so there is no chance of avoiding heavy C-style casting. But I can stand this.

You have no other choice! =)

Quote:

But in general - how to cope with such things?


I don't think there's anything wrong with your inheritence from what you've stated. If all of the objects that need to be rendered derive from a base class with virtual functions to facilitate rendering (which sounds like what you do with CSceneObject), that's pretty-much how it's supposed to work.

The problem, I believe, is in how you're containing the objects. If you just have one container of all the renderable objects, but then you try to handle all of the different classes' non-virtual behaviors there through dynamic casting, you're in trouble. What you should have instead would be a different way of containing the objects in addition to the catch-all container.

For instance, you'd have a renderer container with all CSceneObject* objects in the game, but then maybe you'd additionally have a ZombieManager container with only the CZombie* objects in them. These objects would be in both containers (since all zombies can be rendered), but the ZombieManager class would be in charge of doing Zombie-like things. So your loop would be something like

while (true)
{
zombieMgr.run ();
// -> which internally does:
// for_each (zombies.begin (), zombies.end (), findBrains);

renderer.run ()
// -> which internally does:
// for_each (sceneObjs.begin (), sceneObjs.end (), render);
}

Each container only contains the objects that are appropriate, and the key is that the zombies container contains Zombie* objects, so there's no need for casting.

BTW, this is just a very verbose way of agreeing with ToohrVyk and I believe Scrime (though I've never seen that technique before) earlier in the thread.

Now the problem that naturally arises is ownership since multiple objects are in multiple containers, and who owns what? But since you're already using the shared_ptr, which I believe is a smart pointer implementation, then it should all be taken-care of, right?

Quote:

BTW: As you probably realized I don't need a perfect design. I rate my code once a week and if I don't like it because of sth I'm not afraid of change/redesign/... it, but if sth isn't worth it I don't do that only because I would get bad mark at computer science class for such a piece of code;)


I can fully respect that. Except, what's "sth"?

Share this post


Link to post
Share on other sites
Stoffel => thank you, you are very helpful. The idea with mulitple list for different purposes is very good - I've just implemented it and I find my code polished eventually.


Very helpful for me was also that hint(by ze_jackal) with casting shared_ptrs(which turned out to be redunant because of implicit shared_ptr converions, so the code became event more simple).




Stoffel =>
Whats's sth?:) At my english classes tutors(when explaining grammar rules) were often writing: TO DO <sth>, ALLOW <sb> TO DO <sth>. And that 'sth' and 'sb' became my replacements for 'something' and 'somebody' up till now:)

EDIT

Sorry for editing but i overlooked something.

Shared_ptr is smart pointer and IMHO is a very good implementation, at least for me. It's both flexible and easy to use. It takes care of managing objects lifetime, but it isn't no-brainer, it only facilitates things if you know what you are doing. Once I forgot that shared_ptr can't hold pointers to objects that own another instance of this shared_ptr, so reference counter never reached 0 and whole branch of a tree wasn't delete[]d.

In this case, though, they are perfect - each loop iteration I can chceck if zombie reference counter is equal to 1. If it is, then zombie was deleted from main list and remains only in zombie-specific list, so I can delete it from this list too.

[Edited by - tomek_zielinski on August 18, 2004 3:34:09 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by tomek_zielinski
Now I think that modified JesusDesolese's approach will suit my needs, but feel free to write how are you dealing with this problem in your programs/games - maybe there are other easy-but-not-easy-to-be-seen:) solutions?? Or maybe there is a better desing out there?


From an OO point of view, JesusDesoles's solution is Very Bad, since it needs that your base class CSceneObject knows all its child class. This is definitely a no-no and should be avoided if possible.

Scrime's autolist is far better - at the cost of a static list instance (singleton) by object type. This is not that bad. At least, it will not polute your code with more casts.

dynamic_cast<> is also a good thing to test. It will resolve at runtime and will return NULL is your object is not a CSceneSoldier (or whatever) object.

This was just my 2 euro cents :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Emmanuel Deloget
From an OO point of view, JesusDesoles's solution is Very Bad, since it needs that your base class CSceneObject knows all its child class. This is definitely a no-no and should be avoided if possible.

Yes, once the inheritance tree expands all hell breaks loose. That is to say, expands in a way that more class spesific actions are needed. ( Helicopter->fly(), Mouse->squeek(),.. )

JD

EDIT: Hmm, I'm sure I saw someone suggest the Visitor pattern here but the post is gone, or I'm just short on coffee.

Anyway, the pattern has some considerations that apply to problem at hand (from Gamma et al.):
"So the key consideration in applying the Visitor pattern is whether you are mostly likely to change the algorithm applied over an object structure or the classes of objects that make up the structure. The Visitor class hierarchy can be difficult to maintain when new ConcreteElement classes are added frequently. In such cases, it's probably easier just to define operations on the classes that make up the structure. If the Element class hierarchy is stable, but you are continually adding operations or changing algorithms, then the Visitor pattern will help you manage the changes."



[Edited by - JesusDesoles on August 20, 2004 3:07:35 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by ze_jackal
Quote:
Original post by tomek_zielinski
In this case, though, they are perfect - each loop iteration I can chceck if zombie reference counter is equal to 1. If it is, then zombie was deleted from main list and remains only in zombie-specific list, so I can delete it from this list too.

  • Sorry if this was already posted, I didn't have enough time to read everybody's posts. I (am going to) implement it this way:

    1) You have an enum of actions (ACT_RUN, ACT_BITE, ACT_STRAFE)
    2) Your base class CSceneCharacter has a function called OnAction.
    3) Instead of calling OnStrafe(), call OnAction(ACT_STRAFE). If the character doesn't use that action, the function just returns.

    Hopefully that helps!

    Share this post


    Link to post
    Share on other sites

    Create an account or sign in to comment

    You need to be a member in order to leave a comment

    Create an account

    Sign up for a new account in our community. It's easy!

    Register a new account

    Sign in

    Already have an account? Sign in here.

    Sign In Now

    Sign in to follow this  

    Important Information

    By using GameDev.net, you agree to our Terms of Use and Guidelines.

    Create an account with GameDev.net and join the game development conversation on our forums, blogs, articles, and more!

    Sign me up!