Version 4 (modified by magnate, 9 years ago) (diff)

Update with detailed explanation of changes.

Ok, here is a quick guide to the Easter 2011 changes to object flags.

It starts at [r0fa73a98], which closes #120. It made o_ptr->flags authoritative (I used "canonical" by mistake), meaning that object_flags() simply returns o_ptr->flags. Similarly, object_pval_flags() simply returns o_ptr->pval_flags[]. This allowed the removal of all the curse-flag hackery where o_ptr->flags was treated as authoritative for curse flags but not for anything else. It meant changes to ego_apply_magic() so that o_ptr->flags and pval_flags picked up the ego template flags at creation time, ditto for copy_artifact_data() and object_prep() for artifact flags and kind (& base) flags respectively. It also made wiz_create_artifact() and wiz_tweak_item() call these flag-setting functions (which were de-static'd). This commit neglected to do the same for make_fake_artifact(), so randarts were broken until this was done in [rd04b413f]. And copy_artifact_data() got a bug which was corrected in [rff6eb39].

Objects in older savefiles will not have authoritative flags, so in [r819bc78a] we add rd_item_3() and increment block versions for rd_inventory(), rd_stores() and rd_objects(). The duplicated code in this commit is later refactored, but the basic issue is that what was rd_item_2() is now rd_item_3(), and rd_item_2() now calls the flag-setting code to give the loaded objects authoritative flags. This is the big risk area for bugs with savefiles. In my staging savefile I found several objects in my inv wouldn't stack with new ones because they had no kind or base flags, but this should not have happened because of these two lines in rd_item_2() in this commit:

of_union(o_ptr->flags, o_ptr->kind->base->flags); of_union(o_ptr->flags, o_ptr->kind->flags);

This mystery remains unsolved and that's why I recommend more testing. If this happens to anyone else then there's almost certainly a bug I haven't found yet, but it could have been a fluke related to when I loaded that particular savefile. I cannot duplicate the problem with any other save.

The next major commit is [r7fb60c95] which created src/object/pval.[ch]. Existing functions object_pval_flags[_known](), object_this_pval_is_visible() and which_pval() moved into pval.c, and a load of .h files were tidied up. One new function was added, object_add_pval(), with two static subsidiaries, object_closest_pval() and object_dedup_pvals(). These are not used at this point so are explained later.

Then came [r6609d57e] which removed the level feeling stuff from copy_artifact_data() - this was much improved by noz in the next commit, after which came the merger of elly's anti-hack stuff and noz's use of it to sort out the Morgoth drop.

Back on object flags, [r2d00926f] finally closed #1404 by using object_add_pval() in ego_apply_magic(). The new function works like this:

  • check if flag is already on the object
    • if so, check if any other flags are associated with the same pval
      • if not, increment it safely and return, merging any duplicate pvals
  • if it's a new flag, or is on a shared pval, check MAX_PVALS to see if we can create a new one
    • if so, put flag on a new pval and increment it, and remove it from its previous pval
    • if not, work out what its new value should be and add/move it to the closest pval

So we can safely apply ego templates to base items without worrying about the order in which pvals are specified. E.g. creating an Elven Cloak of the Magi before this change results in two pvals, with OF_STEALTH on both; afterwards it has two or three pvals (depending on die rolls), with stealth on only one. In this same commit ego_apply_minima() is changed to use object_add_pval(), though this change is buggy and is later fixed.

Then [rd5b573f] fixes object_pval_flags_known() to cope with flags having been moved to a different pval. My tests of Elven Cloaks of Stealth all failed because of this bug, which erroneously showed the stealth flag on two pvals when it actually wasn't! There wasn't actually a bug in object_add_pval().

That's basically it, apart from [r21aad14b] which refactored ego_apply_minima() to allow access to ego_min_pvals(), so that can be called from rd_item_3() without inadvertently restoring plusses to disenchanted egos. (It's so hard to get credit for fixing bugs before they happen!)

From here, object_add_pval() will also be called from randart.c once that's rewritten. It isn't necessary for standarts, because artifact pval details are copied wholesale, as artifact definitions are assumed to overwrite anything on the base item.

For the record, there are two other possibilities for handling multiple pvals, which we have not used:

  1. Eddie suggested that pvals should always be +1, +2, +4 and flags assigned to them to create any value from +1 to +7. With a fourth pval this would cater for up to +15. This would make the add_pval functionality simpler and get rid of which_pval(), and gets us 15 possible pvals for the space of four, but lots of object display code would need rewriting, and lots of other stuff would need refactoring.
  1. FA does away with pval_flags altogether and uses an array for all granular flags. Going down this route means either making o_ptr->flags an array of bytes, or making a total separation between pval flags and binary flags. I would definitely favour the first, if only I understood the ramifications for all the bitflag manipulations (of which there are hundreds).