C++ SFML Animation System 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.


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


Here is the code:

Animation base class

class Animation {
    typedef std::chrono::high_resolution_clock hrc;

private:

    std::chrono::time_point&lt;hrc&gt; 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&lt;std::chrono::microseconds&gt;(diff);
        m_us += (unsigned int)msDuration.count();
    }

public:

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

    bool finished() {
        return m_endUs &lt;= m_us;
    }

    virtual bool Update(sf::Sprite&amp; 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&amp; 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&lt;Animation&gt; 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 &lt;typename T, typename... Args&gt;
    void SetAnimation(Args... args) {
        m_animationPtr = std::make_unique&lt;T&gt;(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()-&gt;Update(sprite)) {
                m_animationPtr = nullptr;
            }
            sf::Color c = sprite.getColor();
        }
        target-&gt;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(&amp;shape, floatRectToVec2u(shape.getGlobalBounds()));
    circle.SetAnimation&lt;FadeIn&gt;(1000);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        circle.draw(&amp;window);
        window.display();
    }

    return 0;
}
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&lt;hrc&gt; 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&lt;std::chrono::microseconds&gt;(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 &lt;class Rep, class Period&gt;
explicit Animation(const std::chrono::duration&lt;Rep, Period&gt;&amp; 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 &lt;= m_us;
}



You are missing const.


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



Good.

Animation child class


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



Missing explicit &mdash; 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&amp; 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&lt;double&gt;(m_time_elapsed) / m_total_time;
        color.a = static_cast&lt;int&gt;(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&amp; 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 &mdash; except for the C-style casts:


remove them if possible;
otherwise, use unsigned{size.x} if possible;
otherwise, use static_cast.



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



You are missing perfect forwarding:

template &lt;typename T, typename... Args&gt;
void SetAnimation(Args&amp;&amp;... args)
{
    m_animationPtr = std::make_unique&lt;T&gt;(std::forward&lt;Args&gt;(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()-&gt;Update(sprite)) {
            m_animationPtr = nullptr;
        }
        sf::Color c = sprite.getColor();
    }
    target-&gt;draw(sprite);
}



Always turn warnings on &mdash; 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 &mdash; 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(&amp;shape, floatRectToVec2u(shape.getGlobalBounds()));
    circle.SetAnimation&lt;FadeIn&gt;(1000);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        circle.draw(&amp;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.