Sign in to follow this  

Nasty OO design issues

This topic is 4725 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

My code crashes when I cast "sideways" (from one derived class to another derived class of a common base class). I'm using C++ with VC71. A TangibleObject has orientation and position. A VisibleObject:public TangibleObject has a draw() method to provide its own mechanism to draw itself. A Controller:public TangibleObject adds pure virtual methods to TangibleObject to move or rotate the object, facilitating human control by abstracting low level rotations through yaw,pitch,roll,move methods. NaturalController:public Controller implements these methods to provide natural quake-style first person controls. Controller and its derived classes add only methods, not variables. Camera:public VisibleObject is just a visible representation of a camera. A typical scene might have three cameras, and each camera is visible to the others. For our purposes they might as well be cubes or trees or people. This has NOTHING to do with the math/OGL camera (this is taken care of by TangibleObject and invert pushing it to OGL to see from the object's viewpoint).
    TangibleObject* camera=new Camera; //create a new object
    Controller control=(NaturalController*)camera; //take control of the object
    control->move(vector3(-.5,-.5,0)); //move it

The third line crashes. I have previously been able to cast "sideways" like this to great effect. Perhaps I have been lucky: I feel like I am playing with fire, pushing OO to my compiler's limit. Is this allowed? Why is it crashing? My previous success was with simpler inheritance- perhaps the polymorphic nature of Controller is getting in the way? Relevant headers (should be sufficiently concise)
class Object
{public:
    unsigned int id;
    std::string name;
    Object(std::string str="");
    virtual ~Object();
};

//has an orientation and position in the world
class TangibleObject : public Object
{
protected:     
    matrix44 _world;

public:
    matrix44 world(); //accessor for _world

    void scale_local     (const double magnitude);
    void rotate_local    (const vector3& axis, const double radians);
    void translate_local (const vector3& v);
    void scale_world     (const double magnitude);
    void rotate_world    (const vector3& axis, const double radians);
    void translate_world (const vector3& v);

    vector3 right();
    vector3 up();
    vector3 forward();

    vector3 global_right();
    vector3 global_up();
    vector3 global_forward();

    TangibleObject();

    void dump(int x, int y); //debugging (x,y)=screen pixels
};

class Controller : public TangibleObject
{
public:
    virtual void pitch(double rad)=0;
    virtual void yaw(double rad)=0;
    virtual void roll(double rad)=0;
    virtual void move(vector3 d)=0;
};

class NaturalController : public Controller
{
public:
    void pitch(double rad);
    void yaw(double rad);
    void roll(double rad);
    void move(vector3 d);
};

//visible
class VisibleObject : public TangibleObject
{ 
public: 
    virtual void draw()=0;
    VisibleObject();
    ~VisibleObject(){}
};


class Camera : public VisibleObject
{
public:
    void draw();
};

Share this post


Link to post
Share on other sites
Why did you expect this to work? You're calling a virtual function from Controller in a class that doesn't have Controller in it's inheritance hierarchy. Which means that the vtable entries won't be in the object. So trying to call it as if the Controller vptr was there means you're calling into what's more or less random memory. Severe bad mojo.

If you want Camera to support operations from the Controller class, then derive it from Controller.

Share this post


Link to post
Share on other sites
[edit] Crash resolved (incomplete update to new code). Works as expected :)

If I change NaturalController so that it inherits directly from TangibleObject, short circuiting the abstract base class Controller, and change the code accordingly, I get:
Unhandled exception at 0x00000000 in katana4.exe: 0xC0000005: Access violation reading location 0x00000000.

00000000 ???
00000001 ???
00000002 ???
00000003 ???
00000004 ???





and it dumps me to a useless debugger dissassembly shown above. I'm fairly certain I'm not trying to dereference an uninitialized pointer (which would explain the 0x00000000 in debug mode and the crash). WTF

[edit]
@SiCrane, OK, here there aren't virtual functions, so what you described is not happening. All I want to do is use Controller methods on a TangibleObject's data, which should be kosher because a Controller IS a TangibleObject

[Edited by - thedustbustr on January 2, 2005 1:48:48 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by thedustbustr
All I want to do is use Controller methods on a TangibleObject's data, which should be kosher because a Controller IS a TangibleObject
Nope. You can't guarantee that a TangibleObject is a Controller. Time to refactor. Consider free-standing functions.

Share this post


Link to post
Share on other sites
It was just luck. If I had to guess, I would say that there was a pointer patchup that didn't work properly as a result of casting which sent program control spinning into la la land.

As I said before, if you want to use Controller's member functions with the Camera class, than derive from Controller. Or as Oluseyi suggests, refactor to use free standing functions.

As a general rule: if you do something with a C style cast that a static_cast<> or a dynamic_cast<> wouldn't allow, then you're probably stepping into the land of undefined behavior.

As another rule of thumb: casts in general are a bad sign.

Share this post


Link to post
Share on other sites
@Oluseyi, Why not? And I know that a TangibleObject is not necessarily a Controller, but a Controller is always a TangibleObject.

[edit] It works, BTW. Just resolved the crash. ...or am I lucky again
[edit] Err, maybe not, I've updated the code several times since this... I'll have to look into it some more

Share this post


Link to post
Share on other sites
Casting 'sideways', as you put it, simply doesn't work. In your instance, a Camera is NOT a Controller (in other words, Controller is not in Cameras hierarchy, either up or down). You can ONLY cast up the hierarchy (which always works), or down (only if you KNOW for certain that the object is of that type). As SiCrane, the simple solution is to derive Camera from Controller (or NaturalController).

Though at this point you're starting to run into a class-explosion problem. Not everything is best described through inheritance: composition is also your friend.

For instance: Controller's job is really just to manipulate the position and rotation of a TangibleObject. Since these are publically accessible qualities, it's possible to remove Controller from the hierarchy entirely and simply have it compose a TangibleObject, as such:


class Controller
{
protected:
TangibleObject* _object;
public:
Controller(TangibleObject* object):_object(object) {}

virtual void pitch(double rad)=0;
virtual void yaw(double rad)=0;
virtual void roll(double rad)=0;
virtual void move(vector3 d)=0;
};



Now your concrete controller (eg. NaturalController) implements pitch,yaw,roll, and move by manipulating _objects position and rotation. Then controlling your camera becomes:


Controller* control=new NaturalController(camera);
control->move(vector3(-.5,-.5,0));



In fact, most problems can be solved using composition instead of inheritance, and it usually ends up much cleaner.

Morbo

Share this post


Link to post
Share on other sites
Quote:
Original post by thedustbustr
@Oluseyi, Why not? And I know that a TangibleObject is not necessarily a Controller, but a Controller is always a TangibleObject.
What bearing does that have on anything? You're trying to cast a Camera into a Controller (invalid), and then invoke Controller methods on the TangibleObject portion of the object. You might want to look up "slicing."

Share this post


Link to post
Share on other sites
@Morbo, That is exactly what I needed. The change in nomenclanture (from "SomethingObject" to "Controller" reflects this pattern perfectly. I'll use this

@Oluseyi, SiCrane, Thanks, I understand how I screwed it up now. You guys have been a great help in figuring this mess out

Share this post


Link to post
Share on other sites
@Morbo,
class Controller
{
protected:
TangibleObject* _object;
public:
Controller(TangibleObject* object):_object(object) {}

virtual void pitch(double rad)=0;
virtual void yaw(double rad)=0;
virtual void roll(double rad)=0;
virtual void move(vector3 d)=0;

vector3 right();
vector3 up();
vector3 forward();
};

class NaturalController : public Controller
{
public:
void pitch(double rad);
void yaw(double rad);
void roll(double rad);
void move(vector3 d);
};

//preferred way, but this probably won't work cause virtuals only work with pointers, right?
Controller control=&NaturalController(camera1);
error C2440: 'type cast' : cannot convert from 'Camera *' to 'NaturalController'

//second best way
Controller* c = new NaturalController(camera1);
error C2664: 'NaturalController::NaturalController' : cannot convert parameter 1 from 'Camera *' to 'const NaturalController &'

//only if i have to...
Controller* c = new NaturalController((TangibleObject*)camera1);
error C2664: 'NaturalController::NaturalController' : cannot convert parameter 1 from 'TangibleObject *' to 'const NaturalController &'


How do I resolve these

It's late and I'm tired and this will probably be obvious to me tomorrow morning but I cant for the life of me get this to compile tonight

Share this post


Link to post
Share on other sites
Quote:
Original post by thedustbustr
@Morbo,
*** Source Snippet Removed ***
How do I resolve these

It's late and I'm tired and this will probably be obvious to me tomorrow morning but I cant for the life of me get this to compile tonight


Constructors are not inherited. You have defined
Controller(TangibleObject* object)
but you are trying to call
NaturalController(TangibleObject* object)
which you never defined. Add this to the definition of NaturalController and the second initialization method you showed should work:
NaturalController(TangibleObject* object) : Controller(object) {}
Constructors are not inherited, but you can explicitly call the constructor of any parent class like I just showed. In fact, for all classes that do not have default constructors, you will be required to (assuming one of their constructors is not called by another parent constructor lower in the hierarchy but still in the class in question's ancestry).

Share this post


Link to post
Share on other sites
As has been stated numerous times, your casting doesn't make sense. Your camera is simply NOT a NaturalController. A Camera in your design is a VisibleObject which is a TangibleObject which is an Object. NaturalController is no where in the direct Camera hierarchy, therefore a Camera is not a kind of NaturalController (nor does it support a NaturalController interface).

It seems to me like what you are doing is attempting to make a multiple-inheritance relationship, where a camera is both a NaturalController AND a VisibleObject, only you don't fully understand the flow and meaning of an inheritance hierarchy yet. What you probably were attempting to do was this:


class Object
{public:
unsigned int id;
std::string name;
Object(std::string str="");
virtual ~Object();
};

//has an orientation and position in the world
class TangibleObject : public Object
{
protected:
matrix44 _world;

public:
matrix44 world(); //accessor for _world

void scale_local (const double magnitude);
void rotate_local (const vector3& axis, const double radians);
void translate_local (const vector3& v);
void scale_world (const double magnitude);
void rotate_world (const vector3& axis, const double radians);
void translate_world (const vector3& v);

vector3 right();
vector3 up();
vector3 forward();

vector3 global_right();
vector3 global_up();
vector3 global_forward();

TangibleObject();

void dump(int x, int y); //debugging (x,y)=screen pixels
};

class Controller : public virtual TangibleObject // So children which use multiple inheritance share one TangibleObject
{
public:
virtual void pitch(double rad)=0;
virtual void yaw(double rad)=0;
virtual void roll(double rad)=0;
virtual void move(vector3 d)=0;
};

class NaturalController : public Controller
{
public:
void pitch(double rad);
void yaw(double rad);
void roll(double rad);
void move(vector3 d);
};

//visible
class VisibleObject : public virtual TangibleObject // So children which use multiple inheritance share one TangibleObject
{
public:
virtual void draw()=0;
VisibleObject();
~VisibleObject(){}
};


class Camera : public VisibleObject, public NaturalController // Both a VisibleObject and a NaturalController
{
public:
void draw();
};

int main()
{
Camera blah;
TangibleObject& camera = blah; //create a new object
Controller& control = dynamic_cast< NaturalController& >( camera ); //take control of the object

// Or, more simply:

Controller& your_control = blah;
}



In that case, your casting makes sense, since a Camera is now both a VisibleObject AND a NaturalController. However, while this works, I would personally consider it a bad design as there are more likely much better ways to accomplish what you are attempting to do, as was mentioned in this thread.

Quote:
Original post by Morbo
Casting 'sideways', as you put it, simply doesn't work. In your instance, a Camera is NOT a Controller (in other words, Controller is not in Cameras hierarchy, either up or down). You can ONLY cast up the hierarchy (which always works), or down (only if you KNOW for certain that the object is of that type).

Just noting that you can do sideways casting (called cross-casting) with a dynamic_cast, however, not in the manner he is attempting to do. You can cross-cast when a type directly has multiple bases using multiple inheritance and you need to cast from one of those bases to the other. Again, though, that is often a sign of bad design unless you are absolutely positive it's what you should be doing (i.e. a good use can be casting between interfaces of an object whose updated versions of their interface branches out over time).

Share this post


Link to post
Share on other sites
This snippet crashes on the third line in Release mode only. Debug mode works fine.

Camera* camera1=new Camera;
Controller* control=&NaturalController(camera1);
control->move(-control->forward()*20);

Unhandled exception at 0x004056a0 in katana4.exe: 0xC0000005: Access violation reading location 0x0000000c.


Why doesn't this work in Release?

Share this post


Link to post
Share on other sites
thedustbustr: Sorry for not mentioning needing to have NaturalController define a constructor as well, and thanks to mumpo for stepping for that one.

The problem is with the second line:

Controller* control=&NaturalController(camera1);

It should be:

Controller* control=new NaturalController(camera1);

Or you could do:

NatrualController control(camera1);

If you don't need the controller to be dynamic.
As you had it, you were grabbing the address of a temporary object -- the NaturalController(camera1) -- which becomes invalid almost immediately. Debug mode tends to allow this stuff because of various memory padding it uses.



Share this post


Link to post
Share on other sites

This topic is 4725 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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