A respawn color can be picked randomly

[TheoPannetier]

Context

To respawn a player with a new colour (#612), we need to be able to sample a color at random between red, green, blue.

Test

• [x] Test is ready and on develop

// (628) A random color between red, blue, green can be sampled
  {
    const color r = create_red_color();
    const color g = create_green_color();
    const color b = create_blue_color();

    const int a_seed{42};
    std::mt19937 rng1(a_seed);
    const color a_color = get_random_respawn_color(rng1);
    assert(a_color == r || a_color == g || a_color == b);

    const int another_seed{43}; /// please change seed if draws same color as 42
    std::mt19937 rng2(another_seed);
    const color another_color = get_random_respawn_color(rng2);
    assert(another_color == r || another_color == g || another_color == b);
    assert(a_color != another_color);

    const int same_seed = a_seed;
    std::mt19937 rng3(same_seed);
    const color same_color = get_random_respawn_color(rng3);
    assert(same_color == a_color);
  }

[richelbilderbeek]

@TheoPannetier: I enjoy the enthusiasm of this big test! Using for-loops in tests, however, usually is a code smell. In your suggested case, I feel the test is more focussed on the C++ RNG, which we should assume is correct.

I think the focus of the test is different: two randomly-drawn colors should differ.

Following that, the tests it can be simplied quite a bit. Here I'll suggest how.

One approach is to use std::rand in the back. You should never use std::rand in important code, but AFAICS, we use those random colors only in debugging. That would be my definition of not important.

// Check if the color is actually random
{
   const auto c = get_random_color();
   const auto d = get_random_color();
   assert(c != d); // Chance 1 in 256^3 this fails
}

I think this a great test with using a RNG engine:

// Check if the color is actually random
{
  const int seed{42}
   std::mt19937 rng(seed);
   const auto c = get_random_color(rng); //Will modify RNG
   const auto d = get_random_color(rng); //Will modify RNG
   assert(c != d); // Chance 1 in 256^3 this fails, pick a different seed if needed
}
// Check if the color is actually random yet repeatable
{
   const int seed{42}
   std::mt19937 rng1(seed);
   std::mt19937 rng2(seed);
   const auto c = get_random_color(rng1); //Will modify RNG
   const auto d = get_random_color(rng2); //Will modify RNG
   assert(c == d);
}

[TheoPannetier]

Hi @richelbilderbeek, I definitely agree with getting rid of the for loop in the test, it does feel awkward. I have a preference for the second proposed implementation of the test, but I have a concern – is the chance to pick the same colour twice really that low? The function does not pick among random amounts of RGB, but only between red (i.e. 255, 0, 0), green and blue. So 1/3 chances that the same colour is picked in the test?

Maybe that can simply be circumvented by adjusting the seed?

[richelbilderbeek]

@TheoPannetier

Indeed, if the function is made to be like this ...

is the chance to pick the same colour twice really that low? The function does not pick among random amounts of RGB, but only between red (i.e. 255, 0, 0), green and blue. So 1/3 chances that the same colour is picked in the test?

... I agree with the reasoning completely.

Maybe that can simply be circumvented by adjusting the seed?

Yes! Just pick the right seed :-)

[TheoPannetier]

Indeed, if the function is made to be like this ...

Eh, yes, I agree that only picking between three fixed colours is a bit awkward, but we need this function to regenerate the colour of players after they respawn, and so far we don't know how to deal with random colours. Quite a misnomer then, I'll aim to refactor 🤔

[richelbilderbeek]

@TheoPannetier sounds great! Thanks for keeping up the naming quality :-)

[TheoPannetier]

Hello, sorry but I had to revert the progress on this (see #659). I noticed you guys had overwritten my test with the implementation proposed above by @richelbilderbeek , which avoids over-testing but leads to the wrong solution. As described above we need to draw randomly between green (i.e., 0, 255, 0), blue, or red, rather than a random RGB colour. I have updated the test to require this, and incorporated Richel's critic in it. Your solution also breaks develop (#657) so I reverted it.

[richelbilderbeek]

@TheoPannetier thans for sharing and taking the initiative to fix this ruthlessly! As I've followed the Issues and commits ad PRs closely: Keep up the good job :+1: !

[TheoPannetier]

Re-opening, the solution from https://github.com/tresinformal/drakkar/commit/ffcd0ac382c13b9c80aeb044b3e5e981c0b2b33c seems to be from the version of the test I have overwritten (i.e., not the test on develop), and that does not correspond to what the Issue tries to address