Implementing the restructure changes

[PowerWyrm]

Digger description leaks info ("it can clear rubble...") when the digger is not identified (or worn).

Fix: in obj_known_digging(), add a check vs object_this_mod_is_visible(OBJ_MOD_TUNNEL).

[PowerWyrm]

A copy/paste error in mod_slot_mult(): before the restructure, off-weapon multiplier for SHOTS was 4, it was changed to 3 in f49f44c.

[PowerWyrm]

Brands and slays are broken since 78cf8d.

When making an attack, the best brand/slay is given by calling improve_attack_modifier() on each equipment slot.

pointer best_slay;
for (slots) improve_attack_modifier(best_slay);
multiplier = best_slay->multiplier

Before the restructure, the function was doing:

improve_attack_modifier(pointer best_slay)
{
for (slays) {if (slay->multiplier > best_slay->multiplier) best_slay = slay;}
}

Now it's doing:

improve_attack_modifier(pointer best_slay)
{
int multiplier = 1;
for (slays) {if (slay->multiplier > multiplier) multiplier = slay->multiplier; best_slay = slay;}
}

Basically each call resets the best brand/slay. The result is that the last call wins (in this case the weapon) and off-weapon brands/slays are ignored.

Fix: in improve_attack_modifier(), line 481, the code should be

int best_mult = 1;

if (*brand_used) best_mult = brand_used->multiplier;
else if (*slay_used) best_mult = slay_used->multiplier;

[PowerWyrm]

Just looking at improve_attack_modifier(), it seems that only the multiplier is used, so using those fancy pointers seem useless and it would probably be better to pass and return the multiplier as a pointer:

		int j;
		const struct brand *b = NULL;
		const struct slay *s = NULL;

		my_strcpy(verb, "hit", sizeof(verb));

		/* Get the best attack from all slays or
		 * brands on all non-launcher equipment */
		for (j = 2; j < player->body.count; j++) {
			struct object *obj = slot_object(player, j);
			if (obj)
				improve_attack_modifier(obj, mon, &b, &s, verb, FALSE, TRUE,
										FALSE);
		}

		improve_attack_modifier(obj, mon, &b, &s, verb, FALSE, TRUE, FALSE);

		dmg = melee_damage(obj, b, s);

becomes

		int j, mult = 1;

		my_strcpy(verb, "hit", sizeof(verb));

		/* Get the best attack from all slays or
		 * brands on all non-launcher equipment */
		for (j = 2; j < player->body.count; j++) {
			struct object *obj = slot_object(player, j);
			if (obj)
				improve_attack_modifier(obj, mon, &mult, verb, FALSE, TRUE,
										FALSE);
		}

		improve_attack_modifier(obj, mon, &mult, verb, FALSE, TRUE, FALSE);

		dmg = melee_damage(obj, mult);

and modify melee_damage() accordingly.

[PowerWyrm]

About improve_attack_modifier(), the previous remark is even more true if RF_HURT_FIRE and RF_HURT_COLD are finally implemented as they should be (double damage in melee if using a BRAND_FIRE/BRAND_COLD weapon). Passing and returning the multiplier would allow using a multiplier of 4 for weak brands and 6 for normal brands instead of 2 and 3.

[Nick]

A copy/paste error in mod_slot_mult(): before the restructure, off-weapon multiplier for SHOTS was 4, it was changed to 3 in f49f44c.

Nice one.

For the brands and slays, the extra info is needed for power calculations, and I believe current code does calculate the modifier correctly. Implementing HURT_FIRE etc correctly sounds like a thing for 4.1

[Derakon]

Before the restructure, the function was doing:

improve_attack_modifier(pointer best_slay)
{
for (slays) {if (slay->multiplier > best_slay->multiplier) best_slay = slay;}
}

Now it's doing:

improve_attack_modifier(pointer best_slay)
{
int multiplier = 1;
for (slays) {if (slay->multiplier > multiplier) multiplier = slay->multiplier; best_slay = slay;}
}

And this kind of thing is why you always, always, always INCLUDE THE CURLY BRACES on your one-liner if/for/while blocks. Someone's going to come in later and want to add a line to the block, and they won't notice that there's no curly brace, so their line will always run, creating weird and often hard-to-find bugs. It's so common that I consider the lack of curly braces (in languages that use them, i.e. not Python) to be a bug itself and will fix them whenever I see them in code I'm working on.

[PowerWyrm]

Found two bugs while implementing the new slay_power() function:

1) Power doesn't take into account KILL brands because

if (num_slays + num_brands == 0) return p;

Clearly it should be

if ((num_brands + num_slays + num_kills) == 0) return p;

2) Before the restructure, an object having no slays/brands was adding dice power to the overall power (instead of the dice power multiplied by the average multiplier of all slays/brands vs monsters). After the restructure, nothing is added. So in fact, the previous line should be

if ((num_brands + num_slays + num_kills) == 0) return p + dice_pwr;

[the Invisible Stalker]

And this kind of thing is why you always, always, always INCLUDE THE CURLY BRACES on your one-liner if/for/while blocks. Someone's going to come in later and want to add a line to the block, and they won't notice that there's no curly brace, so their line will always run, creating weird and often hard-to-find bugs. It's so common that I consider the lack of curly braces (in languages that use them, i.e. not Python) to be a bug itself and will fix them whenever I see them in code I'm working on.

Leaving out the braces is fine if you don't break the line. I still usually put them in, because sooner or later someone usually wants to add statements, but the real problem here is that we humans tend to believe indentation is semantic even in languages where it isn't. If you don't break the line in the conditional then that mistake is much harder to make. Unless, of course, you are in the habit of writing multiple statements on a single line, in which case your code will unreadable to most humans, with or without braces.

[PowerWyrm]

Found another bug in the slay cache. Before the restructure, the cache was checked vs known slays and brands (of_inter between known flags and slay/brand flags). After the restructure, a plain comparison is done (brands_are_equal/slays_are_equal).

Fix: pass "known" parameter to check_slay_cache/brands_are_equal/slays_are_equal and compare like everywhere else vs "known || brand/slay->known".

[Derakon]

Leaving out the braces is fine if you don't break the line.

If you aren't breaking the line but can't spare two characters for the curly braces, then you have more general problems you need to deal with. Always use curlies.

[PowerWyrm]

Found another bug in the slay cache. Before the restructure, the cache was checked vs known slays and brands (of_inter between known flags and slay/brand flags). After the restructure, a plain comparison is done (brands_are_equal/slays_are_equal).

Fix: pass "known" parameter to check_slay_cache/brands_are_equal/slays_are_equal and compare like everywhere else vs "known || brand/slay->known".

And in fill_slay_cache() too because you need to add the known slay/brand combination to the cache.

[PowerWyrm]

Since 428140, weak brands cannot be added to randarts anymore. In append_random_brand(), multiplier is set to 3. To bring back old behavior, just allow multiplier of 2 (just like for slays where you can choose 3 for "slay" or 5 for "kill").

[PowerWyrm]

The element vs spell restructure reverted a bunch of fine code which grouped in a list-xxx.h file all side effects from elemental attacks. Now the side effects are again coded individually in player handlers. In the process, some durations were changed:
- poison: 10+1d(dam) -> 10 + rand0(dam)
- shards: 1d(dam) -> dam
- water: 1d40 -> rand0(40), which can be 0 by the way...
- ice: 1d15 -> rand0(15)
- force: 1d20 -> rand0(20)

[Nick]

The element vs spell restructure reverted a bunch of fine code which grouped in a list-xxx.h file all side effects from elemental attacks. Now the side effects are again coded individually in player handlers. In the process, some durations were changed

Excellent, thanks - I want any changes to this sort of thing to be conscious