CF: Re: apply() cleanup

[Jan Echternach <>]

I have ported my applc() cleanup patch to the latest CVS version and
fixed some bugs.  After the trivial bugs had been fixed, I couldn't
find any more bugs in the new apply code, only a bunch of bugs in the
rest of crossfire.  I'm not sure if the patch is already stable enough
to put it in the CVS tree, because I haven't done any systematic
testing yet.

The patch itself is available as

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

(or patch13d.unified.gz if you prefer unified diff format).  Changes
since patch13b follow.  I'll also attach the changes in patch13b
relative to original crossfire-0.95.5.


QUESTIONS

Runes without level - bug or feature?

The vampire on /Lake_Country/snake_pit/Chaos_lair4 doesn't use the handle
unless the player is at most two map squares away (only possible in wiz mode).
I don't know if this bug was caused by the apply() changes or if it is
something unrelated.


CHANGES (since patch13b)

[items in brackets are bugfixes for bugs introduced with patch13b]

CONE:
  don't multiply damage by 20
SIGN:
  [wasn't handled in move_apply()]
  [stats.food != 0 check was accidently removed in patch13b]
  you can hear magic mouths even if you are blind
  however, now you can also read signs with FLAG_WALK_ON/FLAG_FLY_ON if you are
    blind
  (different object types or a new flag would be required)
RUNE:
  detonation moved from move_rune() to move_apply() -> runes will now always
    detonate if somebody steps on them
  allow runes with FLAG_FLY_ON
CONE:
  bugfix: don't attack with AT_COUNTERSPELL

completely removed GRAVE and MONEY_CHANGER

common/object.c: free_object(): set op->count to 0
common/object.c: remove_ob(): skip bottom half if map state is MAP_SAVING
common/object.c: was_destroyed(): new function
include/object.h: new typedef tag_t
move_apply() returns void, check_walk_on() uses was_destroyed()

[common/button.c: check_sacrifice(): used uninitialized 'op' variable]

common/main.c: process_events(): check if object was destroyed after call
to process_object()

lib/checkarch.pl:
  check for archetypes with walk_on, walk_off, fly_on or fly_off and without
    a type
  check for archetypes with type FIREWALL (type 62) and without a level

-- 
Jan
CHANGES

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

* apply(): null pointer check removed

* (bugfix?) print message if scroll is unusable because of invalid
spell number

* bugfix: if the altar casted a spell, the message was sent to the
(destroyed!) sacrifice, not the player

* 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:
  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
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 some hit function?