Some stuff that is bothering me, but would be a nightmare to fix

[PowerWyrm]

Here's a simple example:

bool multiply_monster(struct chunk *c, const struct monster *mon)
{
...
become_aware(child);
...
}

with:

void become_aware(struct monster *mon)
{
...
if (square_isseen(cave, obj->grid))
...
if (! monster_carry(cave, mon, given))
...
square_delete_object(cave, obj->grid, obj, false, false);
...
}

Half of the code assumes there is only one chunk "cave" and the other half takes into account the possibility of more. This doesn't feel right. Now imagine you're in the cave generation code where for some profiles there are multiple chunks generated, you could end up with:

generate(c) -> calls f() -> calls g(cave != c)

then you would be in big trouble...

A thorough approach would be to remove all occurences of "cave" and replace them with a "struct chunk *c" parameter in the functions. For example:

bool multiply_monster(struct chunk *c, const struct monster *mon)
{
...
become_aware(c, child);
...
}

with:

void become_aware(struct chunk *c, struct monster *mon)
{
...
if (square_isseen(c, obj->grid))
...
if (! monster_carry(c, mon, given))
...
square_delete_object(c, obj->grid, obj, false, false);
...
}

but of course that would be an insane amount of work.

I know the feeling, I've been through this with my variant which has a similar problem: since it's multiplayer, the "cave" parameter is saved on each player, but most functions use "struct player *p" as a parameter and then use p->cave inside, while some functions use a double parameter signature (struct player *p, struct chunk *c)... and of course I had to track every occurence of p->cave and replace them with "c". And at this time there are still some intertwined functions that don't use a cave parameter inside functions that use one and I fix this as I discover them...

[backwardsEric]

It's a problem with both the player and cave globals - some functions have been modified to expect struct chunk * or struct player * arguments but end up calling functions that do not and internally use the globals. Given that it's fairly widespread, it's likely less daunting to fix it piecemeal - working up from the lowest levels where those globals should never be accessed.

The example of the multiple chunks that can be in use during cave generation leading to potentially inconsistent results isn't particularly relevant. At least in Vanilla, the global cave is NULL during cave generation (it's set to that by prepare_next_level() at generate.c:1381 and then reset to point to the fully generated cave at points later in prepare_next_level()) so any use of a function that accesses that global without some guard to handle it being NULL is going to crash and burn (thus this issue, https://github.com/NickMcConnell/Fir...and/issues/223 , with First Age Angband and a contributor to this one, https://github.com/NickMcConnell/Fir...and/issues/188 ).

[Nick]

I feel like this is a bit of a balancing act. On one hand, any function with 'player' or 'cave' probably "should" take a struct player * or struct chunk * argument. On the other hand, giving a whole lot of functions an extra one or two arguments that are only ever going to take one value is a lot of work and makes the code harder to read.

I think that adding the arguments when we're working on code and it seems like a good idea is probably enough, rather than doing a big seek out and change program. I did one of those for replacing single coordinates with struct grids, so it's possible, but I feel like that made the code unambiguously better and this doesn't.

Of course, I'm not maintaining a variant with multiple players

[Nick]

OK, having gone some way down the road of fixing this, I'm looking to simplify and clarify wherever possible. Here are some thoughts:

• My belief is that any function that can only sensibly be called while character_dungeon is true need not take a struct chunk * argument, as it will only ever use the cave global. The first instance of this I have is the monster AI (all of mon-move.c), but there are others - probably all monster group behaviour, for example.
• Watch out for functions which should have a particular argument structure (the functions in mon-predicate.c and most of the functions in cave-square.c, for example).

[Pete Mack]

Nick--what about Single Combat?

[Nick]

Nick--what about Single Combat?

I don't think it's related to this issue, unless you're seeing something I'm not.

[Nick]

I know the feeling, I've been through this with my variant which has a similar problem: since it's multiplayer, the "cave" parameter is saved on each player, but most functions use "struct player *p" as a parameter and then use p->cave inside, while some functions use a double parameter signature (struct player *p, struct chunk *c)... and of course I had to track every occurence of p->cave and replace them with "c". And at this time there are still some intertwined functions that don't use a cave parameter inside functions that use one and I fix this as I discover them...

In view of this, what do you think about prepare_next_level() losing its struct chunk ** argument, since it is always called with first argument &cave ? It is also always called with second argument the player global - is this the correct thing for you?

[PowerWyrm]

For PWMAngband, prepare_next_level() is different since it's called only when the cave doesn't exist and therefore uses a "struct chunk *prepare_next_level(struct player *p)" prototype.

struct chunk *c = chunk_get(&p->wpos);

if (!c) c = prepare_next_level(p);
else { cave_illuminate and other stuff related to "c"... }