Bugs and issues in 4.1.1

[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.

[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

[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?

[PowerWyrm]

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

[PowerWyrm]

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

[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]

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]

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]

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.

[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]

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.

[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.

[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.

[kandrc]

Current top of tree:

Program received signal SIGSEGV, Segmentation fault.
0x000000000047ff76 in remove_contradictory (art=0xa6d3f8) at obj-randart.c:2348
2348                                    art->curses[i] = 0;
(gdb) print i
$1 = 15
(gdb) print *art
$2 = {name = 0x9da108 "of Agorian", 
  text = 0x8b4ee8 "This wondrous suit of fine-linked chain shimmers as though of pure silver.  It stands untouched amidst the fury of the elements, and a power of concealment rests within.", aidx = 97, next = 0xa6d538, tval = 9, 
  sval = 14, to_h = -12, to_d = -13, to_a = 0, ac = 0, dd = 6, ds = 5, 
  weight = 180, cost = 105000, flags = "\000@\000\002", modifiers = {
    0 <repeats 14 times>}, el_info = {{res_level = 0, flags = 2 '\002'}, {
      res_level = 0, flags = 2 '\002'}, {res_level = 0, flags = 2 '\002'}, {
      res_level = 0, flags = 2 '\002'}, {res_level = 0, flags = 0 '\000'}, {
      res_level = 0, flags = 0 '\000'}, {res_level = 0, flags = 0 '\000'}, {
      res_level = 0, flags = 0 '\000'}, {res_level = 0, flags = 0 '\000'}, {
      res_level = 0, flags = 0 '\000'}, {res_level = 0, flags = 0 '\000'}, {
      res_level = 1, flags = 0 '\000'}, {res_level = 0, 
      flags = 0 '\000'} <repeats 13 times>}, brands = 0x0, slays = 0x9d6288, 
  curses = 0x0, level = 40, alloc_prob = 30, alloc_min = 55, alloc_max = 127, 
  created = false, seen = false, everseen = false, activation = 0x0, 
  alt_msg = 0x0, time = {base = 2, dice = 0, sides = 0, m_bonus = 0}}
(gdb) bt
#0  0x000000000047ff76 in remove_contradictory (art=0xa6d3f8)
    at obj-randart.c:2348
#1  0x000000000047ffea in add_ability (art=0xa6d3f8, target_power=181, 
    freq=0x7fffffffd380, data=0x8ba298) at obj-randart.c:2370
#2  0x0000000000480b1f in design_artifact (data=0x8ba298, tv=9, 
    aidx=0x7fffffffd55c) at obj-randart.c:2620
#3  0x0000000000480f45 in create_artifact_set (data=0x8ba298)
    at obj-randart.c:2722
#4  0x00000000004818a3 in do_randart (randart_seed=84580334, create_file=true)
    at obj-randart.c:2919
#5  0x000000000048a35c in do_cmd_accept_character (
    cmd=0x755820 <cmd_queue+704>) at player-birth.c:1159
#6  0x000000000040e054 in process_command (ctx=CMD_BIRTH, 
    cmd=0x755820 <cmd_queue+704>) at cmd-core.c:222
#7  0x000000000040e14d in cmdq_pop (c=CMD_BIRTH) at cmd-core.c:250
#8  0x000000000040e1f6 in cmdq_execute (ctx=CMD_BIRTH) at cmd-core.c:291
#9  0x00000000004a9ff9 in textui_do_birth () at ui-birth.c:1246
#10 0x00000000004b4bd7 in start_game (new_game=false) at ui-game.c:404
#11 0x00000000004b4c49 in play_game (new_game=false) at ui-game.c:428
#12 0x00000000004ed5bd in main (argc=1, argv=0x7fffffffdbf8) at main.c:524
(gdb)

[PowerWyrm]

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?