Hello,
Im doing a small game engine in c++ for fun, and im stuck with a design problemt.
Here is a simplified sketch of part of the design:
class ITexture
{
...
virtual void lock() = 0
virtual release() = 0;
...
}
class IVideoDevice
{
public:
...
virtual void setTexture(ITexture *tex) = 0;
virtual ITexture *getTexture() const = 0;
...
};
Supose i have a direct3d implementation of this interface:
class D3DTexture : public ITexture
{
public:
void lock()
{
impl->lock();
}
void release()
{
impl->release();
}
IDirect3DTexture9 *get_impl()
{
return impl;
}
private:
IDirect3DTexture9 *impl; // Direct3d COM interface
}
class Direct3DDevice : public IVideoDevice
{
public:
...
void setTexture(ITexture *tex)
{
D3DTexture d3d_tex = dynamic_cast<D3DTexture>(tex);
if ( d3d_tex )
device->setTexture(d3d_tex->getImpl()); // Direct3D COM method
else
throw Exception("Wrong texture type for this device");
}
...
private:
IDirect3DDevice9 *device;
}
In the Direct3DDevice class i have to do a dynamic_cast to get the Direct3DTexture. The signature of the method is somewaht wrong because textures, shaders ,vertex buffers, etc are specific for a certain kind of device. I shouldnt be able to to use a GLTexture in a D3DDevice, but this design only detects the problem at run time and throws a exception. And the same kind of cast will be necessary for other device dependent objects.
Is there a better way to handle this? People say dynamic casts are usually the result of poor design, but i cant find a better way to do this. I dont have much experience in OO design. Any help will be appreciated. Thx in advance.
Design problem
It looks like you're on the right track to a decent OO design, actually, so props on that despite not having much experience. What you're dealing with here is actually a pretty common problem when you have 'parallel hierarchies' (not an official term, but it should make sense) of interface implementations.
The proper solution varies depending on the problem. Your casting actually isn't that bad in this situation, but I'll offer another solution. Perhaps someone else can improve further.
At some spot, you must have a place where D3DTexture is created specifically. At this point, store a reference back up to the D3DDevice inside the D3DTexture. After you're done with this, push the behavior that needs to say 'device->setTexture' into the ITexture interface rather than the IVideoDevice interface. D3DTexture will be able to do this without casting since it has a reference to a concrete D3DDevice--similarly, your GLTexture would probably reference a GLDevice and could make the appropriate call.
You don't need to set it up exactly like this, but hopefully this points you towards a solution. At some point you're constructing specific instances of D3D this and D3D that, or GL this and GL that. If those types must link with eachother and only eachother, you should establish that link during that construction time.
The proper solution varies depending on the problem. Your casting actually isn't that bad in this situation, but I'll offer another solution. Perhaps someone else can improve further.
At some spot, you must have a place where D3DTexture is created specifically. At this point, store a reference back up to the D3DDevice inside the D3DTexture. After you're done with this, push the behavior that needs to say 'device->setTexture' into the ITexture interface rather than the IVideoDevice interface. D3DTexture will be able to do this without casting since it has a reference to a concrete D3DDevice--similarly, your GLTexture would probably reference a GLDevice and could make the appropriate call.
You don't need to set it up exactly like this, but hopefully this points you towards a solution. At some point you're constructing specific instances of D3D this and D3D that, or GL this and GL that. If those types must link with eachother and only eachother, you should establish that link during that construction time.
So there are two problems here, one is that you're losing your type information and then using dynamic_cast to try and reclaim it at run-time. The other is that your dependencies are implicit and you rely on clients of the renderer to keep track of which ITextures came from which IVideoDevices - even if they do manage to correctly only supply D3DTextures there's still no guarantee that the texture actually belongs to that particular Direct3DDevice (and it is reasonable to have multiple d3d devices).
As far as code smells go, you quite rightly picked up on the dynamic_cast smell; the other is an encapsulation smell in the form of the get_impl() function which exposes the private implementation.
In an OO world each object should be responsible for itself, mix that in with the practice of making dependencies explicit and we arrive at a slightly different design (edit: and one which TheOrzo arrived at also [smile]):
D3DDevice provides the underlying service, thinly wrapping the actual direct3d device - let's say for the purpose of handling errors, sanitising the interface and providing RAII. It doesn't have an interface that would be applicable to any other API so it doesn't implement an abstract base class.
Now you can, if you want, wrap texture creation up into a factory, then specialise that on a per-api basis:
Now you can write API-agnostic functions that are able to create, map, fill, unmap and finally bind textures to the pipeline:
Now, hopefully you wouldn't use raw pointers for all these things, shared_ptr's are much more appropriate here and will help to ensure constraints such as releasing all video resources (e.g textures) before releasing the device.
Hope that helps.
As far as code smells go, you quite rightly picked up on the dynamic_cast smell; the other is an encapsulation smell in the form of the get_impl() function which exposes the private implementation.
In an OO world each object should be responsible for itself, mix that in with the practice of making dependencies explicit and we arrive at a slightly different design (edit: and one which TheOrzo arrived at also [smile]):
class D3DTexture : public ITexture{public: D3DTexture(D3DDevice * device) : device(device) , impl(device->createTexture()) { } void * lock() { return impl->lock(); } void unlock() { impl->unlock(); } void bind() { device->setTexture(impl); }private: // don't really use raw pointers, see shared_ptr!! D3DDevice * device; IDirect3DTexture9 * impl;};D3DDevice provides the underlying service, thinly wrapping the actual direct3d device - let's say for the purpose of handling errors, sanitising the interface and providing RAII. It doesn't have an interface that would be applicable to any other API so it doesn't implement an abstract base class.
class D3DDevice{public: IDirect3DTexture9 * createTexture(); void setTexture(IDirect3DTexture9 * texture);};Now you can, if you want, wrap texture creation up into a factory, then specialise that on a per-api basis:
class ITextureFactory{public: virtual ITexture * create();};class D3DTextureFactory : public ITextureFactory{public: D3DTextureFactory(D3DDevice * device) : device(device) { } ITexture * create() { return new D3DTexture(device); }private: D3DDevice * device;};Now you can write API-agnostic functions that are able to create, map, fill, unmap and finally bind textures to the pipeline:
void foo(ITextureFactory & textureFactory) { ITexture texture = textureFactory.create(); void * buffer = texture.lock(); fill( buffer ); texture.unlock(); texture.bind();}Now, hopefully you wouldn't use raw pointers for all these things, shared_ptr's are much more appropriate here and will help to ensure constraints such as releasing all video resources (e.g textures) before releasing the device.
Hope that helps.
Thanks to the OP, dmatter and TheOrzo for the well-frased question and answers in this post. Added to my favorites!
Author
Thank you TheOrzo and dmatter for your help. You guys really solved my problem [smile], exactly what i was looking for.
Best Wishes.
Best Wishes.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
