Make AIJH Actions Reproducible by Using the Games Random Seed

[wichern]

Currently, the actions of AIJH are not reproducible because it uses a different random seed for each execution, resulting in different outcomes every time. I'm unsure whether this behavior is intentional.

This change ensures reproducibility by switching from the standard rand() function to RANDOM.Rand(), which allows for the use of a fixed random seed. With this update, users can replicate results consistently if desired.

If the lack of reproducibility was intentional, feel free to disregard this pull request.

[Flamefire]

I'm not sure we need this and the deterministic RANDOM is a likely more expensive and would make the Async debugging much harder as it now contains logs from the AI which are not relevant to the game.

which allows for the use of a fixed random seed. With this update, users can replicate results consistently if desired.

Which kind of results? I think the only place where this matters is the AI battle

If we do want that I'd rather have a separate RNG (could even be from std::random) for AI which is deterministically seeded as you propose. It should not record the async logs/invocations or at least not influence those from the main game.

[wichern]

Hi @Flamefire

You are right, it only matters for AI battle. I am using them to analyze performance. It would be easier to do if the AI behaviour is reproducible. If you are ok with it, I would update RunProgram() to support a new command line parameter. Only in case someone wants to use a fixed value.

[Flamefire]

For the AI-Battle RunProgram is not used, so that wouldn't help there.

And for regular playing rand() is not only used for AI but also for drawing, which was the initial intention:

// Zufallsgenerator initialisieren (Achtung: nur für Animations-Offsets interessant, für alles andere
    // (spielentscheidende) wird unser Generator verwendet)

i.e.

Init RNG (Attention: Only used for animation offsets...)

Which became outdated as it is also used for sounds and AI.

I suggest to add a global RNG for AI, and maybe even the functions from helpers/random.hpp directly in the AI namespace to forward them using the AI RNG. I.e. something like `T randomValue(...) { return helpers::randomValue(getAiRng(), ...); }

This could be std::minstd_rand or even std::mt19937_64

This can the be seeded by the AI battle CLI and probably also in StartGame

The AI code then needs to be updated to use those new functions.

It is still possible that the results are not fully deterministic as the time when the AI acts might depend on e.g. network packet order. It wasn't written to be deterministic. But I guess it will still work, at least for the AI battle.

[Flamefire]

Looking good. Made a few suggestions. Can you check in general that the template param of randomValue is omitted if possible and the result a const auto if it isn't and const otherwise?

The suggested helper functions should make this much easier and the code more readable.

Suggesting this here because I think it is easier to verify in the original change than to verify this and then the change to the convenience functions again

[Flow86]

are the std rngs defined to have the same randomeness between platforms now? Since for rand() that was implementation specific (that was the reason why we built our own)

[Flamefire]

The rng yes. There might be differences in the distribution implementations leading to slightly different results or something like that. But IMO we don't really need that here: It is reproducible when run on the same platform which is enough for the current use case, isn't it?

[wichern]

In my understanding std::minstd_rand is using a fixed deterministic algorithm which should produce the same values on all platforms. I did not test this though and it is not important for the AI in my opinion.

[Flamefire]

Correct. It is just the distribution classes which are implementation defined. Found a SO post confirming that.

But still yes: Fine for this use case.

[Flamefire]

I have just seen we already have this function in helpers:

Suggested change

	inline unsigned randomIndex(const ContainerT& container)
	{
	RTTR_Assert(!container.empty());
	return randomValue(0u, static_cast<unsigned>(container.size()) - 1u);
	unsigned randomIndex(const ContainerT& container)
	{
	return helpers::getRandomIndex(getRandomGenerator(), container.size());

(template functions don't need to be marked inline)

There is also helpers::getRandomElement which might be useful in AI code too but that for later.

[Flamefire]

I think we have in general:

• rand() % N == X --> AI::random(N) for X < N
• rand() % N != X --> !AI::random(N) for X < N

so:

Suggested change

	if((inventory.people[Job::Private] < 15 || AI::random())
	if((inventory.people[Job::Private] < 15 || AI::random(3))

Can you fix this in all other places, please?

[wichern]

Shouldn't this be random(10u)?

[Flamefire]

Yes. See my previous comment which includes a "recipe" for translating those. Didn't want to annotate all of them to avoid flooding this PR too much ;-)