CF: Re: apply() cleanup

[Jan Echternach <echter@informatik.uni-rostock.de>]

Hi,

Once the design changes where implemented, rewriting apply() took much
less time than I expected.  You will find some brief comments on the
changes at the end of this message.  The patch compiles, but is
completely untested.  Please take a look at the QUESTIONS section
below.

The patch is available at

  http://www.informatik.uni-rostock.de/~echter/cf/patch13b.gz

because it seems to be too large for the mailing list.

There is one major design change I want to implement before I would
consider the patch ready for the CVS tree.  I added return values
indicating whether an object was destroyed to many functions.  But this
approach is not powerful enough to handle cases like some object
stepping on a fire bullet which calls check_fired_arch() which calls
explode_object() which calls hit_map() which can destroy many objects,
including the object that stepped on the fire bullet.  There is no easy
way for hit_map() to know if it destroyed the object we're interested
in.  Furthermore, higher levels of the code probably deal with more than
one object at the same time.  A simple return value can't handle that.
Finally, this approach isn't very robust because if one function is
modified such that it can destroy objects all callers have to be
changed, which includes changing the interface of all functions between
the modified function and the initial caller.

A different solution for handling destroyed objects is using a
reference count scheme.  This can also handle issues like op->owner.
Some simple code would look like

  make_reference (op);
  do_something (op);   /* can be very complex code here */
  if (break_reference (op))
    return;   /* op destroyed, abort current function */

or

  make_reference (op);
  do_something (op);   /* can be very complex code here */
  if (is_destroyed (op))
    return;   /* op destroyed, abort current function */
  do_something_else (op);   /* more arbitrarily complex code */
  break_reference (op);

make_reference() increases the reference count.  free_object() and
break_reference() but the object on the list of free objects only if
both the reference count is 0 and the object has FLAG_FREED set.
Complex code like check_fired_arch() and multiple references by
different code dealing with the same object would not be a problem.


CHANGES

* player specific code moved from apply() to player_apply() and
player_apply_below().

* apply(): null pointer check removed

* split apply() -> move_apply() / manual_apply()
 
* move_apply(), check_walk_on(), insert_ob_in_map() and other functions:
New parameter 'originator' that is the object that caused another object
to be inserted into the map.  This removes the need to look through all
map objects above an altar to find the player who dropped the sacrifice.

* always check FLAG_UNPAID in manual_apply()

* don't check FLAG_UNPAID in apply_special(), new function
monster_apply_special() which checks FLAG_UNPAID and calls apply_special()

* many new functions like apply_altar() and apply_spellbook() to make
the switch statements in move_apply() and manual_apply() shorter

* changes to actual apply() code:

ALTAR:
  if the altar casted a spell, the message was sent to the (destroyed!)
    sacrifice, not the player
  spell casting altars should work now if not operated by a player
  common/button.c, operate_altar(): altar's message is sent to altar's
    map, not player's map
  bugfix: if sacrifice was accepted, apply() didn't tell the caller
    that it has been destroyed
ARROW, CONE, FBULLET, BULLET:
  now returns 'victim destroyed' if victim was killed by the attack
TRAPDOOR:
  play sound only if something fell through the trapdoor
  print trapdoor message _before_ falling into trapdoor
TRIGGER_ALTAR (in check_trigger()):
  don't check for sacrifices if called from animate_trigger()
  don't call check_trigger() from fix_auto_apply() anymore (this change
    breaks maps with altars that have their sacrifice already stored on
    them)
DEEP_SWAMP:
  deep_swamp() -> walk_on_deep_swamp() / move_deep_swamp()
  when deep_swamp() (now move_deep_swamp()) is called from
  process_object(), assume that state (swamp->stats.food) is already 1
EXIT:
  removed support for multisquare players
  don't print 'is closed' message when moving on an exit, only print this
    message if exit is applied manually
SIGN:
  only increment last_eat if it hadn't reached the limit yet
SCROLL:
  (bugfix?) print message if scroll is unusable because of invalid
    spell number
SPELLBOOK:
  removed partial support for applying by monsters
BOW, WAND, ROD, HORN:
  moved code to apply_special()
CLOCK:
  bugfix: Monsters must not apply a clock, they would crash the server.
POWER_CRYSTAL:
  Bugfix? Don't call esrv_update_item() if not applied by a player.
LIGHTER:
  Bugfix? Just return 0 if not applied by a player.

* changes to apply_special():

BOW:
  remove double check_skill_to_apply()
ROD, HORN:
  change message from "you apply ..." to "you ready ..."

BOW: FLAG_READY_BOW is now set by apply_special(), but that seems
redundant because monster_check_apply() also sets this flag.  Remove
setting of this flag from apply_special() because it could have a
more complex meaning in monster.c or remove setting of this flag from
monster_check_apply() because such stuff should be handled in apply.c?
FLAG_READY_BOW is also unset in apply_special() if a bow is unapplied.


TODO

insert_ob_in_map(), transfer_ob(), teleport(): check return values
everywhere

--> Lots of bugs here.  Much code can use free'd objects.

grep FLAG_FREED */*.c for even more bugs, because objects can be reused
before FLAG_FREED is tested.  (Even if they can't be reused right now,
such code is very fragile.)

socket/item.c (look_at): Set FLAG_NO_APPLY before remove/insert? Or
better yet, write a move_on_top() function?  There are other places with
similar code like make_gravestone() and apply(TREASURE).

random_maps/monster.c (insert_multisquare_ob_in_map): Head is inserted
first.  What happens if it is destroyed?

make_gravestone(): free_string(stone->name) immediately before using
stone->name?

Test map for the new 'originator' parameter used by apply_altar():
Altar of devourers, triggered by new code if face of death is cast on
a victim at the altar's location.


QUESTIONS

insert_ob_in_map() should be insert_ob_in_map_simple() sometimes?

move_apply(CONE): Why damage multiplied by 20?  I managed to do damage
780 with a firewall.

manual_apply(SKILL): Player applies a visible skill object, but
FLAG_READY_SKILL is cleared?  Looks like a bug to me.

GRAVE, MONEY_CHANGER: Unused?  I disabled the code with #if 0.

move_apply(): Split into move_on_apply() and move_off_apply()?  Most
code in move_apply() is specific to items that only have FLAG_FLY_ON
or FLAG_WALK_OFF set.  Only some code is used with FLAG_FLY_OFF or
FLAG_WALK_OFF.

EXIT and similar objects: Continue processing objects at old location even
if we entered the exit?  There could be some problems changing this.
For example, if a trapdoor is open but the destination is blocked,
continue processing objects below the trapdoor?  Continue processing
objects if trapdoor is closed?

deep_swamp(): Don't directly reduce hit points, but use hit_player() or
something like that?

-- 
Jan
-
[you can put yourself on the announcement list only or unsubscribe altogether
by sending an email stating your wishes to crossfire-request@ifi.uio.no]