3
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ (Too late, but) Welcome to Code Review! Nice first question. \$\endgroup\$ – L. F. Sep 1 at 8:31
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.