Re: CF: Re: apply() cleanup

[Mark Wedel <mwedel@scruznet.com>]

Jan Echternach wrote:

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

 Your right.  But it wouldn't be hard to change the free_object function to
clear the count value to something..

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

 Yeah - that is one of those things that is slowly getting cleaned up.

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

 But how are you going to track the owned objects?  I guess you could add a
field in the objects to point back to the owners, but I really don't want
crossfire to have to start implementing its own garbage collection method. 
Think that if one function someplace forgets to clear a referance - slowly
things fill up with objects.

 More importantly, I don't think it really works good in the crossfire model as
a whole.  Functions should not be creating referances to objects and hope to get
those back - its not like crossfire is designed in any way for a known set of
events - you could have any number of players and that function could be called
again with any number of different calling parameters.  In theory, almost no
functions should be using static variables, and those that do should only really
be doing so because it is to track if they have been called before or it needs
to return a complex object/string that we know is short lived.  If crossfire
ever got threaded, even that last part would have to change.

 But the issue that I didn't see an answer to is what problem is trying to be
fixed here.  Needing to know if an object is destroyed in apply should only
happen in limited cases, and in those, they should be able to tell the calling
function (via return value) that the object is gone.  I can't think of too many
places where you have functions stacked many levels deep, all of them passing an
object and wanting to manipulate the object after it has been applied.


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

 In fact, no change may be needed to the function - it may just be a matter of
assigning the player objects a very high visibility.

 Probably in 0.96, I will implement a stacking intead of visiblity - stacking
having more meaning as it would have defined and meaning values of below floor,
floor level, on the floor, and high up objects.

> 
> Should I remove the code completely?

 I think the money_changer can go.  I am not sure on the grave - something may
or may not become of that.  It could probably go, and if someone wants to
develope it furhter later on, they can pull that out of old versions.


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

 If a player or other object has gotten moved, we should probably recall
check_walk_on - in fact, insert_ob_in_map should do that for us, so that the
original check walk on (which caused an exit to get applied, and then an object
moved, which then caused check_walk_on again), could just return at that point.

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

 Yeah - but there are general problems with many areas in that they can cause a
nest of calls to be made.  I certainly think that you are correct in that apply
should inform check_walk_on the status, and check_walk_on should abort if it
deems it necessary.
-
[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]