[crossfire] Code restructuring

[Mark Wedel]

Alex Schultz wrote:
> Currently in the crossfire code, how objects behave is intertwined all
> throughout the code, and this is in many ways a problem as it makes it
> difficult to find code related to a type of object. It's current state
> also makes it relatively difficult to add code for new object types.

   While the current organization isn't great, and should be redone, I'd 
disagree a little bit about the adding new objects - while hardly simple, I 
don't think any method will make it really simple - it can become simpler, but 
never really simple.

  The current code organization is based more on the function/operation of the 
object, and not the object itself (all the apply logic is together, all the time 
code is together).  Given there are a lot more object types than operations, it 
does make sense to organize by type (but even then, I think some grouping is 
necessary - I'm not sure I want 80 .c files, each covering a specific type of 
object, when some may just be a few lines of code.

  A real case of this is the general armor category - right now, there are 
boots, gloves, armor, shields, helmets, cloaks, etc, but all behave the same 
way, so it doesn't make sense to have different .c files for those - just a 
single armor.c file would be correct (but then in that case, we start looking at 
whether all those should be different types, or be a single type with subtypes, 
and even the subtypes may be overkill with the body code, since that was the 
main reason for all the different types.)

> 
> One possible solution to these issues, would be to move type-specific
> code, into a structure such as types/*.c, where files could be things
> such as types/teleporter.c or types/food.c. Also, some existing files
> such as swamp.c and disease.c also correspond well to this and would
> need little modification to be moved over.

  That works.

> Instead of making calls to things like apply_ob(), one would call things
> such as ob->arch->apply(), which would be function pointers stored in
> the archetypes, to functions in the aforementioned type-specific files.
> Archetypes would have such function pointers initialized to a "nop"
> function as a placeholder in order to avoid needing to check if the
> function pointer is null or not. After the archetype list is
> initialized, an init_types() function will run something like
> type_teleporter_init() from each of the type-specific files, which would
> register the type and adjust the function pointers of all archetypes of
> that type.

  Few quick thoughts:

  Unless we allow or plan for these callbacks to be modified on an object by 
object basis, I'm not sure if putting that callback in the object structure 
makes sense - it probably makes more sense to just have a static table, eg, 
apply_callbacks[].  The per type registration code could then update that. 
Doing that is certainly easier than going through all the archetypes updating 
the ones of the matching type (and if we're just doing that, what advantage do 
we get vs just using a table then?)

  This saves some amount of memory (pointer size * number of callbacks/object, 
eg, if we have 8 callbacks and running on a 64 bit systems, that is 64 
bytes/object).  That probably isn't a huge deal, but could add up.

  I think that may also be easier to code, or at least transition - in this 
case, simple 'if apply_callbacks[ob->type] apply->callbacks[ob->type]() else ..' 
if all that is needed.

  I do think it is neccessary not to set these to dummy functions - for 
performance, you don't want to be making function calls if it isn't do anything 
(no matter which implementation is chosen).  This maybe isn't a big deal for 
apply code, since that generally has to be triggered by some player, but there 
could certainly be time based callbacks that you don't want to be making if they 
are no-ops.

  Last thought - the data passed to these functions has to be well thought out, 
as all the callbacks of the same type will be getting the same data passed in 
(apply_armor will get the same parameters as apply_weapon as apply_handle, etc). 
  In some cases, these functions may not care what the extra data is, but there 
may be others that care what some field is.

  So this needs to be fairly well researched - you probably don't want to get 
halfway through doing the objects and get to one that needs some other parameter 
passed, and now have to go back through all the ones you did to update the 
calling structure.

> This design would also allow plugins to insert their own types, though
> the core of the crossfire should be statically linked and not use
> plugins for types normally.

  Adding such functionality would have to be carefully considered - specific 
notes/thoughts:
- If this is an object attribute, then a plugin could modify the callback 
structure for a specific object.  Problem is how to save/load that callback 
information.

- If this is a type attribute, some form of type reservation is needed.  If a 
plugin (script) uses type 999, and I don't know that and also use type 999 for a 
new object, things now break.  Or for that matter if two different script 
register different callbacks for the same type, problems occur.  The one plus 
side of using a table is it can be easier to detect these duplicate entries.

> 
> While such a change is technically manageable, it would need to be done
> bit by bit, and there may be issues with it breaking other code that
> people are working on, for this reason some people believe this
> restructuring should be done in a separate branch. Others are concerned
> about the extra effort of maintaining the branch and believe that it
> should not be done in a branch. There is also the possibility that the
> conversion may only be halfway done in a release, hence the code would
> be a little messy at such a release. Personally I am not sure which way
> is the right way to implement such a restructure in terms of those issues.

  I think such a restructure should wait until post 2.0 (I suppose work could 
start now in a branch).

  This endeavor may make sense to do in a branch, simply because multiple people 
could be working on it.  The issue always with branches is how many people will 
use it, if not many, the benefit of branches go down.

  I'd almost say it is better to do an approach piece by piece in the main 
branch, simply because such a major reorganization is such that merging isn't a 
simple issue.  Eg, I fix a bug in apply.c, but in the branch, it is 
types/weapon.c - the merge isn't going to catch it (it will catch that apply.c 
is different, but you'll have to manually move that change to types/weapon.c).

  However, if types/weapon.c is in the main branch, but there is still lots of 
stuff in server/apply.c, I'll fix whatever is in CVS.  Some of this may depend 
on how long it takes to make this transition.

  Other thought to make this work better is to send out some type of schedule, 
eg, I'm going to update these objects on this date, so either make your updates 
now, or you'll have to merge them after the commit.