Implementing the restructure changes

[Nick]

Clearly, this should be:

static void calc_hitpoints(struct player *p)
{
...
p->xxx
...
}

void update_stuff(struct player *p)
{
...
calc_hitpoints(p);
...
}

Clearly, and the same goes for a bunch of those player-calcs functions.

[PowerWyrm]

Before the "object flag apocalypse", ice effects were checking against cold immunity/resistance/vulnerability for damage. Now damage is handled by the corresponding element... and since there is no RES_ICE, you take full damage from ice attacks. This is probably not intended...

Side effects are handled corrrectly (inventory damage if no cold immunity, cuts if no rShards, stunning if no pStun).

Fix in adjust_dam():

	/* If an actual player exists, get their actual resist */
	if (p && p->race) {
                int res_type = GF_ICE? GF_COLD: type;
		
                resist = p->state.el_info[res_type].res_level;

		/* Notice element stuff */
		equip_notice_element(p, res_type);
	}

[PowerWyrm]

Another thing that is puzzling me is the new format of the xxx_stuff() methods.

Before:

void update_stuff(struct player *p)
{
...
calc_hitpoints();
...
}

Now:

void update_stuff(struct player_upkeep *upkeep)
{
...
calc_hitpoints();
...
}

However, if you look at the calc_hitpoints() method...

static void calc_hitpoints(void)
{
...
[B]player[/B]->xxx
...
}

So much for using a parameter in the calling method...

Clearly, this should be:

static void calc_hitpoints(struct player *p)
{
...
p->xxx
...
}

void update_stuff(struct player *p)
{
...
calc_hitpoints(p);
...
}

[PowerWyrm]

Did you notice

	dice_pwr = damage_dice_power(obj);
	p += dice_pwr;

earlier in object_power?

It actually seems possible to me that dice power is being incorrectly added twice for objects with slays and brands, but this code is quite dense and I need to read it more thoroughly.

Actually the old code was doing this:

1) Compute dice power (simply does power for xdy, independently of brands and slays)

2) Add power once for all objects (to take into account the dice)

3) Compute an averaged "slay" multiplier: 1 for objects with no slays/brands, something greater than 1 for objects with slays/brands

4) Add power again for all objects (multiplier * dice power)

So the dice power was indeed added twice. Was that a bug or not, no idea... But the current code (part 4) now adds 0 for items without brands and 1+x for items with brands. If you don't want to add the power twice for items with slays/brands, you need to substract 1 to the best multiplier when computing the averaged multiplier for slays.

[Nick]

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;

Did you notice

	dice_pwr = damage_dice_power(obj);
	p += dice_pwr;

earlier in object_power?

It actually seems possible to me that dice power is being incorrectly added twice for objects with slays and brands, but this code is quite dense and I need to read it more thoroughly.

[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

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

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

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.

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

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

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

[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