Re: CF: Re: apply() cleanup

[Mark Wedel <>]

Jan Echternach wrote:

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

 I didn't really look over the code, as there was lots of changes, but instead
decided to look over your message to get a higher level idea of what is going
on.

 I guess my main question here is:  What are you trying to fix/improve that
needs such functionality?  Granted, it is good to know when the object you are
dealing with has been destroyed, but there are many ways to check that right now
- see my notes below.

> 
> 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 */

 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.

 I don't see much use for the make_referance - using referance counts is useful
for things like shared strings where if the referance count goes to zero we can
free the string.  But in the case described above, and other cases, that object
is getting freed regardless of whether there are referances to it or not, so
tracking them doesn't do much good.


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

 Makes sense.

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


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


> CLOCK:
>   bugfix: Monsters must not apply a clock, they would crash the server.

 and of course, no reason for them to apply it anyways, as they don't care what
time it is.

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


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


> 
> insert_ob_in_map() should be insert_ob_in_map_simple() sometimes?

 Yes - this was originally done for the polymorph code which would traverse a
pile of objects while tranforming them.  It was needed because the object it
transformed something to might be the same as something on that space, and the
insert would then result in a rearrangement of the stacking of that object (as
it may merge with something else).  insert_ob_in_map_simple basically inserts it
into the map but doesn't do any merging of the objects or special stacking, so
you can be sure the stack you are traversing is not changing.

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

 Don't know.

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

 Yes - I noticed this last night when a character applied a holy symbol and he
already had a skill - a pretty nonsensical message was displayed.

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

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

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

> 
> 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.
-
[you can put yourself on the announcement list only or unsubscribe altogether
by sending an email stating your wishes to ]