Implementing the restructure changes

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

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

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.

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

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]

Piles of objects are now fully functionnal in PWMAngband... I think. Time to make a backup save of the source and start implementing the change...

[Nick]

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

The correct thing for V to do - and I am hoping to get it done before 4.1 - is to move all the known player stuff to cave_k and gear_k. This should make it easier for your variant, because then the client side should know exactly cave_k and gear_k (which contain everything about the player's knowledge of stuff), whereas the server knows cave and gear (completely, and there's no need to mess around with putting knowledge info on anything).

It's fairly amazing that you're managed to implement everything - and also very useful to me.

[PowerWyrm]

rd_objects_aux() uses rd_item directly. I guess it could use the rd_item_version parameter instead...

Anyway, 23/78 files ported

[PowerWyrm]

Looking at the latest code, it seems that held_m_idx (on objects) is declared (and set) but never used anywhere.

[PowerWyrm]

In obj_known_damage(), the "obj" variable inside a loop masks the "obj" parameter. This probably gives a compilation warning.

[PowerWyrm]

make_fake_artifact() creates a copy of an object, which may allocate memory for slays and brands. The object -- even if created as a local structure "object_type_body" -- should still be freed.

[PowerWyrm]

floor_carry() never deletes the "drop" object if it fails. This should really rarely happen (probably only if a character spends enough time on a level to fill every square with a pile of objects), but the code probably should handle the case.

[PowerWyrm]

floor_carry() never deletes the "drop" object if it fails. This should really rarely happen (probably only if a character spends enough time on a level to fill every square with a pile of objects), but the code probably should handle the case.

In fact this happens very often, because of drop_near(). Objects that break or that cannot be placed up to three spaces away on a pile (except artifacts) are not dropped on the ground either (as per floor_carry). In this case, the corresponding memory is not freed.

[PowerWyrm]

44/78 files ported. I hope I'm not ruining the whole code in the process...

[Nick]

Looking at the latest code, it seems that held_m_idx (on objects) is declared (and set) but never used anywhere.

That's amusing - looks like that was mainly used when iterating through the object list. No longer needed now there's no object list, but I probably won't remove it right away.