Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old July 23, 2021, 10:22   #1
PowerWyrm
Prophet
 
PowerWyrm's Avatar
 
Join Date: Apr 2008
Posts: 2,891
PowerWyrm is on a distinguished road
Some stuff that is bothering me, but would be a nightmare to fix

Here's a simple example:

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

Code:
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:

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

Code:
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...
__________________
PWMAngband variant maintainer - check http://powerwyrm.monsite-orange.fr (or http://www.mangband.org/forum/viewforum.php?f=9) to learn more about this new variant!
PowerWyrm is offline   Reply With Quote
Old July 23, 2021, 18:50   #2
backwardsEric
Adept
 
Join Date: Aug 2019
Posts: 208
backwardsEric is on a distinguished road
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 ).
backwardsEric is offline   Reply With Quote
Old July 23, 2021, 22:47   #3
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 56
Posts: 9,117
Donated: $60
Nick will become famous soon enough
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
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old July 27, 2021, 08:16   #4
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 56
Posts: 9,117
Donated: $60
Nick will become famous soon enough
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).
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old July 27, 2021, 11:46   #5
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 6,539
Donated: $40
Pete Mack is on a distinguished road
Nick--what about Single Combat?
Pete Mack is offline   Reply With Quote
Old July 27, 2021, 12:50   #6
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 56
Posts: 9,117
Donated: $60
Nick will become famous soon enough
Quote:
Originally Posted by Pete Mack View Post
Nick--what about Single Combat?
I don't think it's related to this issue, unless you're seeing something I'm not.
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old August 4, 2021, 22:56   #7
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 56
Posts: 9,117
Donated: $60
Nick will become famous soon enough
Quote:
Originally Posted by PowerWyrm View Post
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?
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old August 5, 2021, 09:35   #8
PowerWyrm
Prophet
 
PowerWyrm's Avatar
 
Join Date: Apr 2008
Posts: 2,891
PowerWyrm is on a distinguished road
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.

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

if (!c) c = prepare_next_level(p);
else { cave_illuminate and other stuff related to "c"... }
__________________
PWMAngband variant maintainer - check http://powerwyrm.monsite-orange.fr (or http://www.mangband.org/forum/viewforum.php?f=9) to learn more about this new variant!
PowerWyrm is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Where is stuff? Speusippus Vanilla 13 January 24, 2011 22:55
Too much stuff. Zappa AAR 26 June 16, 2010 18:04
Should I be bothering to collect loot to sell? BlackFlame Vanilla 14 October 5, 2009 08:36
[Z2.74c] (Nightmare mode) - Suicide's Mission quaxocal AAR 7 October 24, 2008 19:24
[Z] Anyone ever beat the nightmare or ironman modes? quaxocal Variants 8 July 26, 2007 19:18


All times are GMT +1. The time now is 13:55.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2021, vBulletin Solutions Inc.