Sign in to follow this  
Batzer

Object initialization design issue

Recommended Posts

Batzer    592

So I have been encountering a number of annoying design decesions, because I want to initialize my objects with the constructor.

Sounds weird but let me explain: Say I have a class A and class B. Class B manages multiple instances of A, but A depends on an initialized object of B.

 

Here is a concrete simplified example from my code:

class Renderer {
private:
    ID3D11Device* device;
    ConstantBuffer cbuffer;
}

class ConstantBuffer {
public:
    ConstantBuffer(ID3D11Device* device);
}

So the issue in this case is that cbuffer needs an initialized ID3D11Device in its constructor. I hope the problem is kind of clear now.

How do you get around this issue without using pointers? Or is this just a design flaw on my side?

Share this post


Link to post
Share on other sites
ApochPiQ    23005
class DeviceWrapper
{
public:
    DeviceWrapper()
    {
        // Initialize MyDevice here, according to RAII principles
    }

    ~DeviceWrapper()
    {
        // RAII again: clean up MyDevice
    }

private:
    ID3D11Device* MyDevice;
};

class ConstantBuffer
{
public:
    ConstantBuffer(DeviceWrapper& wrapper)
        : MyDeviceWrapper(wrapper)
    {
    }

private:
    DeviceWrapper& MyDeviceWrapper;
};

class Renderer
{
private:
    DeviceWrapper MyWrappedDevice;         // IMPORTANT - must be first (order of construction is crucial)

    ConstantBuffer MyConstBuffer;

public:
    Renderer()
        : MyConstBuffer(MyWrappedDevice)
    {
    }
};

Share this post


Link to post
Share on other sites
Batzer    592

Thanks for your fast reply!

So basically making a wrapper class is a valid solution for this kind of problem.

I kind of get the impression that the body of a constructor is useless in C++ since it seems to be best practice to initialize everything in the initializer list.

Is using a pointer (or std::unique_ptr) in this case considered bad practice?

Share this post


Link to post
Share on other sites
ApochPiQ    23005

Constructor bodies are still useful in some cases where you need to perform additional logic beyond just constructing members. For instance, the constructor of the DeviceWrapper is the perfect place to do your D3D device initialization.

 

A pointer (or smart pointer) is totally legitimate [b]if[/b] you want it to be nullable and/or portray some specific ownership semantics. I prefer references when possible, since they are a little less error-prone, but nobody is going to hang you by your entrails for using a pointer to a component that you need access to - especially if the lifetime/ownership semantics of that component are nontrivial.

Share this post


Link to post
Share on other sites
Aardvajk    13207

Something I've gradually started to come to prefer over the years is to not actually bind, in this example, the device to the renderer but to just pass the renderer a reference to the device in every method that needs to use it. Renderer an odd example, but a more realistic example, a Texture, I used to do:

class Texture
{
public:
    Texture(GraphicsDevice &graphics);

    void load(const std::string &path);

private:
    GraphicsDevice *graphics;
};

I've come to prefer:

class Texture
{
public:
    Texture();

    void load(GraphicsDevice &graphics, const std::string &path);
};

I'd be hard pressed to say exactly why I find this easier to deal with and maintain if I'm honest but over the course of an entire system, it feels cleaner to me. Since I can't actually identify clear reasons, this may be me being superstitious.

 

EDIT: Suppose one thing I like is I'm not hiding dependancies. I.e. with approach 1, if I pass a Texture around I am silently and invisibly passing a GraphicsDevice around as well. With the second approach, this is explicit and so easier to track where the GraphicsDevice object is moving to. Bit moot if the Texture interface never exposes the GraphicsDevice though to be fair.

Edited by Aardvajk

Share this post


Link to post
Share on other sites
jmpep    375

In your case, you might want to consider NOT requiring ConstantBuffer to have an initialized device in the constructor. I would then defer the initialization to "init" methods. Something like:

// headers

class Renderer {
private:
    ID3D11Device* device;
    ConstantBuffer cbuffer;
public:
    bool init();
}
 
class ConstantBuffer {
public:
    ConstantBuffer();
    bool init(ID3D11Device* device);
}

// cpps
void Renderer::init() {
 // inialize ID3D11Device
 // ...
 cbuffer.init( device );
 // ...
}

void ConstantBuffer::init() {
  // ...
}
 

This gives you some flexibility when dealing with errors, for example. The only way a constructor can notify errors is by throwing exceptions, but you may not want to use them (either now or a in a future port). Having a init method let you fail via exceptions or via returning error values.

 

Having a separate init method also let you re-initialization, which you may want in the future.

 

This design pattern, by the way, is called Two-Phase Construction: http://developer.nokia.com/community/wiki/Two-phase_construction.

Share this post


Link to post
Share on other sites
Hodgman    51234

The only way a constructor can notify errors is by throwing exceptions, but you may not want to use them (either now or a in a future port). Having a init method let you fail via exceptions or via returning error values.

You can return an error value from a constructor through an out-parameter.
 
The catch with two-phase construction is that it requires your objects to have a "zombie state", where they're constructed but not yet initialized. For some classes that's easy, like an empty std::vector, or a null cbuffer, but for other classes it's sometimes ugly. The client also needs to be aware of this design (knowing that it's possible for a valid cbuffer pointer to internally be null), so they probably need an IsInitialized() function, etc.

Zombie states are also a 3rd option for reporting errors out of a constructor - you require the user to call IsInitialized() immediately afterwards.

Personally, I just like to make it so constructors cannot fail, so I have a CreateDevice function which makes an ID3D11Device, and then constructs my own device class, passing in the D3D object as a parameter.

Share this post


Link to post
Share on other sites
jmpep    375


You can return an error value from a constructor through an out-parameter.

 

Emmm, yep, I completely forgot about that one. The problem is that you still end up with a messed up object resulting from calling the constructor, but it is in a permanent uninitialized state. With two-phase construction at least you can go from uninitialized to initialized via the init method :)

 

Regadless, I am with you that the best constructor is one which simply cannot fail :) I like your method if encapsulating Direct3D in the API is not the point of Renderer and ConstantBuffer...

Share this post


Link to post
Share on other sites
Krohm    5030


I'd be hard pressed to say exactly why I find this easier to deal with and maintain if I'm honest but over the course of an entire system, it feels cleaner to me.
I don't know Aardvajk, I'm not quite on it. The main problem I have here is that you cannot take a Texture object and poll which device created it (maybe we can pull out the creation device from the OS handle, but in isolation, that's still not enough). This information will have to be tracked with surrounding code. It's not like I'm against this idea in itself but I always considered statements like "parameter X must be creator" a last resort. If I am allowed to specify a different object "for every method", then I'd intuitively feel entitled in calling load on a device and bind on multiple.

Share this post


Link to post
Share on other sites
Aardvajk    13207

 


I'd be hard pressed to say exactly why I find this easier to deal with and maintain if I'm honest but over the course of an entire system, it feels cleaner to me.
I don't know Aardvajk, I'm not quite on it. The main problem I have here is that you cannot take a Texture object and poll which device created it (maybe we can pull out the creation device from the OS handle, but in isolation, that's still not enough). This information will have to be tracked with surrounding code. It's not like I'm against this idea in itself but I always considered statements like "parameter X must be creator" a last resort. If I am allowed to specify a different object "for every method", then I'd intuitively feel entitled in calling load on a device and bind on multiple.

 

 

That's a very good point. Its hidden in this example by the fact that I only have the one GraphicsDevice, but thinking about this in a different context where multiple "creators" of some kind of "resource" can exist, it would make much more sense to bind the creator to the resource I suppose.

 

I suppose I just dislike the idea of pointers to objects floating about inside class implementations when I can just supply the object when it is needed, but your point is well made and fair.

Share this post


Link to post
Share on other sites
Hodgman    51234

Both of these:

//#1
class Texture {
  Device* device;
public:
  Texture(Device* device);
  void Bind(int slot);
}
//#2
class Texture {
public:
  Texture(Device* device);
  void Bind(Device* device, int slot);
}

And my personal choice:

//#3
class Texture {
  friend class Device;
  Texture();//n.b. private
}
class Device {
public:
  Texture* CreateTexture();
  void Bind(Texture*, int slot);
}

...all suffer from one of two variations of that same issue.

With #2 and #3, you can create a texture using one device, and then try and bind it to another, which won't work.

Device a, b;
//#2
Texture t(a);
t.Bind(b, 0);//error
//#3
Texture* t = a.CreateTexture();
b.Bind(t, 0);//error

With #1, you can have client code that looks correct though, but is ultimately flawed:

void DrawThing( Texture& diffuse, Texture& normals )
{
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//were those textures actually bound to this device, or to another one?? Who knows!
}

The client code for option #1 seems too much like "spooky action at a distance" to me, so I avoid it as well.
There's no way to automatically audit your code in option #1 to catch these kinds of errors.

To solve this issue, I'd give the texture object a pointer back to the device in debug mode only, because it's not normally required, which you can then use to catch these kinds of mistakes at runtime.

#ifdef _DEBUG
# define myDEBUG(a) a
#else
# define myDEBUG(a) 
#endif
class Texture {
friend class Device;
  Texture( Device* d ) myDEBUG(:dbg_Owner(d)) {}
  myDEBUG( Device* dbg_Owner );
}
void Device::Bind( Texture* t, int slot )
{
  myASSERT( t->dbg_Owner == this ); // if created in a different device, let the programmer know their code is broken
}
Edited by Hodgman

Share this post


Link to post
Share on other sites
Lactose    11446
//#2
class Texture {
public:
  Texture(Device* device);
  void Bind(Device* device, int slot);
}

With #2, you can have client code that looks correct though, but is ultimately flawed:

void DrawThing( Texture& diffuse, Texture& normals )
{
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//were those textures actually bound to this device, or to another one?? Who knows!
}

The client code for option #2 seems too much like "spooky action at a distance" to me, so I avoid it as well.
There's no way to automatically audit your code in option #2 to catch these kinds of errors.

Unless I'm missing something (which I might very well be!), this wouldn't compile.

Given the function definition, it should be

diffuse.Bind(&device, 0); and normals.Bind(&device, 1);

Or...?

Share this post


Link to post
Share on other sites
Aardvajk    13207

//#2
class Texture {
public:
  Texture(Device* device);
  void Bind(Device* device, int slot);
}
With #2, you can have client code that looks correct though, but is ultimately flawed:
void DrawThing( Texture& diffuse, Texture& normals )
{
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//were those textures actually bound to this device, or to another one?? Who knows!
}
The client code for option #2 seems too much like "spooky action at a distance" to me, so I avoid it as well.
There's no way to automatically audit your code in option #2 to catch these kinds of errors.
Unless I'm missing something (which I might very well be!), this wouldn't compile.
Given the function definition, it should be
diffuse.Bind(&device, 0); and normals.Bind(&device, 1);
Or...?

It's just pseudo code. Doesn't need to compile to illustrate the point Hodge was making.

Share this post


Link to post
Share on other sites
Lactose    11446

 

It's just pseudo code. Doesn't need to compile to illustrate the point Hodge was making.

 

True, but the comment indicates that you can't know that the device you're drawing with is the same device as the textures were bound to.

With the device explicitly used in the Bind(), you'd be able to see it's the same, no? That said, I'm not at all familiar with automatic audits, which Hodge refers to directly after.

---

EDIT: Although I guess the problem is apparent in more complex situations, where the bind doesn't necessarily occur in the same place as the draw. I can see the beast rear its head in that case.

Edited by Lactose!

Share this post


Link to post
Share on other sites
Hodgman    51234

I numbered the examples wrong. Replace "#2" with "#1" and vice versa... [edit] I've edited the post now to swap those numbers around [/edit]

#1's Bind function doesn't take the device as a parameter, instead getting it from a member. This means the user doesn't know which device it's being bound to (as indicated by the comment).

[edit] I accidentally down-voted Aardvajk below when scrolling with fat thumbs on a tiny phone... Someone care to countervote that for me? smile.png

Edited by Hodgman

Share this post


Link to post
Share on other sites
Aardvajk    13207

To be fair though, I'd expect this imaginary Device to return some kind of error if we attempt to bind a resource to it that it didn't create, and we can catch this error and respond accordingly. This is only really necessary in the case that an untrappable error condition would occur as a result of binding to the wrong device, which is difficult to imagine in any real world system.

 

But an interesting thread and food for thought.

Share this post


Link to post
Share on other sites
Batzer    592

Wow thank you guys for all the answers smile.png

I went with Hodgman's solution (with CreateDevice) for my Renderer class, but I'll keep the other solutions in mind. Great stuff.

@Hodgman: I countervoted wink.png

Edited by Batzer

Share this post


Link to post
Share on other sites
Aardvajk    13207

I numbered the examples wrong. Replace "#2" with "#1" and vice versa... [edit] I've edited the post now to swap those numbers around [/edit]

#1's Bind function doesn't take the device as a parameter, instead getting it from a member. This means the user doesn't know which device it's being bound to (as indicated by the comment).

[edit] I accidentally down-voted Aardvajk below when scrolling with fat thumbs on a tiny phone... Someone care to countervote that for me? smile.png

 

Heh, I wondered why I got a message about a 0 vote on that. No probs, can spare a few points among friends smile.png

 


@Hodgman: I countervoted

 

Why thank you, sir.

Edited by Aardvajk

Share this post


Link to post
Share on other sites
Waterlimon    4398

I support the idea of ASSUMING that data from device A wont be used with device B at runtime, and doing checks but only in debug mode.

 

This is quaranteed to give the optimal runtime performance since it is basically the same as compile time checks, except we have to do it manually because the compiler is dumb. For the same reason I disagree containing a pointer to the device within the object UNLESS actually required, in which case the pointer still shouldnt go INSIDE the object but rather be "bundled" with the object in places where this data is needed. eg.  Texture and std::pair<Texture,Device*> respectively.

 

So, Texture should not contain a Device*, but you may still bundle a Device* with the texture in places where this is required.

Share this post


Link to post
Share on other sites
dmatter    4826

...  #1, #2 and #3 ...

 

Interesting stuff.

My take on those 3 designs...

 

#1 could be viewed as a closure/partial-application of the device into the Bind() call, in a sense the Texture class is just a closure type. The DrawThing function should know that if it accepts closure types as its arguments then it doesn't get to dictate the closed environment. Either it should not have accepted closure types or it needs a way to assert a pre-condition that the two textures refer to the same device.

 

#2 makes me feel much more uncomfortable that #3, but I would have a hard time saying exactly why. Maybe it's just a matter of convention. Like Krohm, I would feel somewhat entitled to construct with one device and bind to another - until reading the documentation.

 

#3 technically suffers the same problem as #2 but follows a more familiar convention so I think it really is better than #2. It also exposes an underlying truth that the device acts like a factory for textures, whereas I think #2 kind of gives the impression that a device is merely a component of a texture.

 

 

Designs #2 and #3 try to pretend that a texture is a stand-alone object and that it can be separated from the device at one point (at creation) and re-join with it later on (at bind-time). This is almost true and close-enough that it can be implemented that way provided there are sufficient runtime checks at the re-join point.

 

Design #1 never lets the device separate from the texture, so the only thing it does is try to present a texture as a stand-alone object.

 

Textures being stand-alone raises the question of whether a texture might be able to out-live the device.

#2 and #3 don't really deal with that too well - the destruction of the device object may put the texture objects into a weird state (another example of spooky action at a distance).

Design #1 could deal with that if it used a shared_ptr to reference the device which guarantees that the lifetime of the device is at least as long as the texture's.

 

 

I usually think all of this comes down to limitations in the language itself, that you have to enforce this kind of thing yourself and there aren't great ways to describe the relationships to the compiler or to the runtime environment.

Other areas of the language have the same problem, for example you wouldn't expect to be able to use an iterator in a different container to the one that created it, or to use an iterator after the container has been destructed.

 

Maybe using handles (like integer handles) rather than objects would side-step some of those issues.

Share this post


Link to post
Share on other sites
Hodgman    51234

#1 could be viewed as a closure/partial-application of the device into the Bind() call, in a sense the Texture class is just a closure type. The DrawThing function should know that if it accepts closure types as its arguments then it doesn't get to dictate the closed environment. Either it should not have accepted closure types or it needs a way to assert a pre-condition that the two textures refer to the same device.

Yeah, with #1 you could add in error checking such as below... However, I'd prefer the error checking example that I made for #3, because that was internal to the system, as opposed to being the client's responsibility.

void DrawThing( Texture& diffuse, Texture& normals )
{
  myASSERT( diffuse.Device() == &device );
  myASSERT( normals.Device() == &device );
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//The textures were bound to this device, otherwise we would've crashed above
}

Textures being stand-alone raises the question of whether a texture might be able to out-live the device.

If you're using a managed language, then the texture would probably have a reference to the device, so that the device can't be garbage collected while someone's still using it's texture.
If you're using a C-style resource management, then you should be used to dealing with specifications/rules for object lifetimes. It's a common theme that poor lifetime management will lead to bugs with horrible symptoms, so you get used to this kind of planning and engineering pretty quickly wink.png
If you're using "modern C++", then you could either go the "managed" style route above, where a texture contains a (strong) reference to the device, so the device cannot be deleted while someone still holds onto it's resources... or you could instead only hand out weak-references of resources to the client, so that when the device is destroyed, all the user's references are invalidated automagically.
 
Personally, I find the C-style of resource management to actually be superior to the other two, but many would disagree with me tongue.png
Raw pointers are very common in my engine, but don't pose problem due to well defined lifetime rules and good resource allocation practices.
There's a lot of cases where if you hung onto a pointer to some object/resource after it's actual owner had expired, you would get horrible bugs, but that doesn't happen by design.

Maybe using handles (like integer handles) rather than objects would side-step some of those issues.

I actually do this. I don't have a texture/buffer/etc struct for the client to use. I just make new integer types to represent them:

template<class T,class Name>struct PrimitiveWrap
{
	PrimitiveWrap() : value() {}
	explicit PrimitiveWrap( T v ) : value(v) {}
	operator const T&() const { return value; }
	operator       T&()       { return value; }
private:T value;
};
#define TYPEDEF_ID( name )				\
	struct tag_##name;				\
	typedef PrimitiveWrap<u32,tag_##name> name;	//
...
TYPEDEF_ID( TextureId );
...
TextureId tex = device.CreateTexture(...);

If you wanted to assert which device owned which resource, you could pack the device ID into some of the bits of that integer, along with the actual resource index.

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