C++ SFML Animation System

Question

I'm attempting to write an animation system on top of SFML, and I've come up with a design where I can wrap sf::Drawable objects into a RenderComponent class, and the RenderComponent class can be given an Animation object at any time, which will overwrite a previous animation if one exists. Here is what I'm looking for.

1. Am I using the std::unique_ptr correctly/optimally?
2. Should I be using a pointer to store the Animation?
3. Is my method of settings the animation (with a variadic template) too complicated, and is there a better way?
4. I would normally separate the code into header and implementation, but for the brevity, I am uploading it in pure headers. Ignore that please.
5. Any general advice.

Here is the code:

Animation base class

class Animation {
 typedef std::chrono::high_resolution_clock hrc;
private:
 std::chrono::time_point<hrc> m_now;
protected:
 unsigned int m_us; // us for microseconds
 unsigned int m_endUs;
 void UpdateTime() {
 auto end = hrc::now();
 auto diff = end - m_now;
 m_now = end;
 auto msDuration = std::chrono::duration_cast<std::chrono::microseconds>(diff);
 m_us += (unsigned int)msDuration.count();
 }
public:
 Animation() {
 m_us = 0;
 m_now = hrc::now();
 }
 bool finished() {
 return m_endUs <= m_us;
 }
 virtual bool Update(sf::Sprite& spr) = 0;
};

Animation child class

class FadeIn : public Animation {
public:
 FadeIn(int ms) {
 m_endUs = ms * 1000;
 }
 // Updates the sprite based on the timeline, and returns if the animation is over
 virtual bool Update(sf::Sprite& spr) {
 UpdateTime();
 if (finished()) return true;
 sf::Color color = spr.getColor();
 color.a = (int)((float)m_us / m_endUs * 255);
 spr.setColor(color);
 return false;
 }
};

Render Component

class RenderComponent {
private:
 sf::Texture m_texDefault;
 std::unique_ptr<Animation> m_animationPtr;
public:
 RenderComponent() { }
 RenderComponent(sf::Drawable* element, sf::Vector2u size) {
 sf::RenderTexture rt;
 rt.create((unsigned int)size.x, (unsigned int)size.y);
 rt.draw(*element);
 m_texDefault = rt.getTexture();
 }
 template <typename T, typename... Args>
 void SetAnimation(Args... args) {
 m_animationPtr = std::make_unique<T>(args...);
 }
 void draw(sf::RenderTarget* target) {
 sf::Sprite sprite;
 sprite.setTexture(m_texDefault);
 // Handle animation and set pointer to null if done
 if (m_animationPtr) {
 if (m_animationPtr.get()->Update(sprite)) {
 m_animationPtr = nullptr;
 }
 sf::Color c = sprite.getColor();
 }
 target->draw(sprite);
 }
};

A helper function

sf::Vector2u floatRectToVec2u(sf::FloatRect r) {
 sf::Vector2u vec;
 vec.x = (unsigned int)ceil(r.width);
 vec.y = (unsigned int)ceil(r.height);
 return vec;
 auto start = std::chrono::high_resolution_clock::now();
}

Main function

int main()
{
 sf::RenderWindow window(sf::VideoMode(200, 200), "SFML works!");
 sf::CircleShape shape(100.f);
 shape.setFillColor(sf::Color::Green);
 RenderComponent circle(&shape, floatRectToVec2u(shape.getGlobalBounds()));
 circle.SetAnimation<FadeIn>(1000);
 while (window.isOpen())
 {
 sf::Event event;
 while (window.pollEvent(event))
 {
 if (event.type == sf::Event::Closed)
 window.close();
 }
 window.clear();
 circle.draw(&window);
 window.display();
 }
 return 0;
}

• L. F.: (Too late, but) Welcome to Code Review! Nice first question.

Answer 1

Animation base class

typedef std::chrono::high_resolution_clock hrc;

In modern C++, use an alias-declaration:

using hrc = std::chrono::high_resolution_clock;

It is arguably more readable.

std::chrono::time_point<hrc> m_now;
unsigned int m_us; // us for microseconds
unsigned int m_endUs;

The types and names of the last two members are not helpful. (By the way, unsigned int = unsigned.) And they should be private:

private:
 using time_point = std::chrono::high_resolution_clock::time_point;
 time_point m_now;
 using duration = std::chrono::high_resolution_clock::duration;
 duration m_time_elapsed;
 duration m_total_time;

The derived classes should have read-only access to m_total_time:

protected:
 auto total_time() const
 {
 return m_total_time; 
 }

void UpdateTime() {
 auto end = hrc::now();
 auto diff = end - m_now;
 m_now = end;
 auto msDuration = std::chrono::duration_cast<std::chrono::microseconds>(diff);
 m_us += (unsigned int)msDuration.count();
}

This function is a bit complex because of the conversion between different types. They can be simplified:

void UpdateTime()
{
 auto time = hrc::now();
 time_elapsed += time - m_now;
 m_now = time;
}

Animation() {
 m_us = 0;
 m_now = hrc::now();
}

The constructor should provide a means to set the m_total_time member. And it should use member initializer clauses instead of assignment:

template <class Rep, class Period>
explicit Animation(const std::chrono::duration<Rep, Period>& total_time)
 : m_now{hrc::now()}
 , m_time_elapsed{}
 , m_total_time{total_time} // std::chrono::duration supports conversion
{
}

(The support for different durations is for convenience.)

bool finished() {
 return m_endUs <= m_us;
}

You are missing const.

virtual bool Update(sf::Sprite& spr) = 0;

Good.

Animation child class

FadeIn(int ms) {
 m_endUs = ms * 1000;
}

Missing explicit — an integer is not logically a FadeIn. Type mismatch (you are using unsigned in the base class). With the design mentioned, just do this:

using Animation::Animation;

And the constructors will work as expected.

// Updates the sprite based on the timeline, and returns if the animation is over
virtual bool Update(sf::Sprite& spr) {
 UpdateTime();
 if (finished()) return true;
 sf::Color color = spr.getColor();
 color.a = (int)((float)m_us / m_endUs * 255);
 spr.setColor(color);
 return false;
}

Missing override. Don't use C-style casts. float may be too imprecise for this calculation. Don't put the whole if statement on a single line.

The color algorithm should be in a separate function:

private:
 sf::Color get_color() const noexcept
 {
 auto color = spr.getColor();
 double ratio = static_cast<double>(m_time_elapsed) / m_total_time;
 color.a = static_cast<int>(ratio * 255);
 return color;
 }

Also, if overflow is not a concern, just multiply first and then divide to avoid the floating point. And then the function can be simplified:

virtual bool Update(sf::Sprite& spr) override
{
 UpdateTime();
 if (finished()) {
 return true;
 } else {
 spr.setColor(get_color());
 return false;
 }
}

Render Component

RenderComponent() { }
RenderComponent(sf::Drawable* element, sf::Vector2u size) {
 sf::RenderTexture rt;
 rt.create((unsigned int)size.x, (unsigned int)size.y);
 rt.draw(*element);
 m_texDefault = rt.getTexture();
}

Good — except for the C-style casts:

• remove them if possible;

• otherwise, use unsigned{size.x} if possible;

• otherwise, use static_cast.

template <typename T, typename... Args>
void SetAnimation(Args... args) {
 m_animationPtr = std::make_unique<T>(args...);
}

You are missing perfect forwarding:

template <typename T, typename... Args>
void SetAnimation(Args&&... args)
{
 m_animationPtr = std::make_unique<T>(std::forward<Args>(args)...);
}

void draw(sf::RenderTarget* target) {
 sf::Sprite sprite;
 sprite.setTexture(m_texDefault);
 // Handle animation and set pointer to null if done
 if (m_animationPtr) {
 if (m_animationPtr.get()->Update(sprite)) {
 m_animationPtr = nullptr;
 }
 sf::Color c = sprite.getColor();
 }
 target->draw(sprite);
}

Always turn warnings on — unused c variable should issue a warning. (I am pretty sure sprite.getColor() has any side effects.)

A helper function

sf::Vector2u floatRectToVec2u(sf::FloatRect r) {
 sf::Vector2u vec;
 vec.x = (unsigned int)ceil(r.width);
 vec.y = (unsigned int)ceil(r.height);
 return vec;
 auto start = std::chrono::high_resolution_clock::now();
}

This function is a pure math function, so should probably be noexcept. The name is a bit awkward — it does not mention ceil at all. Also, it seems that ceil should be std::ceil. And what does the last line do?

If sf::Vector2u can be constructed with the coordinates, the code is simplified:

sf::Vector2u ceil_vector(sf::FloatRect r)
{
 return {std::ceil(r.width), std::ceil(r.height)};
}

Main function

int main()
{
 sf::RenderWindow window(sf::VideoMode(200, 200), "SFML works!");
 sf::CircleShape shape(100.f);
 shape.setFillColor(sf::Color::Green);
 RenderComponent circle(&shape, floatRectToVec2u(shape.getGlobalBounds()));
 circle.SetAnimation<FadeIn>(1000);
 while (window.isOpen())
 {
 sf::Event event;
 while (window.pollEvent(event))
 {
 if (event.type == sf::Event::Closed)
 window.close();
 }
 window.clear();
 circle.draw(&window);
 window.display();
 }
 return 0;
}

The main function looks nice. (I don't why you are explicitly specifying 100.f here instead of 100, but maybe there's a good reason.) return 0; is redundant for main and can be omitted. event can be declared in the inner loop with for.