2D falling snow animation

Question

I was hoping that someone could review this code.

My main concern is that the contents of main() are difficult to read. I thought about extracting some segments of code into functions (to aid comprehension) but it felt unnecessary because these segments are not completely repeated.

#include <SFML/Graphics.hpp>
#include <random>

// Maps input in the range [0..1] to an output in the range [0..1].
// Represents the piecewise function:
// y(x) = 2*x^2             when x < 0.5
//      = -2*x^2 + 4*t - 1  when x >= 0.5
float easeInOutQuad(float normalisedTime)
{
  auto & t = normalisedTime;
  return t < 0.5 ? 2*t*t : 2*t*(2 - t) - 1;
}

struct Tween
{
  float begin;
  float change;
  float time{0};
  float duration;
  float current() const
  {
    return begin + change*easeInOutQuad(time/duration);
  }
};

int main()
{
  sf::RenderWindow window{sf::VideoMode{500, 500}, "Snow"};
  auto const inverseFramerate = 1.f/60;
  window.setFramerateLimit(static_cast<unsigned>(1/inverseFramerate));
  auto const numberOfSnowflakes = 200;
  Tween tweens[numberOfSnowflakes];
  float yPositions[numberOfSnowflakes];
  auto generator = std::mt19937{};
  // Returns a normally distributed random number.
  auto randomChangeInX = [&generator]()
  {
    static auto distribution = std::normal_distribution<float>{0.f, 10.f};
    return distribution(generator);
  };
  // Returns a uniformly distributed random number in the range [min..max).
  auto randomBetween = [&generator](float min, float max)
  {
    return std::generate_canonical<float, 10>(generator)*(max - min) + min;
  };
  for (auto i = 0; i < numberOfSnowflakes; ++i)
  {             
    tweens[i].begin = randomBetween(0, window.getSize().x);
    tweens[i].change = randomChangeInX();
    tweens[i].duration = randomBetween(1, 5); 
    yPositions[i] = randomBetween(-20, window.getSize().y);
  }
  auto const defaultRadius = 5.f;
  auto circle = sf::CircleShape{};
  while (window.isOpen())
  {
    auto event = sf::Event{};
    while (window.pollEvent(event))
    {
      if (event.type == sf::Event::Closed)
      {
        window.close();
      }
    }
    for (auto i = 0; i < numberOfSnowflakes; ++i)
    {
      tweens[i].time += inverseFramerate;
      if (tweens[i].time > tweens[i].duration)
      {
        tweens[i].begin += tweens[i].change;
        tweens[i].change = randomChangeInX();
        tweens[i].time = 0;
      }

      yPositions[i] += 1/tweens[i].duration;
      if (yPositions[i] > window.getSize().y)
      {
        yPositions[i] = -defaultRadius*2;
        tweens[i].begin = randomBetween(0, window.getSize().x);
        tweens[i].change = randomChangeInX();
        tweens[i].time = 0;
      }
    }
    window.clear();
    for (auto i = 0; i < numberOfSnowflakes; ++i)
    {
      circle.setPosition(tweens[i].current(), yPositions[i]);
      circle.setRadius(defaultRadius/tweens[i].duration);
      window.draw(circle);      
    }
    window.display();
  }
}

Answer 1

Tween is not a very good name. How about Snowflake? And why is it a struct and exposing all of its implementation details. I would personally place it in a class, initialize its values via constructor, and give it a public function like fall() or something to update it each frame.

class Snowflake
{
public:
    void fall();
private:
    sf::CircleShape snowflake;
    float begin;
    float change;
    float time{ 0 };
    float duration;
    float current() const
    {
        return begin + change * easeInOutQuad(time / duration);
    }
};

By putting the circle in the class you can update it internally. More on all that later.

---

float easeInOutQuad(float normalisedTime)
{
  auto & t = normalisedTime;
  return t < 0.5 ? 2*t*t : 2*t*(2 - t) - 1;
}

It seems a little unusual to assign a reference to the argument and then return the reference. I'm actually a little surprised this didn't boom. it's a trivially copied float. Just pass it by value.

---

auto const inverseFramerate = 1.f / 60;
window.setFramerateLimit(static_cast<unsigned>(1 / inverseFramerate));

That is an odd way to write

window.setFramerateLimit(60);

Now you could replace the magic number 60 that I just passed here with a named constant but I would actually argue that this is one of those rare cases when you shouldn't. This will be the only call to this function and the name won't be more readable here. Most readers are likely to immediately recognize that the 60 is setting 60 fps.

No cast. No equation. easily readable. less lines.

---

auto const numberOfSnowflakes = 200;
Tween tweens[numberOfSnowflakes];
float yPositions[numberOfSnowflakes];

This is a much more useful named constant. However. Don't use C array[]s. std::array or std::vector are better C++ containers. std::array is static sized like the C-style array[] and std::vector is dynamically sized. Don't forget to #include whichever you choose to use. You should keep the constant but switch to the more preferred constexpr

constexpr int number_of_snowflakes = 200;

---

auto generator = std::mt19937{};

A lot of your difficult to read code revolves around your use and misuse of this object. Allow me to show you a slightly easier to use way of handling PRNG. I'd wrap it all into its own named function and do away with the two harder to read and use lambdas. Then inside the function you seed the generator and declare it static so it doesn't get destroyed when it goes out of scope.

//we take unsigned and return float in order to work with SFML's window size which is unsigned
float generateRandom(unsigned min, unsigned max)
{
    float real_min = static_cast<float>(min);
    float real_max = static_cast<float>(max);
    std::random_device rd;
    static const std::mt19937 generator(rd());
    std::uniform_real_distribution<float> distribution(real_min, real_max);

    return distribution(generator);
}

Now when you want a value between 0 - width you do

start = generateRandom(0, window.getSize().x)

This is a weird reimplementation of uniform_real_distribution. You did it so you could work with negative ranges because you want your snowflakes to start off-screen. But all you need is an offset applied after the fact.

auto randomBetween = [&generator](float min, float max)
{
    return std::generate_canonical<float, 10>(generator)*(max - min) + min;
}; 

---

Now we get to initialization. In the future it is best to declare and initialize variables together. If you need helper functions have them defined beforehand and ready to use rather than defining them in between instantiation and initialization.

for (auto i = 0; i < numberOfSnowflakes; ++i)
{
    tweens[i].begin = randomBetween(0, window.getSize().x);
    tweens[i].change = randomChangeInX();
    tweens[i].duration = randomBetween(1, 5);
    yPositions[i] = randomBetween(-20, window.getSize().y);
}
auto const defaultRadius = 5.f;
auto circle = sf::CircleShape{};

Most of this can be eliminated by doing two things. Move the sf::Circle into our new Snowflake class and we use default initialization within the class. You can also be completely rid of the second yPositions array as the Snowflake class circle can maintain its own position. You will have to pass the starting coordinates to the constructor because the Snowflake class won't have full access to the sf::RenderWindow class. Our class is now starting to look like this:

class Snowflake
{
public:
    explicit Snowflake(sf::Vector2f start_position);

    void fall();
private:
    float change{ generateRandom(1, 10) };
    float duration{ generateRandom(1, 5) };
    float time{ 0 };
    sf::Vector2f position;
    sf::CircleShape snowflake{ default_radius / duration };
    float current() const
    {
        return position.x + change * easeInOutQuad(time / duration);
    }
};

You can do the initialization like so:

std::vector<Snowflake> snowflakes;
snowflakes.reserve(number_of_snowflakes);
for (int i = 0; i < number_of_snowflakes; ++i)
{
    float x = generate_random(0, window.getSize().x);
    float y = generate_random(-20, window.getSize().y);
    sf::Vector2f start_position(x, y);
    snowflakes.emplace_back(Snow(start_position));
}

This will give you a nice standard container of snowflakes at default initialized

---

So now we get to your animation loop. I see now why you did inverseFramerate now, but it was a complicated way of having your snow fall at varying rates. As you have moved everything into the Snowflake class you can have a much simpler animation loop.

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

    for (auto&& snow : snowflakes)
    {
        snow.fall(window.getSize());
    }

    window.clear();
    for (auto&& snow : snowflakes)
    {
        snow.draw(window);
    }
    window.display();
}

All that leaves us with is implementing the fall() and draw() methods from above. draw() is simple and would look something like this:

void draw(sf::RenderWindow& window) { window.draw(snowflake); }

and the fall() method I just copied over your implementation details using my new class member variables. It looks something like this:

---

Put it all together and it would look like this:

#include <random>
#include <vector>

#include <SFML/Graphics.hpp>

constexpr int number_of_snowflakes = 200;
constexpr float single_frame = 1.F / 60;
constexpr float default_radius = 5.F;

// Maps input in the range [0..1] to an output in the range [0..1].
// Represents the piecewise function:
// y(x) = 2*x^2             when x < 0.5
//      = -2*x^2 + 4*t - 1  when x >= 0.5
float ease_in_and_out(float normalised_time)
{
    float t = normalised_time;
    return t < 0.5 ? 2 * t*t : 2 * t*(2 - t) - 1;
}

//we take unsigned and return float in order to work with SFML's window size which is unsigned
float generate_random(unsigned min, unsigned max)
{
    float real_min = static_cast<float>(min);
    float real_max = static_cast<float>(max);
    static std::random_device rd;
    static std::mt19937 generator(rd());
    std::uniform_real_distribution<float> distribution(real_min, real_max);

    return distribution(generator);
}

class Snowflake
{
public:
    explicit Snowflake(sf::Vector2f start_position) : position{ start_position }
    { 
        snowflake.setPosition(position);
        begin = start_position.x;
    }

    void fall(sf::Vector2u window_size)
    {
        time += single_frame;
        if (time > duration)
        {
            begin += change;
            change = generate_random(1, 10);
            time = 0.F;
        }

        position.y = snowflake.getPosition().y;
        position.y += 1 / duration;
        if (position.y > window_size.y)
        {
            position.y = -10.F;
            begin = generate_random(0, window_size.x);
            change = generate_random(1, 10);
            time = 0;
            snowflake.setRadius(default_radius / duration);
        }

        position.x = shift_x();
        snowflake.setPosition(position);
    }
    void draw(sf::RenderWindow& window) { window.draw(snowflake); }
private:
    sf::Vector2f position;
    float begin;
    float change{ generate_random(1, 10) };
    float duration{ generate_random(1, 5) };
    float time{ 0 };
    sf::CircleShape snowflake{ default_radius / duration };

    float shift_x() const
    {
        return begin + change * ease_in_and_out(time / duration);
    }
};

int main()
{
    sf::RenderWindow window{ sf::VideoMode{500, 500}, "Snow" };
    window.setFramerateLimit(60);

    std::vector<Snowflake> snowflakes;
    snowflakes.reserve(number_of_snowflakes);
    for (int i = 0; i < number_of_snowflakes; ++i)
    {
        float x = generate_random(0, window.getSize().x);
        float y = generate_random(0, window.getSize().y) - 20.F;
        sf::Vector2f start_position(x, y);
        snowflakes.emplace_back(Snowflake(start_position));
    }

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

        for (auto&& snow : snowflakes)
        {
            snow.fall(window.getSize());
        }

        window.clear();
        for (auto&& snow : snowflakes)
        {
            snow.draw(window);
        }
        window.display();
    }
}

Could this still be improved? Sure. I left a few magic numbers behind, and we all know those are bad. The names are more descriptive and better describe what is happening but they could probably be improved. SFML allows for classes to inherit from sf::Drawable and you can fix the syntax so it can be called window.draw(Snowflake); instead of passing a reference to the window object to your class. I would also probably extract the fall() method to a couple private helpers. This would

• hide the implementation details of the public method that don't need to be exposed.
• allow you to simplify some of the functionality. (There's a lot of resetting values to default.)

Answer 2

I see some things that may help you improve your program.

Use objects

The Tween struct is a start, but this code would really benefit from the use of actual objects. Right now, the data for each snowflake is in Tween, but most of the code to manipulate it is in main which makes the code much harder to read and understand than it should be. Even more strangely, each Tween contains its own x coordinate, but a separate array holds the y coordinates!

Reconsider the use of random numbers

The usual advice I give is to use a better random number generator, but here, I think the opposite advice is in order. Because std::generate_canonical calls the generator multiple times to assure sufficient entropy (10 bits in your program), it can have the effect of slowing down the program if called frequently. In this case, the simpler std::uniform_real_distribution would probably be a better choice.

Avoid plain arrays

In most instances in modern C++, a std::array is better than a plain C-style array. Not only does the size accompany the structure, but it also simplifies the use of standard algorithms on the array.

Use SFML more fully

A better use of SFML in this case would be to have a Snowflake class, or even better, a Snowstorm class that contains all of the snowflakes, that that derives from both the sf::Transformable and sf::Drawable classes. Doing so would allow your main loop to be much more readable:

while (window.isOpen()) {
    auto event = sf::Event{};
    while (window.pollEvent(event)) {
        if (event.type == sf::Event::Closed) {
            window.close();
        }
    }
    snowstorm.update(inverseFramerate);
    window.clear();
    window.draw(snowstorm);
    window.display();
}

See the SFML tutorial on creating entities for more details.