Implementing the restructure changes

[PowerWyrm]

First thing I've done is move the chunk o_idx array inside the square structure, like it was done for monsters. By doing that, I've spotted a bigger problem that will probably prevent me from moving to linked lists: the array of marked objects for each player.

In Vanilla, the "marked" flag is on each object, and it's a simple byte. In PWMAngband, it is on each player, and it's an array of bytes indexed by object id. unfortunately, there's no way I can get around that easily...

[PowerWyrm]

And there I am... everything ported up to Nov 22, 2014. This means that next changeset is the "Replace level array for objects with grid-based linked lists" one...

At this point I don't see how I'm gonna move everything in my variant from index-based lists to object-based piles. Major problems include:
- piles not fully implemented in the old code: in my variant, the "floor-size" constant is set to 1 instead of 23 because the MAngband code never used piles of objects and I fear that in many places, the code still assumes that a floor tile only contains at most one object (using o_idx directly instead of looping on the next_o_idx field)
- client/server split: only the server part has access to every field of the object structure; the network code only transfers the pertinent fields (tval, sval and such, as well as the object descriptions as plain strings) from server to client and from client to server; objects on the client are referenced by their id (positive for gear items, negative for floor items) and every method uses that id to access the objects

This means I will have to:
- implement piles fully first
- port the server part from the changeset
- find a way to transfer and reference object ids
- port the client part from the changeset

[PowerWyrm]

While porting the new square system, I've found a potential endless loop in square_verify_trap() if multiple traps were implemented.

Usually, a loop like this is used:

while (trap) {
do something
trap = trap->next;
}

In square_verify_trap(), the "trap = trap->next" line is missing.

[Nick]

Now this is really puzzling me...

Change 1c405e6:
- the first parameter for the HASTE spell (monsters) is changed from MON_TMD_FAST to FAST
- effect_param() is modified to take into account monster timed effect names, but the function is called after taking into account player timed effect names, which also include "FAST" as a valid name
- the parser parses monster_spell.txt, examines the HASTE spell, calls effect_param() which should return TMD_FAST (0) and not MON_TMD_FAST (5)
- a monster casts HASTE, effect_do() should be called with 0 as p1 parameter instead of 5, which in return should call mon_inc_timed() with 0 as effect which is MON_TMD_SLEEP!
- the monster should be put to sleep

When I launch the latest 4.0 and summon a Berserker, he casts HASTE perfectly normally...

Brilliant, thank you for this analysis. The reason haste continues to work in 4.0 is the special casing of RSF_HASTE you mentioned earlier; I verified that removing this special case does indeed try to put the monster to sleep.

My solution in development is to make effect_param take the effect index as an argument, and only use the lookup function for the appropriate index - which is, as you have just demonstrated, much better coding practice.

[PowerWyrm]

Now this is really puzzling me...

Change 1c405e6:
- the first parameter for the HASTE spell (monsters) is changed from MON_TMD_FAST to FAST
- effect_param() is modified to take into account monster timed effect names, but the function is called after taking into account player timed effect names, which also include "FAST" as a valid name
- the parser parses monster_spell.txt, examines the HASTE spell, calls effect_param() which should return TMD_FAST (0) and not MON_TMD_FAST (5)
- a monster casts HASTE, effect_do() should be called with 0 as p1 parameter instead of 5, which in return should call mon_inc_timed() with 0 as effect which is MON_TMD_SLEEP!
- the monster should be put to sleep

When I launch the latest 4.0 and summon a Berserker, he casts HASTE perfectly normally...

After implementing the change in my variant, every monster that casts Haste is put to sleep as expected.

[PowerWyrm]

Now this is really puzzling me...

Change 1c405e6:
- the first parameter for the HASTE spell (monsters) is changed from MON_TMD_FAST to FAST
- effect_param() is modified to take into account monster timed effect names, but the function is called after taking into account player timed effect names, which also include "FAST" as a valid name
- the parser parses monster_spell.txt, examines the HASTE spell, calls effect_param() which should return TMD_FAST (0) and not MON_TMD_FAST (5)
- a monster casts HASTE, effect_do() should be called with 0 as p1 parameter instead of 5, which in return should call mon_inc_timed() with 0 as effect which is MON_TMD_SLEEP!
- the monster should be put to sleep

When I launch the latest 4.0 and summon a Berserker, he casts HASTE perfectly normally...

[PowerWyrm]

Change 3767674: in remove_bad_spells(), pflags are copied using of_copy() while it clearly should be pf_copy().

[PowerWyrm]

Change 4ef6687: in write_monster_entries(), file descriptor "old" is never closed.

... and power arrays are not freed if new_monster.txt file cannot be created.

[PowerWyrm]

Change 4ef6687: in write_monster_entries(), file descriptor "old" is never closed.

[PowerWyrm]

Looking at the new monster lore system:

- when parsing the monster file, first base flags are parsed and monster flags are set accordingly, then "on" flags are parsed and added, then "off" flags are parsed and removed from monster flags

- lore flags are saved directly from monster flags

- when parsing the lore file, lore flags are first all set in parse_lore_base(), then parsed and set again in parse_lore_flags(), then combined with base flags in finish_parse_lore()

I don't understand the last part... if flags are saved directly, they should be loaded directly too, so the only useful code here is the one in parse_lore_flags().

Nevermind... looking more closely, I found out that the "base" part in the lore file is only saved when the lore is full, so it's logical to set all the flags in parse_lore_base().

[PowerWyrm]

Looking at the new monster lore system:

- when parsing the monster file, first base flags are parsed and monster flags are set accordingly, then "on" flags are parsed and added, then "off" flags are parsed and removed from monster flags

- lore flags are saved directly from monster flags

- when parsing the lore file, lore flags are first all set in parse_lore_base(), then parsed and set again in parse_lore_flags(), then combined with base flags in finish_parse_lore()

I don't understand the last part... if flags are saved directly, they should be loaded directly too, so the only useful code here is the one in parse_lore_flags().

[Nick]

Changes 7a23301 + ee69d8c:

It seems that the old "visual" and "drawn" flags are not handled correctly in the new code. In display_bolt(), the "visual" flag has no effect at all, while in the old code, it was used to check for delay while moving along the projection path. In display_explosion(), the "drawn" flag works as intended, but in the old code, both "drawn" and "visual" were used for the delay.

I've filed this as a bug. I think I'm up to date with the rest, except for the RSF_HASTE silliness, which I'm leaving because the message issues involved are being dealt with.

[PowerWyrm]

The arg_graphics and arg_graphics_nice externs are defined twice in the code if you use the Windows port (ui-prefs.h + win-layout.c).

[PowerWyrm]

I'm currently looking at changes in the birth process and found out that get_stats() uses a hardcoded value of 18 to roll stats. This was correct before, but now that charisma is gone, this value should be adjusted. This probably has little impact in the function, but the following should be more correct (and clearer to understand):

	int dice[3 * STAT_MAX];

	/* Roll and verify some stats */
	while (TRUE) {
		/* Roll some dice */
		for (j = i = 0; i < 3 * STAT_MAX; i++) {
			/* Roll the dice */
			dice[i] = randint1(3 + i % 3);

			/* Collect the maximum */
			j += dice[i];
		}

		/* Verify totals */
		if ((j > 7 * STAT_MAX) && (j < 9 * STAT_MAX)) break;
	}

[PowerWyrm]

Changes 7a23301 + ee69d8c:

It seems that the old "visual" and "drawn" flags are not handled correctly in the new code. In display_bolt(), the "visual" flag has no effect at all, while in the old code, it was used to check for delay while moving along the projection path. In display_explosion(), the "drawn" flag works as intended, but in the old code, both "drawn" and "visual" were used for the delay.