Bugs and issues in 4.1.1

[Nick]

I see what's causing that crash, will be easy to fix.

In fact, I don't like the new stair placement at all:
- too many levels have all stairs in the same small area
- too many stairs placed inside GVs in nasty places or outside vaults in unreachable enclosed areas
- many classic rooms that usually had stairs (and only stairs) are now completely empty
- stairs placed in the same room templates over and over

Also why not only doing stair placement for up staircases?

Yes, fair enough, I'll have another go.

[luneya]

In fact, I don't like the new stair placement at all:
- too many levels have all stairs in the same small area
- too many stairs placed inside GVs in nasty places or outside vaults in unreachable enclosed areas
- many classic rooms that usually had stairs (and only stairs) are now completely empty
- stairs placed in the same room templates over and over

Also why not only doing stair placement for up staircases?

Actually, if the idea is to not have the player start a level in a death trap, we don't really want to be modifying stair placement at all. A better solution would be to base things on the code that decides where to place @ when entering by recall/deep descent/tele level. Make that algorithm place @ in a safe location, and then spawn an appropriate stair under @ in the special case where the level was entered by stairs and the connected stairs option is turned on.

[PowerWyrm]

Actually, if the idea is to not have the player start a level in a death trap, we don't really want to be modifying stair placement at all. A better solution would be to base things on the code that decides where to place @ when entering by recall/deep descent/tele level. Make that algorithm place @ in a safe location, and then spawn an appropriate stair under @ in the special case where the level was entered by stairs and the connected stairs option is turned on.

That's actually a good idea... I'll steal that for PWMAngband, where the positions for people going up/down/randomly are stored into the cave->join structure, and so will be trivial to implement.

[Nick]

Actually, if the idea is to not have the player start a level in a death trap, we don't really want to be modifying stair placement at all. A better solution would be to base things on the code that decides where to place @ when entering by recall/deep descent/tele level. Make that algorithm place @ in a safe location, and then spawn an appropriate stair under @ in the special case where the level was entered by stairs and the connected stairs option is turned on.

Yes you are, indeed, entirely correct. I'm going to back out the entire stair change and redo based on danger.

[PowerWyrm]

Yes you are, indeed, entirely correct. I'm going to back out the entire stair change and redo based on danger.

I tried to do some changes in PWMAngband while keeping the old system. Basically, I've kept the idea of using three walls in the cardinal directions to decide to place stairs or not, but with the following changes:

- replacing square_is_wall() by feat_is_wall() in square_num_walls_adjacent() to consider only "real" walls and not other features like doors
- coding square_suits_stairs() using the same code as _find_in_range(), which is more efficient than just a for (i=1 to 1000) loop (this loop was in fact the reason while some stairs were placed in the middle of rooms, since it didn't consider enough possible locations before lowering the number of walls)
- using square_suits_stairs() in alloc_stairs() with walls = 3, then decreasing the value if no suitable location is found (which I think should never happen)

Looking at the result, this works fine except for a few cases where stairs can be placed in dangerous locations:
- vaults
- starburst rooms
- huge rooms

Stairs should probably be placed in vaults only if the vault was designed to have stairs (aka in the vault.txt file). In this case, it's easy to fix this by not allowing SQUARE_VAULT squares to be chosen. For other rooms, it would be easy to add a similar flag (SQUARE_NO_STAIRS?) and do the same.

[PowerWyrm]

While stairscumming to test my changes about stair placement, I experienced a strange crash during dungeon generation. Looking at what happened, it seems that the player was placed out of bounds when generating a labyrinth level. Since the previous level was persistent, I checked the code to see if such case could happen, and indeed there's a bug that occurs in this particular case.

With persistent levels, the size of the previous level is kept and passed to the dungeon generation routine:

struct chunk *labyrinth_gen(struct player *p, int min_height, int min_width)

In this case, the previous level was a normal level, so min_height/min_width are equal to regular dungeon height/width. The code then creates a small labyrinth:

int h = 15 + randint0(p->depth / 10) * 2;
int w = 51 + randint0(p->depth / 10) * 2;

With persistent levels, height and width are maxed:

/* Enforce minimum dimensions */
h = MAX(h, min_height);
w = MAX(w, min_width);

Now the code actually generates the level in labyrinth_chunk(), but considers that the outer walls don't belong to the labyrinth:

struct chunk *c = cave_new(h + 2, w + 2);

which for persistent levels is actually bigger than the size of the dungeon.

A simple fix would probably be:

/* Enforce minimum dimensions */
h = MAX(h, min_height - 2);
w = MAX(w, min_width - 2);

[PowerWyrm]

Stairs should probably be placed in vaults only if the vault was designed to have stairs (aka in the vault.txt file). In this case, it's easy to fix this by not allowing SQUARE_VAULT squares to be chosen. For other rooms, it would be easy to add a similar flag (SQUARE_NO_STAIRS?) and do the same.

You clearly don't want stairs in vaults such as "High Temple of the Underworld"...

[PowerWyrm]

A simple fix would probably be:

/* Enforce minimum dimensions */
h = MAX(h, min_height - 2);
w = MAX(w, min_width - 2);

In fact there is more: to generate a proper labyrinth, height and width have to be odd numbers, which is not the case with persistent levels, since classic dungeon size is 66x198. So the previous formula has to be adapted so that h & w are always odd numbers. For example:

    /* Enforce minimum dimensions */
    h = MAX(h, min_height);
    w = MAX(w, min_width);

    /* Enforce maximum dimensions */
    h = MIN(h, z_info->dungeon_hgt - 3);
    w = MIN(w, z_info->dungeon_wid - 3);

[PowerWyrm]

The "Mirrored script "B"s" vault is not centered, probably not intended.

[PowerWyrm]

Oops! In commit b56eea2, the weapon slot is not restored if num == max_num.

[Nick]

While stairscumming to test my changes about stair placement, I experienced a strange crash during dungeon generation. Looking at what happened, it seems that the player was placed out of bounds when generating a labyrinth level. Since the previous level was persistent, I checked the code to see if such case could happen, and indeed there's a bug that occurs in this particular case.

I don't get this, because the persistent levels option uses only the modified profile; all the other *_gen() functions have a line like this:

	/* No persistent levels of this type for now */
	if (OPT(p, birth_levels_persist)) return NULL;

The reason I did this is precisely because I was having the sort of issues you're finding, and I wanted proof of concept. Are you sure you had persistent levels on?

[Sky]

is this any use to you ?

Pile integrity failure at obj-pile.c:143

Guilty object
=============
Name: Cure Critical Wounds

Guilty pile
=============
Name: Cure Critical Wounds
Name: Speed
Name: Restore Life Levels
Name: Restore Mana
Name: & Pair~ of Leather Sandals
Name: Teleportation
Name: & Small Metal Shield~
Name: & Fur Cloak~
Name: [Incantations and Illusions]
Name: Magic Mapping
Name: [Resistances of Scarabtarices]
Name: [Sorcery and Evocations]
Name: & Elfstone~
Name: Metal Lamellar Armour~
Name: Banishment
Name: & Lantern~
Name: [Mordenkainen's Escapes]
Name: [Raal's Tome of Destruction]
Name: Vigor
Name: & Long Bow~
Name: Enlightenment
Name: & Blade~ of Chaos
Name: [Conjurings and Tricks]
Name: & Set~ of Leather Gloves
Name: Healing
Name: [Tenser's Transformations]
Name: & Mithril Arrow~
Name: & Arrow~
Name: & Arrow~
Name: & Seeker Arrow~
Name: & Band~
Name: & Arrow~
Name: & Seeker Arrow~
Name: & Arrow~
Name: & Metal Cap~
Name: [Magic for Beginners]
Name: & Pike~
Name: Cure Critical Wounds

[PowerWyrm]

I don't get this, because the persistent levels option uses only the modified profile; all the other *_gen() functions have a line like this:

	/* No persistent levels of this type for now */
	if (OPT(p, birth_levels_persist)) return NULL;

The reason I did this is precisely because I was having the sort of issues you're finding, and I wanted proof of concept. Are you sure you had persistent levels on?

Well I removed that restriction locally to test the persistent levels for other profiles in debug mode

So yeah, it cannot happen normally and it's just a potential issue.

[Nick]

is this any use to you ?

Some. I'm guessing you were picking up !CCW - any idea how many and how many were in your pack already? Anything else interesting happening at the time?

[Sky]

40 CCW in backpack, but as far as can recall (game crashed , after all) there were no other CCW around. i was on a rune of protection, shooting Polyphemus. he was closing in, so i was holding down my shoot button