Skip to content
Permalink
Browse files

Sequence many, many random calls whose order was undefined

Also add some documentation about these issues for other developers.
Basically, the call order for functions that are present in an
arithmetic expression or a constructor is undefined and can vary
(leading to different effects of seed).  It would be foolish to think
that this got everything, but I hope that this did get everything for
dungeon generation per se; given the complexity of this problem getting
real seeding for the gameplay UI may be a bit far off.

Two areas of concern:
* random_var is clever and fun, but also inherently not a good fit for
  wanting well-sequenced rng calls, because operators inherit this
  undefinedness.
* I didn't really try to address div_rand_round in this commit, which is
  all over the place and potentially hidden in many return calls.
  • Loading branch information...
rawlins committed Jan 29, 2019
1 parent 14c3f58 commit cdddf7c89e994c9635ff92de1d7f98a855bda0c5
@@ -0,0 +1,133 @@
# Developer guidelines for using random numbers in DCSS

Crawl attempts to be deterministically seeded: given a seed (that may be
chosen randomly), dungeon generation (and ideally, everything) is deterministic
thereafter. It does this by using a random number generator that has
"reproducible" behavior,
[http://www.pcg-random.org/](http://www.pcg-random.org/). This leads to certain
practices and caveats that help maintain the usefulness of seeding.

## 1. Background

First, it may help to know a bit about how our RNG works. Given some seed,
a PCG generator generates a sequence of 32-bit unsigned integers, that is
deterministic but hard to predict. You can think of this like a code book:
for a given seed, the integer at index i will always be the same. The various
random functions (e.g. `random2`) then package information from these integers
in a way that is a bit more usable while programming. This means that the state
of the RNG is equivalent to the *number of draws from the generator*. If this
number of draws diverges for any reason between e.g. two versions of crawl,
then the randomization behavior will sharply diverge, probably permanently,
very soon after that. In most cases, the results of a single random draw at
some point in code execution will impact the number of random draws at a later
point. For example, during dungeon generation, the position of a vault might
be chosen by two `random2` calls (which usually does correspond to two random
draws from the raw generator). The position of this vault will then impact what
other vaults can be placed in the same layout, which is checked (roughly) by
randomly trying vaults until one fits -- which will lead to varying numbers of
rng draws.

The dungeon generator RNGs are partitioned by branch, and separate from the UI
and gameplay RNGs. However, vault choice is not fully independent across branch,
so the order of branch generation matters quite a bit. Given a fixed order, a
seed *should* fully determine a dungeon, but this mapping is quite delicate.

## 2. Breaking seed mappings

The following changes (which are often desireable changes) can break the seed
to dungeon mappings, in some way:

* adding or removing a vault
* adding or removing monster types or uniques
* adding or removing items, unique or otherwise

It is extremely hard to predict how big the break will be -- it may be small,
or it may result in an entirely different dungeon. (For example, changing D:1
arrival vaults will likely drastically change the mapping.)

For this reason, seeds should be assumed to be tied to a specific version of
dungeon crawl. Changes that break seeds for stable releases should be avoided
without introducing a new stable release right away -- because online servers
usually update right away, this will cause seeds to diverge between online and
offline versions.

All bets are off for trunk seed->dungeon mappings, which might at best be
specific to a single commit. Similarly, upgrading a game (trunk or otherwise)
where the dungeon is not pregenerated will lead to divergence from *both* seed
to dungeon mappings.

## 3. Specific coding guidelines and caveats

The biggest challenge in ensuring stable seed to dungeon mappings involves
ensuring that random numbers are drawn from the generators in the same order.
There are two specific areas where care is required.

### 3.1 random call evaluation order

C++, in many cases, does not guarantee stable evaluation order for function
calls, and this can result in different behaviors across compilers, OSs, and
potentially even execution instances. For background, see:

* [https://en.cppreference.com/w/cpp/language/eval_order](https://en.cppreference.com/w/cpp/language/eval_order)
* [https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP50-CPP.+Do+not+depend+on+the+order+of+evaluation+for+side+effects](https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP50-CPP.+Do+not+depend+on+the+order+of+evaluation+for+side+effects)

Because the RNG system works by side-effect, this means that there are various
cases where the compiler doesn't guarantee the order of random call side-effect.
The two most important cases to watch out for are:

* order of calls within arithmetic expressions, and
* order of calls within comma-separated arguments to functions, including
constructors.

For example, expressions like the following *does not have a guaranteed order of
function calls* in any version of C or C++:

const int x = 10 + random2(5) - random2(3); // bad
auto y = f(random2(5), random2(3)); // bad

A particularly tempting paradigm that was common in dcss code before seeding
that instantiates the latter is:

const coord_def pos(random2(5), random2(5)); // bad

This is not a theoretical concern: we have found that the order of calls in
cases like these does vary in practice between different compilers on different
OSs, and so any of these calls might get different results. (Because of the
layers of abstraction, the results of swapping call order are not as simple as
swapping the random number results, even if the calls appear the same.)

In order to get stable behavior, what you need to do is ensure a "sequence
point" between any random number calls (see the links above). This unfortunately
leads to less compact code, and cases where `const`ing is not possible. For
example, here is a better version of two of the above cases:

int x = 10 + random2(5); // sequence point
x -= random2(3);

coord_def pos;
pos.x = random2(5);
pos.y = random2(5);

Even operators that are commutative (e.g. `+`), because of the way `random2` is
implemented, in principle aren't safe to have in the same expression.

What sorts of expressions *do* ensure evaluation order? See the above links for
the gory details, but the main safe one is boolean expressions, which do have a
stable order. This does include `? :` expressions. So for example the following
is ok (at least in terms of RNG call sequencing):

if (x_chance_in_y(1,5) && coinflip()) // stable sequencing
do_something();

For everything else, when in doubt, break it into multiple expressions
that are sequenced by `;`s.

### 3.2 choosing from lists

When you are randomly choosing an item from a list (e.g. by randomly picking
an index in the list range), be careful about the underlying list order. This
is a case where the RNG behavior can be fine, but the list order itself could
lead to divergence. For example, when choosing from a list of files provided
by the OS, you cannot assume that all OSs and OS versions will give you these
files in a stable order - sort it yourself.

@@ -87,8 +87,10 @@ static coord_def _place_feature_near(const coord_def &centre,
coord_def cp = INVALID_COORD;
for (int i = 0; i < tries; ++i)
{
cp = centre + coord_def(random_range(-radius, radius),
random_range(-radius, radius));
coord_def offset;
offset.x = random_range(-radius, radius);
offset.y = random_range(-radius, radius);
cp = centre + offset;

if (cp == centre || !in_bounds(cp))
continue;
@@ -1242,7 +1244,8 @@ static void _update_abyss_terrain(const coord_def &p,
if (feat == DNGN_FLOOR && in_los_bounds_g(rp))
{
cloud_type cloud = _cloud_from_feat(currfeat);
int cloud_life = (_in_wastes(abyssal_state.major_coord) ? 5 : 2) + random2(2);
int cloud_life = _in_wastes(abyssal_state.major_coord) ? 5 : 2;
cloud_life += random2(2); // force a sequence point, just in case
if (cloud != CLOUD_NONE)
check_place_cloud(_cloud_from_feat(currfeat), rp, cloud_life, 0, 3);
}
@@ -1772,10 +1775,13 @@ static bool _spawn_corrupted_servant_near(const coord_def &pos)
// Thirty tries for a place.
for (int i = 0; i < 30; ++i)
{
const int offsetX = random2avg(4, 3) + random2(3);
const int offsetY = random2avg(4, 3) + random2(3);
const coord_def p(pos.x + random_choose(offsetX, -offsetX),
pos.y + random_choose(offsetY, -offsetY));
int offsetX = random2avg(4, 3);
offsetX += random2(3); // force a sequence point between random calls
int offsetY = random2avg(4, 3);
offsetY += random2(3); // ditto
coord_def p;
p.x = pos.x + random_choose(offsetX, -offsetX);
p.y = pos.y + random_choose(offsetY, -offsetY);
if (!in_bounds(p) || actor_at(p))
continue;

@@ -271,9 +271,10 @@ static armour_type _acquirement_body_armour(bool divine)
{
// Using an arbitrary legacy formula, do we think the player doesn't care
// about armour EVP?
const bool warrior = random2(_skill_rdiv(SK_SPELLCASTING, 3)
+ _skill_rdiv(SK_DODGING))
< random2(_skill_rdiv(SK_ARMOUR, 2));
int light_pref = _skill_rdiv(SK_SPELLCASTING, 3);
light_pref += _skill_rdiv(SK_DODGING);
light_pref = random2(light_pref);
const bool warrior = light_pref < random2(_skill_rdiv(SK_ARMOUR, 2));

vector<pair<armour_type, int>> weights;
for (int i = ARM_FIRST_MUNDANE_BODY; i < NUM_ARMOURS; ++i)
@@ -1350,11 +1351,12 @@ int acquirement_create_item(object_class_type class_wanted,
{
// New gold acquirement formula from dpeg.
// Min=220, Max=5520, Mean=1218, Std=911
int quantity_rnd = roll_dice(1, 8); // ensure rnd sequence points
quantity_rnd *= roll_dice(1, 8);
quantity_rnd *= roll_dice(1, 8);
acq_item.quantity = 10 * (20
+ roll_dice(1, 20)
+ (roll_dice(1, 8)
* roll_dice(1, 8)
* roll_dice(1, 8)));
+ quantity_rnd);
}
else if (class_wanted == OBJ_MISSILES && !divine)
acq_item.quantity *= 5;
@@ -154,8 +154,9 @@ static void _CURSES_equip(item_def *item, bool *show_msgs, bool unmeld)
_equip_mpr(show_msgs, "A shiver runs down your spine.");
if (!unmeld)
{
const int pow = random2(9);
MiscastEffect(&you, nullptr, {miscast_source::wield},
spschool::necromancy, random2(9), random2(70),
spschool::necromancy, pow, random2(70),
"the scythe of Curses", nothing_happens::NEVER);
}
}
@@ -174,8 +175,9 @@ static void _CURSES_melee_effects(item_def* weapon, actor* attacker,
did_god_conduct(DID_EVIL, 3);
if (!mondied && defender->holiness() == MH_NATURAL)
{
const int pow = random2(9);
MiscastEffect(defender, attacker, {miscast_source::melee},
spschool::necromancy, random2(9), random2(70),
spschool::necromancy, pow, random2(70),
"the scythe of Curses", nothing_happens::NEVER);
}
}
@@ -502,7 +504,8 @@ static bool _WUCAD_MU_evoke(item_def *item, bool* did_work, bool* unevokable)

mpr("Magical energy flows into your mind!");

inc_mp(3 + random2(5) + you.skill_rdiv(SK_EVOCATIONS, 1, 3));
const int mp_inc_base = 3 + random2(5);
inc_mp(mp_inc_base + you.skill_rdiv(SK_EVOCATIONS, 1, 3));
make_hungry(50, false, true);

*did_work = true;
@@ -851,8 +854,9 @@ static void _PLUTONIUM_SWORD_melee_effects(item_def* weapon, actor* attacker,
|| !mons_immune_magic(*defender->as_monster())))
{
mpr("Mutagenic energy flows through the plutonium sword!");
const int pow = random2(9);
MiscastEffect(defender, attacker, {miscast_source::melee},
spschool::transmutation, random2(9), random2(70),
spschool::transmutation, pow, random2(70),
"the plutonium sword", nothing_happens::NEVER);

if (attacker->is_player())
@@ -1040,8 +1044,9 @@ static void _SPELLBINDER_melee_effects(item_def* weapon, actor* attacker,
if (defender->antimagic_susceptible()
&& !mondied)
{
const int pow = random2(9);
MiscastEffect(defender, attacker, {miscast_source::melee},
spschool::random, random2(9), random2(70),
spschool::random, pow, random2(70),
"the demon whip \"Spellbinder\"", nothing_happens::NEVER);
}
}
@@ -708,8 +708,11 @@ void bolt::apply_beam_conducts()
switch (flavour)
{
case BEAM_DAMNATION:
did_god_conduct(DID_EVIL, 2 + random2(3), god_cares());
{
const int level = 2 + random2(3);
did_god_conduct(DID_EVIL, level, god_cares());
break;
}
default:
break;
}
@@ -2833,7 +2836,9 @@ bool bolt::fuzz_invis_tracer()
return false;

// Apply fuzz now.
coord_def fuzz(random_range(-2, 2), random_range(-2, 2));
coord_def fuzz;
fuzz.x = random_range(-2, 2);
fuzz.y = random_range(-2, 2);
coord_def newtarget = target + fuzz;

if (in_bounds(newtarget))
@@ -5319,8 +5324,8 @@ mon_resist_type bolt::apply_enchantment_to_monster(monster* mon)
obvious_effect = true;
if (YOU_KILL(thrower))
{
did_god_conduct(DID_DELIBERATE_MUTATING, 2 + random2(3),
god_cares());
const int level = 2 + random2(3);
did_god_conduct(DID_DELIBERATE_MUTATING, level, god_cares());
}
return MON_AFFECTED;

@@ -5330,8 +5335,8 @@ mon_resist_type bolt::apply_enchantment_to_monster(monster* mon)
obvious_effect = true;
if (YOU_KILL(thrower))
{
did_god_conduct(DID_DELIBERATE_MUTATING, 2 + random2(3),
god_cares());
const int level = 2 + random2(3);
did_god_conduct(DID_DELIBERATE_MUTATING, level, god_cares());
}
return MON_AFFECTED;

@@ -51,13 +51,18 @@ coord_def random_in_bounds()
{
const coord_def &ul = crawl_view.glos1; // Upper left
const coord_def &lr = crawl_view.glos2; // Lower right

return coord_def(random_range(ul.x, lr.x - 1),
random_range(ul.y, lr.y - 1));
coord_def res;
res.x = random_range(ul.x, lr.x - 1);
res.y = random_range(ul.y, lr.y - 1);
return res;
}
else
return coord_def(random_range(MAPGEN_BORDER, GXM - MAPGEN_BORDER - 1),
random_range(MAPGEN_BORDER, GYM - MAPGEN_BORDER - 1));
{
coord_def res;
res.x = random_range(MAPGEN_BORDER, GXM - MAPGEN_BORDER - 1);
res.y = random_range(MAPGEN_BORDER, GYM - MAPGEN_BORDER - 1);
return res;
}
}

// Coordinate system conversions.
@@ -492,10 +492,18 @@ void make_irregular_box(map_lines& map, int x1, int y1, int x2, int y2,
// -> the box may stick out past these
//

coord_def in_tl(random2(in_x), random2(in_y));
coord_def in_tr(random2(in_x), random2(in_y));
coord_def in_bl(random2(in_x), random2(in_y));
coord_def in_br(random2(in_x), random2(in_y));
coord_def in_tl;
in_tl.x = random2(in_x);
in_tl.y = random2(in_y);
coord_def in_tr;
in_tr.x = random2(in_x);
in_tr.y = random2(in_y);
coord_def in_bl;
in_bl.x = random2(in_x);
in_bl.y = random2(in_y);
coord_def in_br;
in_br.x = random2(in_x);
in_br.y = random2(in_y);
_randomly_force_sum_below(in_tl.x, in_tr.x, size_x - MIN_IRREGULAR_SIZE);
_randomly_force_sum_below(in_bl.x, in_br.x, size_x - MIN_IRREGULAR_SIZE);
_randomly_force_sum_below(in_tl.y, in_bl.y, size_y - MIN_IRREGULAR_SIZE);
Oops, something went wrong.

0 comments on commit cdddf7c

Please sign in to comment.
You can’t perform that action at this time.