Implementing the restructure changes

[Nick]

Windows 7. Game crashes as soon as I press L (angband-vTNG-1116-g0a1d754).

OK, we'll need to do some testing in Windows.

Prices - I'm seeing 3 for food and oil, 6 for torches, confirm the 150 for whips. I don't know how wrong that is, or what's caused it.

[PowerWyrm]

Another annoying change concerning effect handling: before, the return value for each effect denoted the fact that the effect was applied or not; now, it simply denotes the fact that the effect is handled or not. Basically every effect handler returns TRUE. This means that pressing ESC while doing something with an object that has an effect on something (scroll of ID, enchant) wastes the object.

Seems also that ID by use is broken: I aimed an unid wand (magic missiles) at something, killed it and the wand didn't get identified.

[Nick]

Another annoying change concerning effect handling: before, the return value for each effect denoted the fact that the effect was applied or not; now, it simply denotes the fact that the effect is handled or not. Basically every effect handler returns TRUE. This means that pressing ESC while doing something with an object that has an effect on something (scroll of ID, enchant) wastes the object.

All right, that's a bug, and probably a result of the fact I was glad just to get the effect code basically working. There's no need to report the extreme uselessness of object effect descriptions

Seems also that ID by use is broken: I aimed an unid wand (magic missiles) at something, killed it and the wand didn't get identified.

Yeah, there are issues with ID by use - the whole basics of the ID code has been rewritten to prepare for rune-based ID, and I didn't get it quite right. Please keep pointing out places where it isn't working.

You might like to note that I had already filed those bugs

[Nick]

In V, it's indeed the FLOOR flag that must be used. Clearly:
- remove square_isboring() and use square_isinteresting() in the targeting code
- use square_isfloor() instead of square_isinteresting() in illumination/darkening code

OK, agreed, this will happen. The one other place I will use square_isinteresting() is in run_test. Currently it uses square_noticeable, which just checks the INTERESTING flag, and I think it's the right thing for the purpose of interrupting a run.

In variants such as PWMAngband that has a wilderness, it's not so simple. There need to be another flag for illumination/darkening code, which has to be set on floor squares, as well as non-floor squares (water, grass, ...).

It depends - in FAangband, grass has the FLOOR flag, water doesn't get lit (I think - it certainly becomes a question of how you're handling the terrain).

[Nick]

Checkpoint 1

OK, these issues are all fixed in development (this message is largely for future Nick, who from past experience has a worse memory even than present Nick).

[PowerWyrm]

Just a little gameplay change I've noted, which is probably due to changing "do_poly" from integer to boolean. This change even occured before 3.5, so it's unrelated to the restructure.

Prior to 3.5, "do_poly" was an integer, the polymorphing effect was using "p_ptr->lev" to compute the chance of a monster resisting it:
- dam = p_ptr->lev
- do_poly = dam
- savelvl = randint1(MAX(1, do_poly - 10)) + 10

In 3.5, "do_poly" was changed into a boolean without anything else changed. This made the formula bugged (the second affectation probably turns do_poly to 1 on most systems), making savelvl always equal to 11.

In the latest code, the formula was simply changed to "savelvl = 11".

This change wasn't probably intended?

[Nick]

Just a little gameplay change I've noted, which is probably due to changing "do_poly" from integer to boolean. This change even occured before 3.5, so it's unrelated to the restructure.

Prior to 3.5, "do_poly" was an integer, the polymorphing effect was using "p_ptr->lev" to compute the chance of a monster resisting it:
- dam = p_ptr->lev
- do_poly = dam
- savelvl = randint1(MAX(1, do_poly - 10)) + 10

In 3.5, "do_poly" was changed into a boolean without anything else changed. This made the formula bugged (the second affectation probably turns do_poly to 1 on most systems), making savelvl always equal to 11.

In the latest code, the formula was simply changed to "savelvl = 11".

This change wasn't probably intended?

Yes, indeed. This particular effect has a long and complicated history, and it's not clear which bits are intentional. It does need sorting out.

[PowerWyrm]

Another glitch due to the restructure in project_m():

if (monster_handler != NULL)
  monster_handler(&context);

/* Force obviousness for certain types if seen. */
if (gf_force_obvious(typ) && context.seen)
  context.obvious = TRUE;

Before the restructure, the "obviousness when seen" line was the first line processed for each type. Therefore these two bits of code should be reversed. Check for example project_monster_handler_OLD_DRAIN():

static void project_monster_handler_OLD_DRAIN(project_monster_handler_context_t *context)
{
	if (context->seen) {
		rf_on(context->l_ptr->flags, RF_UNDEAD);
		rf_on(context->l_ptr->flags, RF_DEMON);
	}
	if (monster_is_nonliving(context->m_ptr->race)) {
		context->hurt_msg = MON_MSG_UNAFFECTED;
		[COLOR="Red"]context->obvious = FALSE;[/COLOR]
		context->dam = 0;
	}
}

With this code, nonliving monsters will erroneously identify a wand of drain life when before they didn't.

[Nick]

Before the restructure, the "obviousness when seen" line was the first line processed for each type. Therefore these two bits of code should be reversed.

With this code, nonliving monsters will erroneously identify a wand of drain life when before they didn't.

Ah yes, thanks - I thought there was still something not quite right there.

[PowerWyrm]

In project_m(), GF_ flags with no associated handlers are not processed anymore. Before the restructure, there was a big switch with a default section at the end:

switch (typ)
{
    case GF_MISSILE (and all obvious effects): ...
    case GF_FIRE (and all handled effects): ...
    default: context.skipped = TRUE; context.dam = 0;
}

This default section is now missing. To fix that, maybe add something like:

if (!gf_force_obvious(typ) && (monster_handler == NULL))
{
    context.skipped = TRUE;
    context.dam = 0;
}

after calling monster_handler().

[Nick]

This default section is now missing. To fix that, maybe add something like:

if (!gf_force_obvious(typ) && (monster_handler == NULL))
{
    context.skipped = TRUE;
    context.dam = 0;
}

after calling monster_handler().

Actually there are no null monster handlers now, just some empty ones, so I guess the thing is just to set

    context.skipped = TRUE;

in all those.

[PowerWyrm]

In dice_parse_state_transition(), there's no way to add a "bonus" part when the spell has a random component. For example, PWMAngband has an elemental bolt spell which fires four bolts, one of each element. I've tried "dice:$Dd8+m4" to be able to display "dam 5d8x4", but the state_table doesn't allow that (only "base" + "bonus" is allowed, like for Meteor Swarm).

Here's an updated, more complete state_table:

static unsigned char state_table[DICE_STATE_MAX][DICE_INPUT_MAX] =
    {
        /* Input:                                -+dm$DU0 */
        /*[DICE_STATE_START] = */       /* A */ "B.EHKB..",
        /*[DICE_STATE_BASE_DIGIT] = */  /* B */ ".CE..B.C",
        /*[DICE_STATE_FLUSH_BASE] = */  /* C */ "..EHKD..",
        /*[DICE_STATE_DICE_DIGIT] = */  /* D */ "..E..D..",
        /*[DICE_STATE_FLUSH_DICE] = */  /* E */ "....KF..",
        /*[DICE_STATE_SIDE_DIGIT] = */  /* F */ ".G.H.F.G",
        /*[DICE_STATE_FLUSH_SIDE] = */  /* G */ "...H....",
        /*[DICE_STATE_BONUS] = */       /* H */ "....KI..",
        /*[DICE_STATE_BONUS_DIGIT] = */ /* I */ ".....I.J",
        /*[DICE_STATE_FLUSH_BONUS] = */ /* J */ "........",
        /*[DICE_STATE_VAR] = */         /* K */ "......L.",
        /*[DICE_STATE_VAR_CHAR] = */    /* L */ ".CEH..LM",
        /*[DICE_STATE_FLUSH_ALL] = */   /* M */ "........"
    };

[PowerWyrm]

Bah, there's also a Thunderstorm spell that uses the format "dice:$B+$Dd$S+m8" to display something like "dam 100+5d6x8" and that still doesn't work because the dice parser doesn't expect a "+" after anything else than the "base" value. For PWMAngband, I've simple hacked the parser to be able to use "-" as separator between dice and bonus so that the "dice" component is properly flushed.

The state table becomes:

static unsigned char state_table[DICE_STATE_MAX][DICE_INPUT_MAX] =
    {
        /* Input:                                -+dm$DU0 */
        /*[DICE_STATE_START] = */       /* A */ "B.EHKB..",
        /*[DICE_STATE_BASE_DIGIT] = */  /* B */ ".CE..B.C",
        /*[DICE_STATE_FLUSH_BASE] = */  /* C */ "..EHKD..",
        /*[DICE_STATE_DICE_DIGIT] = */  /* D */ "..E..D..",
        /*[DICE_STATE_FLUSH_DICE] = */  /* E */ "....KF..",
        /*[DICE_STATE_SIDE_DIGIT] = */  /* F */ "G..H.F.G",
        /*[DICE_STATE_FLUSH_SIDE] = */  /* G */ "...H....",
        /*[DICE_STATE_BONUS] = */       /* H */ "....KI..",
        /*[DICE_STATE_BONUS_DIGIT] = */ /* I */ ".....I.J",
        /*[DICE_STATE_FLUSH_BONUS] = */ /* J */ "........",
        /*[DICE_STATE_VAR] = */         /* K */ "......L.",
        /*[DICE_STATE_VAR_CHAR] = */    /* L */ "GCEH..LM",
        /*[DICE_STATE_FLUSH_ALL] = */   /* M */ "........"
    };

And I can use "dice:$Dd8-m4" for Elemental Bolt and "dice:$B+$Dd$S-m8" for Thunderstorm.

[PowerWyrm]

And it doesn't work...

I also need to declare the expression as "S-" instead of "S" because the parser considers the minus sign as a digit and not an operator.

[Nick]

And it doesn't work...

Once you have it working, I'm happy to just slot it right in - part of the point of all this as the z-* files at least should not need to be changed for variants.