diamond problem with interfaces and down-casting

You've designed your interfaces poorly.

An interface should not be coupled to an implementation. Moreover, it should not be necessary to know the concrete implementation of anything to use its interface.

Less abstractly: I should be able to do anything rendering-wise that I need to with your independent layer alone, without ever referencing a specific graphics API.


You only really need two things: an interface representing the API-independent functionality of each type of object, and an implementation of that interface for a given graphics API. If you want the internal API-specific code to be able to know things that are "off limits" to the rest of the world, add functions to the internal implementation's class.

Don't be too hasty to represent everything via implementation inheritance. It's almost always the wrong tool for the job. If you're using interface inheritance, you can solve all your problems with two classes and you're all done. Don't forget that composition is also a good option.

I think I probably didn't explain my problem well enough. I agree with what you're saying, however this is not the problem. The clients using this wrapper will only interact with the interfaces without ever knowing about the implementation.

If you want the internal API-specific code to be able to know things that are "off limits" to the rest of the world, add functions to the internal implementation's class.

This is the problem. Imagine this piece of code on the client side:

ITexture* myTexture = ...;

myAbstractedDevice->SetTexture( 0, myTexture );

And this is the implementation:

class ITexture : public IResource
{
   ...
};

class oglTexture : public ITexture, public oglResource
{
   ...
};

void oglDevice::SetTexture( unsigned slot, ITexture* texture )
{
   assert( texture );
   assert( texture->GetServer() == this->GetServer() );

   oglTexture* ogl_texture = static_cast<oglTexture*>(texture);
   ...
}

If oglTexture only inherited from ITexture, this would work perfectly, but then I'd have to re-implement the other interfaces (IResource, IDeviceChild, ...) with mostly duplicated code, which makes me think it's not the right way to solve this.

Can you show some examples of what these interfaces actually contain? As it stands I see absolutely no justification for using inheritance at all, let alone multiple inheritance. You're also still upcasting, which is a telltale sign that you're too fixated on inheritance as an organizational method, where it doesn't belong at all.

If oglDevice::SetTexture() can't do everything it needs to do with an ITexture by calling methods that live on ITexture, you've designed a poor interface.

Actually the current workaround I have found is to have a void* ITexture::GetPrivateDeviceData() which doesn't feel very clean but works for now. I'm still searching for a better way.

Can you show some examples of what these interfaces actually contain?

oglTexture:oglTexture( oglGraphicsContext& ctx, const TextureDescription& desc )
{
   glGenTextures( 1, &_gl_texture ) // _gl_texture is a private/protected member variable
   ... // bind and create texture with content from "desc"
}

GLuint oglTexture::GetGLTexture() const // this is the method I wish oglGraphicsContext to see
{
   return _gl_texture;
}


bool oglGraphicsContext::SetTexture( unsigned slot, ITexture* texture )
{
   // check that texure is a texture created by this graphics context
   if( texture->GetDeviceCtx() != this )
      return false;

   auto ogl_texture = static_cast<oglTexture*>(texture); // this is the cast that's needed and won't work with virtual inheritance

   GLuint native_texture = ogl_texture->GetGLTexture();
   ... // use native_texture for whatever...
}

which is a telltale sign that you're too fixated on inheritance as an organizational method, where it doesn't belong at all.

I understand and I've been thinking myself that maybe I'm not choosing the best approach, but can't really think of anything as "clean"/"elegant" as this, which is why I posted here

If oglDevice::SetTexture() can't do everything it needs to do with an ITexture by calling methods that live on ITexture, you've designed a poor interface.

If I implement the ITexture::GetPrivateDeviceData() method as I mentioned above, I can do this, but I still feel this shouldn't really be a visible method. Plus having virtual inheritance on the interfaces will bring more problems to the end client trying to cast for example from IResource to ITexture... This could again be worked around by having a IDeviceChild::CastTo( Type ) but then it will have a performance cost and ugly feel...

You're doing it backwards.


Objects should be thought of like little hives of activity, with some associated state. If you want something to happen, you don't grab the guts of the object and smack them around until they do what you want. Instead, you instruct the object to do what you want, and give it any required data that it doesn't already have inside it.


For your example, instead of a oglDevice reaching into the private details of an ITexture in order to send it to the OpenGL API, add a method called MakeActive to ITexture.

Liskov Substitution principle is kind of important. You have a function:

bool oglGraphicsContext::SetTexture( unsigned slot, ITexture* texture )

What happens if you give it a D3DTexture? By definition, that should be totally fine. However, in the context of the problem you're trying to solve, it inherently isn't totally fine. You can try to fight this, but fundamentally, it's going to fight you back every step of the way, and you'll end up with an API even you don't like. If a function is only valid with a particular subclass, the interface needs to require that subclass.

The problem with your API is that you're not actually trying to abstract ogl and d3d into a single interface -- you're trying to factor out junk that both APIs use. In the end, you're left with two broken APIs that look superficially similar to eachother.

To actually create a single API, you absolutely cannot do this double-dipping inheritance thing. Something has to be uniform, whether it's the texture or device context depends on how you do it.

For your example, instead of a oglDevice reaching into the private details of an ITexture in order to send it to the OpenGL API, add a method called MakeActive to ITexture.

This would certainly solve some problems, such as not needing to cast anymore. However, the way I see it, the Texture object would be modifying the owner's (oglGraphicsContext) state which doen't make much sense. I guess this is also the reason the D3D API goes this way instead of backwards.

[Edit] It would also make it impossible to bind more than just one object in a single call.

What happens if you give it a D3DTexture? By definition, that should be totally fine.

I disagree. The fact that SetTexture contains an ITexture doesn't automatically mean it will be OK to pass any ITexture. I can certainly write in the documentation that the ITexture must be created by the same IGraphicsContext and be in a specific state.

You can try to fight this, but fundamentally, it's going to fight you back every step of the way, and you'll end up with an API even you don't like.

I think I would like the API but not the fact that I would be double checking if the objects are valid everywhere.

I am trying to adopt concepts of future graphics APIs such as PSOs which aliviate a lot of this by only checking at PSOs creation time. However some state just doesn't belong in a PSO...

I am inspiring my class design on D3D11's, which seems to do things exacly like I'm trying to implement, but they somehow manage to not need virtual inheritance.

I think I might be worrying too much about correctness and should think more about ease of use (adopt ::MakeActive() and ignore the fact that I'm modifying state that doesn't belong to this child object), but I'm afraid that by doing so I might be commiting suicide in the long run...

This still doesn't solve being unable to static_cast from an IResource to an ITexture for example. How does D3D do it? The only way I see it working is to avoid shared implementations and only implement things in the final class, which can be a maintenance nightmare...

This would certainly solve some problems, such as not needing to cast anymore. However, the way I see it, the Texture object would be modifying the owner's (oglGraphicsContext) state which doen't make much sense. I guess this is also the reason the D3D API goes this way instead of backwards.

[Edit] It would also make it impossible to bind more than just one object in a single call.

Lies. Pass a container of ITexture* pointers.

I disagree. The fact that SetTexture contains an ITexture doesn't automatically mean it will be OK to pass any ITexture. I can certainly write in the documentation that the ITexture must be created by the same IGraphicsContext and be in a specific state.

Your opinion is relatively unimportant. People who have spent their careers studying software architecture - and won very prestigious awards because of it - created the principles we're talking about. They aren't just subjective hooplah, they are very important for creating good APIs. The Liskov Substitution Principle in question here is not exactly something you are qualified to argue with.

Nothing personal, man, but if you're asking entry level API design questions in a programming forum, you're not ready to debate the validity of LSP in your designs.

I think I would like the API but not the fact that I would be double checking if the objects are valid everywhere.

What are you talking about? If you design your code correctly, the compiler will check for your correctness as much as possible, and you don't need to dick around with redundant validity checks. Your software becomes correct by design, not by "I'll put it in the documentation" or "I'll check it every three lines" or some similar rubbish.

I am inspiring my class design on D3D11's, which seems to do things exacly like I'm trying to implement, but they somehow manage to not need virtual inheritance.

No, dude. They aren't doing anything like what you're doing. D3D11 is not trying to implement OpenGL support. This should be blindingly obvious. Therefore they are solving a different problem and their solution is not automatically something you should try to adopt for your (vastly different) situation.

I think I might be worrying too much about correctness and should think more about ease of use (adopt ::MakeActive() and ignore the fact that I'm modifying state that doesn't belong to this child object), but I'm afraid that by doing so I might be commiting suicide in the long run...

You're not worrying about correctness at all. You're worrying about superstition (your "suicide" worries) and misunderstandings (your completely left-field belief that objects should not modify each other). If ITexture::MakeActive() needs to do something with a FooObject, just call the appropriate method on the FooObject. Done.

This still doesn't solve being unable to static_cast from an IResource to an ITexture for example. How does D3D do it? The only way I see it working is to avoid shared implementations and only implement things in the final class, which can be a maintenance nightmare...

static_cast from a base to a derived class is totally trivial, provided you're not screwing with virtual inheritance. It's still ugly and not cool, but it isn't exactly the bundle of snakes that you've uncovered in your approach.

I don't think we're understanding each other at all. Maybe the fact that english is not my native language is somehow impeding me to explain my problem properly.

Lies. Pass a container of ITexture* pointers.

Where to exacly should I pass an array/container of ITexture pointers if you're telling me I should use ITexture::MakeActive() ?

And doesn't that mean again that this container could have ITexture pointers created from another context?

Your opinion is relatively unimportant. People who have spent their careers studying software architecture - and won very prestigious awards because of it - created the principles we're talking about. They aren't just subjective hooplah, they are very important for creating good APIs. The Liskov Substitution Principle in question here is not exactly something you are qualified to argue with.

Nothing personal, man, but if you're asking entry level API design questions in a programming forum, you're not ready to debate the validity of LSP in your designs.

I'm not really questioning the validity of LSP. I just don't see a way to fully comply with it in this particular problem.

I must add constraints to what is accepted. We don't live in a perfect world, there are exceptions I want to deal with appropriately. Say I give it a texture that is resident in another GPUs memory; I want to return an error from that (instead of invalid rendering or crash), even if in theory it should work, I could document that behaviour.

The *::MakeActive approach would solve this though, but I would like to be able to MakeActive several textures in one call.

What are you talking about? If you design your code correctly, the compiler will check for your correctness as much as possible, and you don't need to dick around with redundant validity checks. Your software becomes correct by design, not by "I'll put it in the documentation" or "I'll check it every three lines" or some similar rubbish.

Again, you misunderstood me. I mean the correctness of the internal state of the objects. I'm not talking about language correctness. In this specific case, by correctness I mean "the texture must belong to the same context you're trying to bind it to for rendering".

Curious question: how would D3D behave if it were given, say a shader, from another device?

No, dude. They aren't doing anything like what you're doing. D3D11 is not trying to implement OpenGL support. This should be blindingly obvious. Therefore they are solving a different problem and their solution is not automatically something you should try to adopt for your (vastly different) situation.

Well the OpenGL or not problem is not relevant at all to the question I'm asking. They still have interface inheritance to which they must provide implementations somewhere. When I said "how does D3D do it", I meant do they duplicate code for each object that needs to provide an implementation to ID3D11Resource or solve this problem any other way? They still must cast it to an implementation pointer somewhere...

If ITexture::MakeActive() needs to do something with a FooObject, just call the appropriate method on the FooObject. Done.

This makes sense and I had already thought of it. However then ITexture::MakeActive() would be nothing but a bridge to the oglGraphicsContext's method. Which I don't have a problem with except if there's a better solution. Also, again this would mean a call to MakeActive() for each texture, and also not the way D3D does it.

static_cast from a base to a derived class is totally trivial, provided you're not screwing with virtual inheritance. It's still ugly and not cool, but it isn't exactly the bundle of snakes that you've uncovered in your approach.

Exacly. So do you agree (excluding personal preference) with me that oglGraphicsContext could accept an ITexture and down-cast it there?

Maybe I should try to put my main question in another way: How can I implement an interface hierarchy and reuse implementation code (without ugly "hacks" like macros) while avoiding multiple inheritance?

Where to exacly should I pass an array/container of ITexture pointers if you're telling me I should use ITexture::MakeActive() ?
And doesn't that mean again that this container could have ITexture pointers created from another context?

It doesn't really matter; you just have a function in a reasonable place where the container is enumerated, and each ITexture's MakeActive is called in sequence. If you want to batch this to API calls behind the scenes, that's something the concrete implementation of ITexture can handle; it shouldn't leak into your abstracted API.

I'm not really questioning the validity of LSP. I just don't see a way to fully comply with it in this particular problem.
I must add constraints to what is accepted. We don't live in a perfect world, there are exceptions I want to deal with appropriately. Say I give it a texture that is resident in another GPUs memory; I want to return an error from that (instead of invalid rendering or crash), even if in theory it should work, I could document that behaviour.

There's nothing preventing you from doing these checks; I really don't understand what you consider problematic here. That logic just needs to live in the implementation and not leak into the interface.

As a complete straw-man example, you could have the concrete implementation of a texture check if it belongs to the correct context, and throw an exception if it does not. Or use error code returns, like D3D does; it really makes very little difference.

The *::MakeActive approach would solve this though, but I would like to be able to MakeActive several textures in one call.

As above: you write a wrapper that accepts a container of ITexture* and calls MakeActive on each. If you want to batch texture calls to your back-end graphics API then that should be hidden from the consumer and done automatically by the concrete implementation of your API interfaces. There's no reason why your API should force the user to think about batching if the implementation can do it transparently.

Again, you misunderstood me. I mean the correctness of the internal state of the objects. I'm not talking about language correctness. In this specific case, by correctness I mean "the texture must belong to the same context you're trying to bind it to for rendering".

Again, I don't see how this is difficult. You just write the check in the appropriate place in the concrete implementation. There's no need for multiple inheritance or any other such cruft.

Well the OpenGL or not problem is not relevant at all to the question I'm asking. They still have interface inheritance to which they must provide implementations somewhere. When I said "how does D3D do it", I meant do they duplicate code for each object that needs to provide an implementation to ID3D11Resource or solve this problem any other way? They still must cast it to an implementation pointer somewhere...

I don't know why you're so fixated on the idea of casting to an implementation pointer. If your interface class is designed correctly, it's almost never necessary, and indeed if it is necessary to program to a specific implementation type, you've borked your design, period.

Exacly. So do you agree (excluding personal preference) with me that oglGraphicsContext could accept an ITexture and down-cast it there?

Absolutely not. You should not be up casting anything.

Maybe I should try to put my main question in another way: How can I implement an interface hierarchy and reuse implementation code (without ugly "hacks" like macros) while avoiding multiple inheritance?

Why do you need a hierarchy?

The idea behind a multibind-in-one-call entry point is to closely mirror functionality provided by both OpenGL and Direct3D.

Your idea of calling MakeActive on several textures and transparently batch it to the backend API, while attractive and certainly more correct in "human reasoning" terms will introduce new complexity (such as tracking the state to know when to actually bind the textures on the backend API etc) which is a step away from the very thin wrapper I'm looking for. Plus, if both OpenGL and D3D already provide entry points with that same exact functionality, why should I create a higher level of managing?

We can continue to discuss this (happy to do so if it will teach me something), but to be honest I came here looking to solve the interface hierarchy + implementation hierarchy problem.

I don't know why you're so fixated on the idea of casting to an implementation pointer. If your interface class is designed correctly, it's almost never necessary, and indeed if it is necessary to program to a specific implementation type, you've borked your design, period.

First of all, let me tell you I completely agree with you.

However, I don't think it matters much in this specific case. Users are unlikely to expect an OpenGL resource to successfuly bind to a Direct3D one. My oglGraphicsContext implementation doesn't need to know the exact implementation type, it only needs to check for compatibility (example: to check if the final implementation implements GetGLHandle()).

I'm not fixated on the idea of casting to an implementation pointer, and I actually hate that I see no other option than doing so in order to be able to do things like multi-bind. D3D does this also, they don't have an ID3DTexture2D::MakeActive method. Surely the device checks for compatibility with the object you throw at it; probably through QueryInterface or similar.

Absolutely not. You should not be up casting anything.

I was confused myself when I first created this topic. Downcasting is what I mean.

Why do you need a hierarchy?

Because a Resource IS-A GraphicsChild, and a Texture IS-A Resource and so on.

If you're asking why I need the implementation hierarchy; well I don't need it, I just want to share implementation code. Say I have 10 types of device resources; doesn't make much sense to re-implement IResource methods manually in each of the derived types, no?

For sure getting rid of the implementation hierarchy would solve all my problems, but if I have to do that (and there's no other, better way) I begin to wonder if C++ isn't missing something...

C++ is missing a lot of things, but that's a different discussion :-)

Do you really have code that is shared between your different device resources? I have trouble imagining what a Texture implementation would share with a Mesh or a Shader or whatnot.

I've been following this topic for a while because I've come up against the same problem mdias has in the past. In almost the exact same context, too.

What was confusing to me is that Direct3D is full of interface classes that inherit from one another, but I don't see how any of these interfaces can be implemented without creating the diamond problem mdias has.

It may be pointless to speculate, but let's try anyway with a simple Direct3D example: there are two classes, ID3D11Texture2D and ID3D11Buffer. Both of these classes inherit from ID3D11Resource. So, how do you think Direct3D implements each of these objects?

One might imagine classes ConcreteD3D11Texture2D and ConcreteD3D11Buffer both inherit from ConcreteD3D11Resource, and ConcreteD3D11Resource implements the ID3D11Resource interface, but this creates the exact diamond problem mdias originally posted about. So maybe instead ConcreteD3D11Texture2D and ConcreteD3D11Buffer both contain a ConcreteD3D11Resource, and they manually implement the ID3D11Resource functions with simple passthrough functions to the concrete resource they contain?

class ID3D11Resource {
public:
    virtual int GetType() const = 0;
};

class ID3D11Texture2D : public ID3D11Resource {
public:
    virtual void GetDesc(D3D11_TEXTURE2D_DESC *pOut) = 0;
};

class ID3D11Buffer : public ID3D11Resource {
public:
    virtual void GetDesc(D3D11_BUFFER_DESC *pOut) = 0;
};

// one potential implementation

class ConcreteD3D11Resource : public ID3D11Resource {
};

class ConcreteD3D11Texture2D : public ID3D11Texture2D {
    ConcreteD3D11Resource mRes;
public:
    virtual int GetType() const { return mRes.GetType(); }
    virtual void GetDesc(etc) { etc; }
};

// same thing for ConcreteD3D11Buffer 

It's easy for me to look at Direct3D and say "this is a well designed interface" but it's very hard to see how one might implement an interface similar to Direct3D's while abiding by the rules you've laid out above (which all sound like good, reasonable rules!)

Maybe part of the problem is upholding D3D as an example of good API design.

The LSP as mentioned above is a core concept of OO itself, and the fact that the design violates it is a huge hint that something is wrong.

The way I solve this is that I choose between D3D/GL/etc at compile time. There's no situation where I need to be half way through running a D3D game and suddenly switch over to using GL...
So, now the problem isn't about using OOP interfaces, it's just about implementation hiding techniques, such as PIMPL.

OOP (without virtual inheritance):

struct IBase           { virtual void A(); virtual ~IBase()=0; };
struct IMiddle : IBase { virtual void B(); virtual ~IMiddle()=0; };
struct ITop : IMiddle  { virtual void C(); virtual ~ITop()=0; };

struct MyTop : ITop { void A(); void B(); void C(); };

PIMPL:

template<class T, class I> struct Pimpl
{
  Pimpl() : impl(new I) {}
  ~Pimpl();
  Pimpl(const Pimpl<T>&);
  Pimpl<T>& opeartor=(const Pimpl<T>&);
  operator I&() { return *impl; }
private:
  I* impl;
}
struct BaseImpl; struct MiddleImpl; struct TopImpl;
struct Base : private Pimpl<Base,BaseImpl>             { void A(); };
struct Middle : Base, private Pimpl<Middle,MiddleImpl> { void B(); };
struct Top : Middle, private Pimpl<Top,TopImpl>        { void C(); };

struct TopImpl { void C(); };
void Top::C() { static_cast<TopImpl&>(*this).C(); }
etc

Or my personal choice, not sure what to call it... PIMPL minus P? (AKA lying to the type system)

template<class T, class I> struct Impl
{
  T* Create() { return (T*)new I; }
  void Release(T* p) { delete (I*)p; } 
private: Impl(); ~Impl(); Impl(const Impl<T,I>&);
};
struct BaseImpl; struct MiddleImpl; struct TopImpl;
struct Base : private Impl<Base,BaseImpl>             { void A(); };
struct Middle : Base, private Impl<Middle,MiddleImpl> { void B(); };
struct Top : Middle, private Impl<Top,TopImpl>        { void C(); };

struct TopImpl : MidleImpl { void C(); };
void Top::C() { reinterpret_cast<TopImpl&>(*this).C(); }
etc

As for textures, buffers, etc... I don't actually bother with any of this though, and just have:

struct TextureId { int id; };

YMMV

Yeah, I do something similar in my own code. I'm a bit lamer in that I don't even bother with the PIMPL stuff, I just have a bunch of forward declared types (struct Texture; struct Buffer; etc) in the header files and the actual implementation is in the d3d/opengl cpp file. That means there's no inheritance and my types don't have member functions (obviously, since the header files only see the forward decl) but for a graphics API abstraction it's been perfectly sufficient, so far.

And I should add that if you want to do this determination that Hodgman mentions at runtime (say... when your program launches or through a configuration manager), you can hide everything through factories. No, it's not entirely "like D3D" at that point, but you decide the API as a consequence of the problem you're trying to solve, not the other way around.

Do you really have code that is shared between your different device resources? I have trouble imagining what a Texture implementation would share with a Mesh or a Shader or whatnot.

Not right now, I'm still in the early stages of developing this wrapper. However I can see myself adding reference couting to all objects through the base class, or adding an IResource::GetType method. Each addition of a method, or worse, change to it's behaviour would be prone to add inconsistency if the implementation doesn't share code.

Maybe part of the problem is upholding D3D as an example of good API design.

You're probably right. But I think there are valid reasons for D3D to be the way it is. Such as being more important to closely mirror the hardware/drivers capabilities than to meticulously follow OO-correctness concepts in a relatively small-scope library.

The truth is, however ugly or not their implementation is, to the user things are abstracted in a way that IMO makes sense. And this is what I'm after.

So maybe instead ConcreteD3D11Texture2D and ConcreteD3lD11Buffer both contain a ConcreteD3D11Resource, and they manually implement the ID3D11Resource functions with simple passthrough functions to the concrete resource they contain?

Yeah, that works but looks ugly. I've thought of a similar solution that would be remove virtual inheritance and:

class Concrete : public IConcrete, public Base
{
public:
   int MyBaseMethod() { return Base::MyBaseMethod(); }
}

..which would have similarities to the PIMPL idiom suggested by Hodgman, requiring an additional level of indirection. And since the base interfaces are pure interfaces with no data, it should be OK, even if ugly..

However this really sounds like we're fighting the language limitations here... Feels really weird to have workarounds like these to solve an OO problem in a "specialized-in-OOP" language.

...obviously, since the header files only see the forward decl...

How do you handle resource specific functionality, such as mapping the buffer and so on?

And I should add that if you want to do this determination that Hodgman mentions at runtime (say... when your program launches or through a configuration manager), you can hide everything through factories. No, it's not entirely "like D3D" at that point, but you decide the API as a consequence of the problem you're trying to solve, not the other way around.

This is exacly the reason why I'm after pure interfaces, othewise I'd just typedef everything on the preprocessor and live a simple life with only a few virtual methods and single inheritance :)

Not right now, I'm still in the early stages of developing this wrapper. However I can see myself adding reference couting to all objects through the base class, or adding an IResource::GetType method. Each addition of a method, or worse, change to it's behaviour would be prone to add inconsistency if the implementation doesn't share code.

Don't prematurely tie your design to poor decisions just because "you can see yourself" needing something - aka. the YAGNI principle. If you know you need it, design for it. If you don't 100% need it right now, don't worry about it.

BTW reference counting can be done easily without inheritance - see std::shared_ptr for a classic example. Intrusive versions are also trivial to design and implement using compile-time template solutions.

GetType() is a code smell.