Re: CF: Re: apply() cleanup

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

On Wed, May 10, 2000 at 09:57:14PM -0700, Mark Wedel wrote:
> Jan Echternach wrote:
> > 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. 

get_owner() and set_owner() just need to maintain a second reference
count for owned objects only.

>  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

Functions already have implicit references by the C language pointers
to objects.  What they need is a way to know if these references are
broken.  One solution is a tag value, the other is explicit
references.

> 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

I didn't mean to imply that some variables must be static.  The
variables that store the tag value while do_something() is called can
have automatic storage duration.

>  But the issue that I didn't see an answer to is what problem is trying to be
> fixed here.

The same problem that is adressed by op->count.  I'm still thinking
that references are a slightly better solution.

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

For example, it happens everywhere insert_ob_in_map() is called and the
object is used after that call.


BTW, I've already started rewriting common/object.c to better see how
reference counts would work in practice.  get_object() and copy_object()
would return an object that already has a reference count of 1.
copy_object() takes a single parameter and internally calls get_object()
for the new object.  Code like polymorph_living() can use a seperate
copy_object_contents() function.  A new put_ob_in_map() function combines
insert_ob_in_map() and break_obref().  Only some code would have to
make direct calls to make_obref() or break_obref().  For instance, most
code in spell_effect.c would look like

  op = get_object();   /* or copy_object() or clone_arch() or whatever */
  ...  /* customize object, e.g. set op->x and op->y */
  put_object_in_map(op);

I like this code because it has a more restrictive structure which
makes future design changes easier.


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

One place is enough to need an alternative way to find out if the
object was destroyed.  I have already given an example:  How would you
implement (with return values only) the case where a monster steps on a
fire bullet and check_fired_arch() is called from apply()?

I have already given other reasons why a return value is often the
worst solution or no solution at all:  Multiple "active" objects at
different function call levels (e.g. player summons monster, monster is
inserted into map and steps on a fire bullet, fireball kills both
player and monster; both the code moving the monster and the code
handling the player may need to know if the monster or player was
killed).  And return values are not very robust.  If a function is
changed such that it can destroy objects, and a return value is not
added to that function (and evaluation of the return value to all
callers), you've got a bug that can severly damage the data structures
of crossfire, e.g.  update_ob_speed() called after that function could
result in a free object on active list.

[insert_ob_in_map() + remove_ob() to keep player on top]
>  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.

Just marking the places where a player is kept on top with /* FIXME */
would be enough for now?

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

Then I will remove all of that code.  It has some bugs and would need
to be maintained without possibility of testing it if it should be a
good base for further development.  Rewriting it will be easier.

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

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

How would you handle closed trapdoors, open trapdoors with blocked
destination and open trapdoors with free destination consistently?
I've stopped at this question because it didn't seem worth changing
current behaviour.  All types of exits are placed by the map desinger
on the map and should be immediately above the floor or even below the
floor.

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

apply() already had a return value of 2 as an indication that the
object was destroyed, and check_walk_on() checked for this return
value.  But it was not always returned when necessary.  Different
meaning of return values needed by check_walk_on() and
player_apply_below() was the main reason why I splitted apply() into
manual_apply() and move_apply().

check_walk_on() needs a sure way to find out it the object was
destroyed.  I think move_apply() shouldn't return anything and just let
check_walk_on() look at the object's tag or use reference counts and
look at the FLAG_FREED.  This would be more robust than the current
solution.

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