Button click logic with SFML

Question

I wrote some functionality that works as intended in order to create functional clickable rectangles. I inherit from RectangleShape in SFML and I have several objects (not just buttons) that inherit from this. The intended logic is to check if a downpress and release pair both happen inside of the same object.

clickableRect.h

#ifndef CLICKABLERECT
#define CLICKABLERECT
#include <SFML\Graphics.hpp>
class clickableRect : public sf::RectangleShape
{
public:
 clickableRect();
 bool click(const sf::RenderWindow & window, const sf::Event & ev);
 void reset();
private:
 bool m_mouseHeld;
 bool m_pressed;
 bool m_active;
 bool isInside(const sf::RenderWindow & window) const;
};
#endif // !CLICKABLERECT

and

clickableRect.cpp

#include "clickableRect.h"
clickableRect::clickableRect() :
 m_pressed(false),
 m_mouseHeld(false),
 m_active(false)
{
 //do nothing
}
bool clickableRect::click(const sf::RenderWindow & window, const sf::Event & ev)
{
 if (ev.type == sf::Event::MouseButtonPressed && isInside(window) && !m_mouseHeld)
 {
 m_pressed = true;
 m_mouseHeld = true;
 }
 else if (ev.type == sf::Event::MouseButtonPressed)
 {
 m_mouseHeld = true;
 }
 if (ev.type == sf::Event::MouseButtonReleased)
 {
 if (isInside(window) && m_pressed)
 {
 m_active = true;
 }
 m_pressed = false;
 m_mouseHeld = false;
 }
 return m_active;
}
void clickableRect::reset()
{
 m_active = false;
}
bool clickableRect::isInside(const sf::RenderWindow & window) const
{
 return sf::Mouse::getPosition(window).x > getPosition().x
 && sf::Mouse::getPosition(window).x < getPosition().x + getSize().x
 && sf::Mouse::getPosition(window).y > getPosition().y
 && sf::Mouse::getPosition(window).y < getPosition().y + getSize().y;
}

I would love any and all feedback on all aspects of the code provided but request specifically to know about:

1. Is this a good use of inheritance and am I doing it right? Should I use a component pattern instead?
2. Can I clean up the logic itself inside the click function. It seems a bit messy to me and also requires me to call reset on the objects immediately after returning a true value.

• yuri: Is this part of a GUI you plan to build or part of the game itself?
• Summer: GUI I suppose. I wrote it specifically for the game I'm working on but did so with reusability in mind(or tried to)
• yuri: Have you considered using one of the many available GUI libraries or are you writing your own specifically to learn?
• Summer: To learn. Should i have added reinventing the wheel? I also just figured out one of my concerns. If I move m_active to function scope and initialize it to false it will not require a reset anytime it is active.

Answer 1

• Compile with all warnings

If you compile this code with all warnings enabled (e.g. -Wall -Wextra -pedantic) the compiler will tell you that your initialization is out of order.
This simply means you initialize your members in a different order than they have been declared in. In your example this is not bad but if you have members that depend on others being initialized before them this can lead to problems.

• & is part of the type in C++

While this is not an error it can look like bitwise AND at a glance and be a potential source of confusion. Prefer to write foo& bar instead of foo & bar.

• User defined types

Also just a minor nitpick but usually user defined types start with an uppercase letter i.e. ClickableRect.
Since you inherit from Shape I think it would make more sense to name your class ClickableShape but this is just personal preference.

• Design

You expose window to your class only to get access to the mouse coordinates. Unless you plan to use window in a different matter in the future it'd be better to hide this info from your class.
A simple approach to testing for a click within your shape could be:

class ClickableShape : public sf::RectangleShape {
public:
 bool click(const sf::Vector2i& mouse_pos) const {
 return getGlobalBounds().contains(mouse_pos.x, mouse_pos.y);
 }
};

Which can then be called as:

ClickableShape cs{};
cs.setSize(sf::Vector2f(50, 50));
cs.setFillColor(sf::Color(0xffffffff));

And finally in your event loop:

case sf::Event::MouseButtonPressed:
 cs.click(sf::Mouse::getPosition(window));
 break;

This way you avoid exposing unnecessary data to your class and can still check if the shape was clicked.

Approach #2:

I'm not sure what you're planning to do with this in the future but another approach to this would be to keep the inheritance short and have the rectangle as a member.

class MyButton : public sf::Drawable {
public:
 MyButton() {
 shape.setSize(sf::Vector2f(50, 50));
 shape.setFillColor(sf::Color(0xffffffff));
 }
 bool click(const sf::Event& event) const {
 return shape.getGlobalBounds().contains(event.mouseButton.x, event.mouseButton.y);
 }
 void draw(sf::RenderTarget& target, sf::RenderStates states) const {
 target.draw(shape);
 }
private:
 sf::RectangleShape shape;
};

• Summer: RectangleShape inherits from Shape and I inherit from RectangleShape. If I change the name to ClickableShape arent I implying that other Shapes (i.e. CircleShape) can also use this functionality? This is part of the reason I was worried about an inheritance chain this deep. I even then have 4 classes that inherit from Clickable.
• Harald Scheirich: @bruglesco With regard to window, the code probably shouldn't pull the mouse coordinates from the window anyway but from the data that is in the event. This way the the coordinates are from where the event was triggered and not the current coordinates, which could be different
• yuri: @bruglesco I'm unsure about your intent but if you're worried about inheritance chains you could also inherit from sf::drawable and keep the rectangle as a member?
• yuri: @HaraldScheirich I used your suggestion about the event in the 2nd approach, was this what you meant or something different?
• Harald Scheirich: Yea that’s what I meant, sorry totally missed that, but I think it’s an important distinction to make, using he current mouse coordinates is the wrong solution in most cases