C++ Interface Review

Started by
8 comments, last by SolarChronus 10 years, 2 months ago

Hey everyone,

Creating some classes here and wanted to make sure I was doing my interface stuff in a maintainable way.

I created a TextButton that reacts to mouse overs, clicks, position/scale/rotation can be manipulated, and has a label. I created an interface for each of those 'elements'.

ILabel

IButton

IMouseOver

IMouseLeftClick

GameObject

My TextButton inherits them works perfectly as designed. As I was moving onto creating other elements, such as an ImageButton which will react the same as the TextButton but display an image instead, I wanted to get some feedback to make sure my current interfaces are pretty scale-able..

One note of concern that sticks out to me. In my IMouseOver interface I added a protected member for keeping track of the mousesState. Now I know, typically, interfaces are supposed to be explicit contracts which should only contain methods and not members. I felt having to define that particular member in every single class that inherits it would be super redundant - it's only accessed by the Setter/Getter in the same interface. (Which I also feel is a bit odd to implement those methods in every derived class since their behavior would never alter).

So if anyone could give this a once over and offer up their advice on the way I'm headed, I'd really appreciate it!

Thanks!

ILabel.hpp


#pragma once
#include <string>

class ILabel
{
public:
	virtual ~ILabel(void) {};
	virtual void SetString(std::string string) =0;
	virtual std::string GetString(void) const =0;
};

IButton.hpp


#pragma once

class IButton
{
public:
	virtual ~IButton(void) {};
	virtual void Enable(void) =0;
	virtual void Disable(void) =0;
	virtual bool IsEnabled(void) const =0;
};

IMouseOver.hpp


#pragma once

namespace MouseOverState
{
	enum MouseOverState
	{
		None =0,
		Begin,
		Stay,
		End
	};
};

class IMouseOver
{
public:
	virtual ~IMouseOver(void) {};
	virtual bool IsMouseOver(sf::Vector2i mousePosition) =0;
	virtual void OnMouseOverBegin(void) =0;
	virtual void OnMouseOverStay(void) = 0;
	virtual void OnMouseOverEnd(void) =0;
	virtual void SetMouseOverState(MouseOverState::MouseOverState mos) { mouseOverState = mos;};
	virtual MouseOverState::MouseOverState GetMouseOverState(void) const { return mouseOverState; };

protected:
	MouseOverState::MouseOverState mouseOverState;

};

IMouseLeftClick.hpp


#pragma once

class IMouseLeftClick
{
public:
	virtual ~IMouseLeftClick(void) {};
	virtual void OnMouseLeftClickPressed(void) =0;
	virtual void OnMouseLeftClickReleased(void) =0;
	virtual void AddLeftClickEvent(boost::function<void()> signal) =0;
};

TextButton.hpp


#pragma once
#include <string>
#include "boost/signals2.hpp"
#include "GameObject.hpp"
#include "ILabel.hpp"
#include "IButton.hpp"
#include "IMouseOver.hpp"
#include "IMouseLeftClick.hpp"
#include "Text.hpp"
#include "Input.hpp"

class TextButton: public GameObject, public virtual ILabel, public virtual IButton, virtual IMouseOver, virtual IMouseLeftClick
{
public:
	TextButton(void);
	TextButton(std::string fontName, int fontSize, sf::Color idleColor, sf::Color hoverColor, sf::Color pressedColor, sf::Color disabledColor, std::string string);
	~TextButton(void);

	//TextButton
	void HandleInput(Key key, Mouse mouse);

	//GameObject
	void Update(float gameTime);
	void Draw(sf::RenderWindow* window, float gameTime);
	void SetPosition(sf::Vector2f);

	//ILabel
	void SetString(std::string string);
	std::string GetString(void) const;

	//IButton
	void Enable(void);
	void Disable(void);
	bool IsEnabled(void) const;
	sf::FloatRect GetWorldBounds(void) const;

	//IMouseOver
	bool IsMouseOver(sf::Vector2i mousePosition);
	void OnMouseOverBegin(void);
	void OnMouseOverStay(void);
	void OnMouseOverEnd(void);

	//IMouseLeftClick
	void OnMouseLeftClickPressed(void);
	void OnMouseLeftClickReleased(void);
	void AddLeftClickEvent(boost::function<void()> signal);

private:
	Text text;
	sf::FloatRect bounds;
	bool isEnabled;
	sf::Color idleColor;
	sf::Color hoverColor;
	sf::Color pressedColor;
	sf::Color disabledColor;
	boost::signals2::signal<void ()> leftClickSignals;

};
Advertisement

IMouseOver.hpp

#pragma once

namespace MouseOverState
{
	enum MouseOverState
	{
		None =0,
		Begin,
		Stay,
		End
	};
};

class IMouseOver
{
public:
	virtual ~IMouseOver(void) {};
	virtual bool IsMouseOver(sf::Vector2i mousePosition) =0;
	virtual void OnMouseOverBegin(void) =0;
	virtual void OnMouseOverStay(void) = 0;
	virtual void OnMouseOverEnd(void) =0;
	virtual void SetMouseOverState(MouseOverState::MouseOverState mos) { mouseOverState = mos;};
	virtual MouseOverState::MouseOverState GetMouseOverState(void) const { return mouseOverState; };

protected:
	MouseOverState::MouseOverState mouseOverState;

};

Why don't you do this instead?

[source]

class IMouseOver
{
public:
virtual ~IMouseOver(void) {};
virtual bool IsMouseOver(sf::Vector2i mousePosition) =0;
virtual void OnMouseOverBegin(const MouseOverState::MouseOverState&) =0;
virtual void OnMouseOverStay(const MouseOverState::MouseOverState&) = 0;
virtual void OnMouseOverEnd(const MouseOverState::MouseOverState&) =0;
};

[/source]

Can you explain why you have the state inside the methods? Are you assuming that each method would be called, sending a referenced state variable, and that method would only act if the state is the correct one?

Can you explain why you have the state inside the methods? Are you assuming that each method would be called, sending a referenced state variable, and that method would only act if the state is the correct one?

Considering the getters and setters vanished in the refactored version, I think you would be passing in the desired state (thereby setting it).

Ok, I think I see. So instead of calling the "SetMouseOverState(...)" and passing in the desired state. I would just instead call the correct behavior induced method with the correct state?

So I would change my current OnMouseOverBegin(...) implementation from:


void TextButton::OnMouseOverBegin(void)
{
     text.SetColor(hoverColor);
}

To this?


void TextButton::OnMouseOverBegin(const MouseOverState::MouseOverState& state)
{
     mouseOverState = state;
     text.SetColor(hoverColor);
}

I can see that lightening up the if chain I'm using so I wouldn't have both the Set(...) and then call the appropriate OnMouseOver___ method, but since I've removed the setter like that would I even need to pass in the desired state? Would this be appropriate?


void TextButton::OnMouseOverBegin(void)
{
     mouseOverState = MouseOverState::Begin;
     text.SetColor(hoverColor);
}

I also noticed just now, he didn't include the variable. Whoops. You won't be setting it at all, just manipulating your data, and since it is passed by reference, you would use it as a transition. In the Begin method, you would set the state passed in to Stay, in the Stay method when it is no longer mouse over, in the End method it would set the state to not a mouse over.

I think the question is why would you ever need to maintain the state over different events? Why are you preserving the MouseOverState from the Begin, Stay, and End events? I would assume these events would induce different states every time a new event is generated. The X,Y position of the mouse will be different. The buttons clicked will be different.

How are you managing the mouse events?

That's a pretty good question, looking at my code I only use the state information to choose which OnMouseOver method should be fired, but beyond that, I output it to a debug log.

The mouse events are being maintained via a struct, every frame I figure out what the state of all the buttons and its position is, then save it for use by the various classes that want to know about the mouse state.

Here is the handle input function if it'll make it easier to see how I'm using the mouse over state variable.


void TextButton::HandleInput(Mouse mouse)
{
	// Don't accept input if the button is disabled.
	if(isEnabled == false)
		return;

	bool isMouseOver = IsMouseOver(mouse.mousePosition);

	//Set MouseOverState
	if(isMouseOver)
	{
		if(mouseOverState  == MouseOverState::None)
		{
			OnMouseOverBegin();
		}
		else if(mouseOverState == MouseOverState::Begin)
		{
			OnMouseOverStay();
		}

		if(Input::IsLeftMouseButtonPressed())
		{
			OnMouseLeftClickPressed();
		}

		if(Input::IsLeftMouseButtonHeld())
		{
			text.SetColor(pressedColor);
		}

		if(Input::IsLeftMouseButtonReleased())
		{
			OnMouseLeftClickReleased();
		}
	}
	else
	{
		if(mouseOverState == MouseOverState::Stay)
		{
			OnMouseOverEnd();
		}
		else if(mouseOverState == MouseOverState::End)
		{
			mouseOverState = MouseOverState::None;
		}
	}
}



void TextButton::HandleInput(Mouse mouse)
{
	// Don't accept input if the button is disabled.
	if(isEnabled == false)
		return;

	bool isMouseOver = IsMouseOver(mouse.mousePosition);

	//Set MouseOverState
	if(isMouseOver)
	{
		if(mouseOverState  == MouseOverState::None)
		{
			OnMouseOverBegin();
		}
		else if(mouseOverState == MouseOverState::Begin)
		{
			OnMouseOverStay();
		}

		if(Input::IsLeftMouseButtonPressed())
		{
			OnMouseLeftClickPressed();
		}

		if(Input::IsLeftMouseButtonHeld())
		{
			text.SetColor(pressedColor);
		}

		if(Input::IsLeftMouseButtonReleased())
		{
			OnMouseLeftClickReleased();
		}
	}
	else
	{
		if(mouseOverState == MouseOverState::Stay)
		{
			OnMouseOverEnd();
		}
		else if(mouseOverState == MouseOverState::End)
		{
			mouseOverState = MouseOverState::None;
		}
	}
}

IMO, you should have handled this on GameObject. That way all objects that inherit from GameObject will get mouse handling for free.

Having said that, considering that your MouseOverState is just an enum, and its values are Begin, Stay, End, I would collapse all those mouse events into just one:

[source]

class IMouseOver

{

public:

virtual void OnMouseOver(const MouseOverState::MouseOverState&) = 0;

}

[/source]

Then, when you want to invoke a Begin/Stay/End event:

[source]

if (isMouseOver)

{

if (currentMouseOver == MouseOverState::None)

{

currentMouseOver = MouseOverState::Begin;

OnMouseOver(currentMouseOver);

}

// ... and so on

}

[/source]

currentMouseOver is an instance variable that belongs to TextButton. Again, If you do this on the GameObject level, this implementation would be in the GameObject, currentMouseOver would belong to GameObject. TextButton, ImageButton, EtcButton would get it all for free, and they don't have to maintain the mouse over state.

You could also go another step. Not all GameObjects should respond to mouse states. Some GameObjects might be invisible, for example. Then you can create another class, let's call it UIObject, which inherits from GameObject and implements the IMouseOver interface. Then your TextButton/ImageButton would inherit from UIObject.

[source]

class GameObject { ... };

class UIObject : public GameObject, public virtual IMouseOver { ... /* implement your mouse stuff here */ };

class TextButton : public UIObject { ... };

[/source]

alnite, thank you for your help.

That last post allowed me to take a step back and look at my interfaces in a different light and enum states in general.

This topic is closed to new replies.

Advertisement