Inherit from sf::Sprite

[sbroadfoot90]

I notice that sf::Sprite does not have a virtual destructor.

In Effective C++, it mentions that if a class does not have a virtual destructor, then it is not intended as a base class for polymorphism because you can get weird partially destructed objects. For example, I have been using sf::Sprite as a base class for a simple class for a ball in a pong game.

Code: [Select]

class Ball : public Sprite {

};

But then if you have something like this,

Code: [Select]

sf::Sprite * Ball = new Ball();

delete Ball;

then only the destructor for the sf::Sprite class will be called and not the destructor for the Ball class.

This has prompted me to consider including a sf::Sprite as a member of the Ball class.

Code: [Select]

class Ball {
sf::Sprite sprite;

};

This works fine, but leads to some verbose syntax such as

Code: [Select]

Ball.GetSprite().GetSubrect().x;

to simply access the width of the ball.

The other problem is, if you wanted to extend the sf::Sprite class to, say, an AnimatedSprite class, it would only make sense to inherit from sf::Sprite

Code: [Select]

class AnimatedSprite : public Sprite {

};

What can I make of this?

[Elffus]

I thought the base class destructor only needed to be virtual if the base class was being deleted. If you derive from sf::Sprite and delete the derived class it should work perfectly. Actually I don't even think sf::Sprite has a destructor.

[sbroadfoot90]

Yep, I'm saying that if you have a pointer to the base class (sf::Sprite)

Code: [Select]


sf::Sprite * Ball = new Ball();

delete Ball;


and then delete it, then the destructor for the derived class would not be called.

Something like,

Code: [Select]


void DeleteIfCollided(sf::Sprite * sprite1, sf::Sprite * sprite2) {
   if(CheckIfCollided(sprite1, sprite2)) {
      delete sprite1;
      delete sprite2;
   }
}


would not work if you called

Code: [Select]


AnimatedSprite *sprite1 = new AnimatedSprite();
AnimatedSprite *sprite2 = new AnimatedSprite();

DeleteIfCollided(sprite1, sprite2);


Because the derived part of the AnimatedSprite class would not be deleted properly.

The interface for sf::Sprite indicates that it isn't supposed to be a base class, but then how should you implement an AnimatedSprite?

[Nexus]

This has prompted me to consider including a sf::Sprite as a member of the Ball class.

Good idea. You should prefer aggregation over inheritance wherever possible, because it is a weaker coupling and thus provides more flexibility.

This works fine, but leads to some verbose syntax such as

Code: [Select]

Ball.GetSprite().GetSubrect().x;to simply access the width of the ball.

Yes, but that is a problem of bad encapsulation. If the sprite is always accessed directly via member function (maybe even through a non-const reference), you could also make the sprite public. Instead, you could adapt the Ball's interface to provide only the necessary functions, for example GetSize().

Or you could even go one step further and not let the ball depend on SFML graphics classes. Instead, you can directly store the size and some other attributes, and construct the sprite just before the rendering (or store it somewhere else). But those are only some spontaneous thoughts

The other problem is, if you wanted to extend the sf::Sprite class to, say, an AnimatedSprite class, it would only make sense to inherit from sf::Sprite

No, it would not. The Liskov Substitution Principle wouldn't be satisfied. For example, what should GetSubRect() return for an AnimatedSprite if there are multiple subrects (one per animation step)?

And as a general note, the rendering API in SFML 2 is currently changing, so it might be worthwhile to wait some time before making a lot of code dependent on sf::Sprite.

[sbroadfoot90]

Hmm, thank you for your response. Very clarifying

The only reason I figured that AnimatedSprite should inherit from Sprite was because of this tutorial here.

http://www.sfml-dev.org/wiki/en/sources/anisprite

For example, what should GetSubRect() return for an AnimatedSprite if there are multiple subrects (one per animation step)?

Also, I thought the SubRect was supposed to be the portion of the texture that is to be rendered, thus for an animated sprite where the texture would be some sort of image with all the different animation frames, subrect would just refer to the current frame, not having one subrect for each frame

[Nexus]

The only reason I figured that AnimatedSprite should inherit from Sprite was because of this tutorial here.

The Wiki tutorials (especially the old ones) should be considered with care. For example, in that tutorial getters aren't const-qualified and the destructor is defined unnecessarily. Not tragic issues, but those are the things that just attracted my attention.

Also, I thought the SubRect was supposed to be the portion of the texture that is to be rendered, thus for an animated sprite where the texture would be some sort of image with all the different animation frames, subrect would just refer to the current frame, not having one subrect for each frame

But you would change the semantics. If you call SetSubRect() and some time later GetSubRect(), the rectangles may be different. And if you overrode the GetSubRect() method, you would get problems because it is non-virtual.

[sbroadfoot90]

But you would change the semantics. If you call SetSubRect() and some time later GetSubRect(), the rectangles may be different. And if you overrode the GetSubRect() method, you would get problems because it is non-virtual.

Right, thanks. So summary is that you shouldn't inherit from sf::Sprite? Can you think of any exceptions?

[Laurent]

I totaly agree with Nexus. Just wanted to say that sf::Sprite has a virtual destructor, because its own base class (sf::Drawable) has one.

[sbroadfoot90]

One more question on this topic.

Now that the Ball no longer inherits from sf::Sprite, I can no longer make a call like this

Code: [Select]


Ball ball;
sf::RenderWindow window;

window.Draw(ball);


Would it be cleaner to create a getter for the sprite that the Ball holds so I can do this?

Code: [Select]

window.Draw(ball.getSprite());

or would it be best to create a member function in Ball that takes the window and draws its sprite onto that window, like this?

Code: [Select]

class Ball {
public:
   void draw(const sf::RenderWindow &window) {
      window.Draw(sprite);
   }
private:
   sf::Sprite sprite;
};

[Laurent]

The second option is the best.

A class should provide services ("draw me"), not data ("get my sprite"). It's much more abstract and encapsulated this way, the public interface of the class is clean and doesn't expose an implementation detail.

If one day you decide to change the internal representation of your ball to something else than a sf::Sprite, or if sf::Sprite is changed/removed in SFML, the second solution is much easier to maintain.

[eXpl0it3r]

... The Liskov Substitution Principle wouldn't be satisfied...

It's kinda funny to see my programming profs name in the wiki article about the principle (Bertrand Meyer). Although I don't like 'his' language Eiffel but he sure did influnce the OOP section a bit.  :wink:

@Nexus: Are you studying Computer Science, or where did you get all this knowledge about O-O design?
I like the idea of being totally flexible and having independed code, but it's hard to find 'the best way'.
I guess one can constantly redesign everything and never get 'perfect' code.  :roll:

[Nexus]

I have read some books (C++ beginner books, and advanced ones like Exceptional C++ and Modern C++ Design). But I think I have learned most about C++ by experimenting and discussing in forums...

But it's true, there is rarely the one and only solution. It pays off to know multiple approaches and choose an appropriate one when needed
And there are general guidelines like "keep dependencies low", "hide implementation details" etc. which can always be considered. There have been some design discussion threads here recently, you'll find a lot of those tips.