Re: CF: Re: apply() cleanup

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

On Tue, May 09, 2000 at 09:12:56PM -0700, Mark Wedel wrote:
>  That can easily be done by something like:
> 
> 	tag = op->count;
> 	do_something(op);
> 	if (tag != op->count) return; /* object changed/destroyed */
> 
>  A simpler check can be just a QUERY_FLAG(op, FLAG_FREED), but this does not
> always work if the item has been recycled - using the tag/count value would
> handle that case.

At least a function like

  int was_destroyed (object *op, uint32 old_tag)
  {
    return QUERY_FLAG (op, FLAG_FREED) || op->count != old_tag;
  }

is required.  Checking only the tag doesn't tell you if the object was
destroyed, only if it was reused.

(Note that type of tag must be uint32 - some code currently used int or long.)

I don't see how much simpler this solution would be.  You still have to
perform special actions before and after calling do_something().  A
minor disadvantage is that you have do declare a variable for the tag
somewhere.  An advantage over the reference solution would be that you
can't get incorrect reference counts if you forget to break a
reference.  But that could be checked automatically at the top level in
process_events() where the total reference count must be equal to the
number of reference by owned objects.  And the reference count solution
doesn't need to worry about tag wraparounds.

> > * apply(): null pointer check removed
> 
>  While not a big deal to remove that, I would note taht there are probably lots
> of checks in the code which could be argued as 'unnecessary' since that event
> should never happen.  But having checks to gracefully handle such situations is
> almost always preferable to a core dump.

Code that passes a NULL pointer to apply() probably crashes anyway.

> > SPELLBOOK:
> >   removed partial support for applying by monsters
> 
>  Not sure what this means.  I know that many monsters would learn the spells in
> spellbooks they have (although they did not get destroyed on use) - it seems to
> me that such behaviour is desired.

Some of the code did a

  if (op->type == PLAYER)

and some other code just assumed that it's a player and would crash
otherwise.  There was a

  if (op->type != PLAYER) return;

right at the beginning of the SPELLBOOK code.

> > 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.
> 
>  The above probably never happen in any case, as monsters will only apply stuff
> in which they have apply flags set for, and I don't think the above items
> currently fall into such categories.

can_apply & 32 means to apply everything except those object types
which have their own can_apply bits.

> > 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).
> 
>  In fact, I don't see a major reason why the player always needs to be on top. 
> But if the player should always be on top, it is easy enough to modify
> insert_ob_in_map to put any new items below the player (if we assume the player
> is always on top, it should only be one check to see if the top object is the
> player.  And if the object is being inserted is a player, same thing - just put
> that new object on the very top)

I agree.  Modifying insert_ob_in_map() is probably the best solution.

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

I will remove that and wait for players complaining that their spells
don't do any damage anymore.  ;)

> > GRAVE, MONEY_CHANGER: Unused?  I disabled the code with #if 0.
> 
>  I believe so.  Graves were going to be used so that players could bury other
> players and the like, but that never got finished up.  Money changers probably
> are not needed anymore after addition of converters and slaying of type money
> were added.

Should I remove the code completely?

> > 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.
> 
>  Looking at the archetypes, there is in fact no standard archetype that has
> fly_off set.  walk_off is used for things like buttons and inventory checkers so
> they go back to their original state.  However, it can (and is) added to other
> objects in some maps, so I don't think seperation is a good idea - if that is
> done, may as well hard code what items effectively have walk_off set.
> 
>  For example, I could see adding walk_off to pits in some map, on the idea that
> maybe you could get on top the unstable pit, but getting off is not a sure
> thing.

Setting FLAG_WALK_OFF on arbitrary objects breaks remove_ob().  The
patch already includes a log message if check_walk_on() in remove_ob()
destroyed the object.

> > 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?
> 
>  I'm not really sure what you mean here - what is doing the processing?  Are you
> talking about other objects being applied, or something else?

I mean that check_walk_on() continues processing objects that used to
be below the object, even if that object is now at a completely
different place.

> > deep_swamp(): Don't directly reduce hit points, but use hit_player() or
> > something like that?
> 
>  Problem is what attack type do you use?  If you use physical, the players get
> the benefit of armor, which probably isn't really appropriate if you are
> drowning in  swamp.  Actually, you could probably use AT_INTERNAL - that was
> added after the swamp code, and nothing affects damage taken by that method.

Fiddling with the player's hitpoints everywhere makes it impossible to
change handling of the player's death.  For example, the reference
count solution for detecting destroyed objects can detect dead players
without problems if the player object is copied to a new player object
that is put at the /city/city map and the old player object is freed.

BTW, check_walk_on() could try to kill a player multiple times because
apply() didn't tell check_walk_on() if the player was already killed.
Only the if (op->stats.hp < 0) check in hit_player() made this work
without bugs.  But that's still ugly design.

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