Skip to content

Commit

Permalink
bugfix random_shuffle is removed in C++17 - replace with std::shuffle (…
Browse files Browse the repository at this point in the history 
…#7575)
  • Loading branch information
@ofTheo
ofTheo committed Jul 21, 2023
1 parent 4b5632a commit cce8428
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion libs/openFrameworks/utils/ofUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <algorithm>
#include <sstream>
#include <type_traits>
#include <random>

/// \section Elapsed Time
/// \brief Reset the elapsed time counter.
Expand Down Expand Up @@ -225,7 +226,8 @@ int ofGetWeekday();
/// \sa http://www.cplusplus.com/reference/algorithm/random_shuffle/
template<class T>
void ofRandomize(std::vector<T>& values) {
random_shuffle(values.begin(), values.end());
//switch from random_shuffle ( removed in some C++17 impl )
std::shuffle(values.begin(), values.end(), std::default_random_engine(0));
}

/// \brief Conditionally remove values from a vector.
Expand Down

9 comments on commit cce8428

@artificiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original random_shuffle() was seeded by the implementation-dependant srand() which is called at least once in ofSeedRandom() within of::priv::initutils(). this replacement uses a std::default_random_engine with a constant seed of 0, so the shuffle will be predictable (which is probably not the vernacular expectation of a "random" process). calling a std::random_device in lieu of the seed is typically how unpredictable randomness is obtained:

std::random_device rd;
std::shuffle(values.begin(), values.end(), std::default_random_engine(rd()));

as as stop-gap measure the code above will correctly replace the srand-seeded random_shuffle, albeit not optimally as it generates a new seed at every ofRandomize() call, which is wasteful as the default_random_engine only needs 1 seed. perhaps ofSeedRandom() should instantiate a random device and store a static of::utils::seed value that gets re-used? and if a value is passed, it set the of::utils::seed, in parallel so what happens with srand()?

(and as a matter of fact having a function called ofRandomize() is not ideal... perhaps it should be deprecated in favour of ofShuffle(), like the std::shuffle dropped the random_ part, as the randomness is managed elsewhere...)

@ofTheo
Copy link
Member Author

@ofTheo ofTheo commented on cce8428 Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly 0 means use time as the seed, so it is random and not a single seed.
Trying to find a reference for that - I'll see if I can dig it up ( but if that's not correct we should fix )

@artificiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i double-checked and the (0) version produces predictable results:

void ofApp::setup(){
	std::vector<int> v {1,2,3,4,5,6,7,8};
	ofRandomize(v);
	for (auto & i: v) ofLogNotice("1") << i;
}

gives the same

[notice ] 1: 7
[notice ] 1: 3
[notice ] 1: 8
[notice ] 1: 2
[notice ] 1: 5
[notice ] 1: 4
[notice ] 1: 1
[notice ] 1: 6

re-thinking about it, it should definitely be fixed not by generating a seed within ofRandomize() like I did above but by augmenting ofSeedRandom with a random device, as we will probably want to update other OF random utils with modern random stuff, which will need such a seed instead of depending on srand. (and it is important to keep the current model of being able to choose between "unpredictable randomness" and "reproducible randomness" by passing a desired seed)

while we're at it, ofSeedRandom() is a confusing name (the "seed" part is probably thought as the verb), especially as it radically changes behaviour between the no-argument ("random" random seed) and the argument version.

proposal to deprecate ofSeedRandom and ride as close as possible to the std approach and separating the seed store/recall from the gen with something like:

void ofSetRandomSeed(unsigned int seed) // store the OF seed
unsigned int ofGetRandomSeed() // returns the OF seed
unsigned int ofAdvanceRandomDevice() // advance the internal (static?) OF random device and return the next value

generating a default random seed in of::priv::initutils() then means ofSetRandomSeed(ofAdvanceRandomDevice()) and users wishing for a specific seed will simply overwrite it with a call to ofSetRandomSeed(12212). functions relying on the seed will retrieve it with ofGetRandomSeed()

@artificiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in order to facilitate transitioning

void ofSetRandomSeed()

could call ofSetRandomSeed(ofAdvanceRandomDevice()), but should be deprecated-at-birth as it's a bad shortcut

@ofTheo
Copy link
Member Author

@ofTheo ofTheo commented on cce8428 Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!
Thanks @artificiel.
I think deprecate makes sense (but we'll need it for a while).

Want to do a PR with your above proposal?

Could ofAdvanceRandomDevice() be ofGetNewRandomSeed()

Advance reads a bit like expert instead of new / increment

@dimitre
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case it helps something take a look at this PR @artificiel
#7572

@artificiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimitre mt19937 is meaningful when used directly as a generator, but in the case of providing a single seed to the app instance, I don't think the distribution matters so much. (but perhaps we want to offer different random generators within ofUtils, in which case the 19937 might be relevant). but in any case we don't want a new seed at every ofRandomize() call.

@ofTheo I can wrap a PR but not immediately. I will also take a look at other random stuff to make sure we don't paint ourselves in a corner by moving too fast. as far as the naming, "advance" refers to the principle of the generator which literally increments its state each time it's called. perhaps it's more logical to offer an interface to a generic ofRandomDevice, and call it directly? so

unsigned int ofRandomDevice() // equivalent to the () operator of random_device

then

ofSetRandomSeed(ofRandomDevice()) // sets the seed to a random value

@artificiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've opened a discussion #7579

@artificiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've appended some concrete code to #7579

Please sign in to comment.