Re: CF: Re: apply() cleanup

[Mark Wedel <mwedel@scruznet.com>]

Jan Echternach wrote:

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

 get_owner and set_owner need extra logic because objects have a object pointer
to the owner.  So it needs to verify if what that points to is still the proper
object, or if the owner has been freed and it should thus free the referance to
it.

 In the case of apply, I don't think that really works - I am not clear what the
owner/parent object should be - it would more be like some function is the
owner, so that doesn't work as well.

 Note that checking a save op->count value against the current op->count is the
best check to see if the object has been freed (op->count is unique or each
object and will never be reused).  This does not work if op was merged with some
other object, but I can't think of any good way to handle that - refcounts don't
really help, because the parent functions are still looking at one or the other
objects memory, of which one doesn't exist in a meaningful state.


> 
> 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'm not really sure what that really buys over insert_ob_in_map - as I see it,
the only difference is map is taken implicity from op instead as a passed
parameter.

 I'm doubtful on the usefulness of referance counts.  it seems that the only
place where you can really use them is something is when the stack of functions
get pretty deep - using referance counts would let the object sit around until
that stack is reduced to a normal level.

 The problem I see with that is that after the object is effectively destroyed,
everyone that still may want to use it should basically only check to see if the
object is dead and do no more processing on it.  And using the op->count values
above works fine for that.

 I'm just a little worried that someone forgetting to put a break_obref will
cause a difficult to find memory leak down the road.

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

 your right - return values do not work.  checking against op->count should.


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

 Yah - I am not sure of any real bugs if the player is not on top.  In most
cases, it is nice to have that higher visibility.

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

 closed_trapdoors - might as well return immediately with no processing since it
isn't going to change instaneously.
 open trapdoors with blocked destination - This is difficult, as it probably
depends on the blackage.  For example, if it is just a player, a sword should be
able to fall on top of the player, however we can't have other players (or
monsters) falling on top of that player.  But if the block is something else
(like a stone block such that nothing can fall), should return.  But this case
is probably should just not bother procesing.
 open trapdoor with open destination - should probably process all objects - it
makes sense that everything with the player should fall through with them.


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

 And less errorprone - as you noted, in many cases code written may not return
the right codes.
-
[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]