sf::Drawable virtual inheritance

[korczurekk]

Hi,
sf::Drawable is actually just an interface and IMHO should be inherited with 'virtual' keyword.

My use case is very simple. I have class Widget, which inherits sf::Drawable and adds new method and class FpsMeter, which inherits sf::Widget AND sf::Text (It's better than having private member sf::Text as programmer can directly use sf::Text methods on my FpsMeter). However, this causes 'ambigous base' error.

class Widget
        : virtual public sf::Drawable
{
public:
        virtual void update(const WidgetEvent ev) = 0;
};
 

class FpsMeter
        : public Widget
        , public sf::Text
{
        std::list<float> times;

public:
        using sf::Text::Text;

        FpsMeter();
        virtual void update(const WidgetEvent ev);
};
 

This can be solved by private inheritance of sf::Drawable in both sf::Text and my FpsMeter.

Summary
It would be nice if all SFML classes could virtually inherit sf::Drawable, everything that has to be changed in each class is replacing ': public Drawable' with ': virtual public Drawable'.

I can apply patch myself and send pull request if you approve my idea.

[eXpl0it3r]

virtual inheritance sounds more like a hack to something that should be avoided on the design stage. Diamond inheritance is not recommended and usually shows an abstraction/design issue. There's a reason why composition over inheritance is being repeated again and again.

Additionally if I understand this correctly virtual inheritance adds yet another v-table so the different bases can be distinguished, thus adding additional overhead to every drawable.

With these points in mind, I don't think this should be added.

As for your example, you make the basic inheritance mistake so you can be a bit lazy with the interface implementaiton. Your FpsMeter is a Widget (I assume sf::Widget was a typo), but your FpsMeter has an sf::Text, as such the correct way is to inherit from Widget and add an sf::Text as composition. In fact, if you have more than one widget that  are sort of like a text object, you could create an intermediate text widget which implements the specific interface and holds the sf::Text object.
And at the end of the day, if you implement your own GUI system, you most likely do not want the widgets to be drawables directly, because you might want to perform some batching, depth resolving or similar things first, before actually rendering it to an SFML render target.

[korczurekk]

Additionally if I understand this correctly virtual inheritance adds yet another v-table so the different bases can be distinguished, thus adding additional overhead to every drawable.

I tested it before proposing change, there is no additional vtable nor any detectable overhead (both in normal game and syntetic tests). Virtual inheritance is based on existing vtable and affects only virtual functions.

As for your example, you make the basic inheritance mistake so you can be a bit lazy with the interface implementaiton. Your FpsMeter is a Widget (I assume sf::Widget was a typo), but your FpsMeter has an sf::Text, as such the correct way is to inherit from Widget and add an sf::Text as composition. In fact, if you have more than one widget that  are sort of like a text object, you could create an intermediate text widget which implements the specific interface and holds the sf::Text object.

I already thought about it and for me it looks like FpsMeter is sf::Text, nothing more than sf::Text that changes it's state depending on delta time.

And at the end of the day, if you implement your own GUI system, you most likely do not want the widgets to be drawables directly, because you might want to perform some batching, depth resolving or similar things first, before actually rendering it to an SFML render target.

I don't, it's just some simple code to add a few buttons to my game, if I wanted a GUI system I'd use tgui or sfgui. 

//edit:

Diamond inheritance is not recommended and usually shows an abstraction/design issue. There's a reason why composition over inheritance is being repeated again and again.

We can argue whether my use case is good example or not, there are situations in which multiple inheritance is recommended and as virtual inheritance doesn't affect efficiency (it could if base class had any member variables/objects, but sf::Drawable doesn't. I also couldn't reproduce performance loss even with member variable) there is no reason why SFML should tie down its users.

And here's synthetic test that I used:
https://gist.github.com/KoczurekK/ad623d8e48594819eead5002d0b4b5fe

[Nexus]

I tested it before proposing change, there is no additional vtable nor any detectable overhead [...] and as virtual inheritance doesn't affect efficiency

That's not exactly true, see here or here. Making inheritance virtual for this very specific case imposes a potential penalty for 99% of sf::Drawable use cases where this is not necessary. I agree with eXpl0it3r regarding that careful design can usually avoid DoD relationships.

Also, "detectable overhead" is very vague. Of course you won't see your game run at half the framerate because of virtual inheritance. But as SFML is a library that has thousands of different application scenarios, such analysis has to be taken very carefully, on multiple compilers, otherwise it's worthless. Your benchmark may not be representative because it uses a single translation unit, where the compiler can perform a lot of optimizations (static binding) because it sees all the involved classes. This may be different if this is offloaded to the linker, or even at runtime through dynamic libraries.

I already thought about it and for me it looks like FpsMeter is sf::Text, nothing more than sf::Text that changes it's state depending on delta time.

sf::Text is a class to visualize text graphically and nothing more. Your FpsMeter however adds logic to it. Its purpose is to display the FPS; how this is done (by sf::Text) is an implementation detail. Many of the sf::Text methods may not be meaningful for your case, see LSP.

We can argue whether my use case is good example or not, there are situations in which multiple inheritance is recommended

Yes, but multiple inheritance and Diamond-of-Death-relation are not equivalent.

[korczurekk]

That's not exactly true, see here or here.

First source is 5 years old, second – 8. 

I agree with eXpl0it3r regarding that careful design can usually avoid DoD relationships.

Yep, I agree. It's actually first situation since years when I realised I have fallen into diamond problem and I want to keep it that way.
However, considering DoD definition, virtual inheritance can make this situation non-ambigous and that means, using virtual keyword solves my problem. It's not DoD anymore.

Also, "detectable overhead" is very vague. Of course you won't see your game run at half the framerate because of virtual inheritance. But as SFML is a library that has thousands of different application scenarios, such analysis has to be taken very carefully, on multiple compilers, otherwise it's worthless. Your benchmark may not be representative because it uses a single translation unit, where the compiler can perform a lot of optimizations (static binding) because it sees all the involved classes. This may be different if this is offloaded to the linker, or even at runtime through dynamic libraries.

You have a point. I tested it now with multiple translation units and dynamic linking – there's still no difference in execution time. Oh, well there is. ~0.1 second per 2000000000 calls with multiple translations units and ~0.1 second per 142857143 calls with dynamic linking, both are less than 1ns per call (same code but adjusted to multiple translation units and dynamic linking). Also keep in mind that my tests involve things that won't happen while using sf::Drawable (like member varialbe)
Yet you're right that it may be this good only in GCC, I'll test it on clang and MSVC later.

//edit: Okay, tested on clang 3.8.
Static linking: virtual inheritance is 0.14s per 4e9 calls faster than traditionall one.
Dynamic linking: Normal is 0.71s per 4e9 calls faster.
So, as you can see differences are way below standard error and I can't really measure anything worth being considered.

sf::Text is a class to visualize text graphically and nothing more. Your FpsMeter however adds logic to it. Its purpose is to display the FPS; how this is done (by sf::Text) is an implementation detail. Many of the sf::Text methods may not be meaningful for your case, see LSP.

All methods existing in sf::Text are useful in FpsMeter, even sf::Text::setString. Oh, and I didn't break LSP, FpsMeter can be used everywhere where sf::Text is being used.

Yes, but multiple inheritance and Diamond-of-Death-relation are not equivalent.

Multiple inheritance is most popular cause of DoD and as I stated a 'few' lines above, It's not DoD if virtual keyword is used (properly ofc).

[Laurent]

All methods existing in sf::Text are useful in FpsMeter, even sf::Text::setString

So what does your FpsMeter class do, then?

[korczurekk]

So what does your FpsMeter class do, then?

Adds update method (I posted it), which once parses previously set string and then (every time update is being called) sets text to properly parsed FPS. If programmer doesn't call update it behaves like normal sf::Text, which is pretty handy.