[crossfire] Code restructuring

[Alex Schultz]

Mark Wedel wrote:
> 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.
>   
Well, indeed it is never really very simple, but I believe some of the
changes described below would make it simpler by a noticeable amount.
>   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.)
>   
And these issues are why I don't believe it should be strictly one file
per type, and that given files should register to multiple types if that
makes sense. (Offtopic though, it does seem to me that in the case of
armor, it really should just use one type, and then use subtypes,
instead of how it is currently)
Also, it might be good to have a types/common directory, for store
functions that are not part of the "core" but are used by multiple files
in types/*.c (each of which corresponding to one or more object types,
but usually as few as makes sense)
>> 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.
>   
Well, what I was proposing wasn't per-object, but was per-archetype, as
in, one wouldn't access ob->apply, one would access ob->arch->apply, so
the memory hit wouldn't be very bad. However, what you say does have a
point still.
>   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.
>   
Hmm, that is true, however if we don't use no-op defaults, then checking
to make sure it isn't null, should IMHO be put into a macro, to make
sure nobody forgets that check.

The way I see it, there are 3 ways to take it, per-object,
per-archetype, and per-type (static array). In any case. I think
per-type is best if we only want the callbacks defined by the core (or
by plugins if an interface for plugins is made), however if we want the
callbacks defined by the archetypes or maps that wouldn't work. I'm now
thinking perhaps per-type would be best, but I am unsure.

>   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.
>   
Yes, that certainly does need to be kept in mind, however could be easy
to deal with it in many cases where there is currently a unifies
function for things like applying, because in those cases we shouldn't
need anything passed other than what the current function passes.
>> 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).
>   
Assuming 2.0 will be release some time in the next 4 months or so, that
makes sense. I wouldn't want to start now in a branch though due to
effort of keeping in sync.
>   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).
I'm currently estimating the break-even point, in terms the overhead vs.
benefit of branches, would be somewhere around 3-6 people depending on
how actively they would use it, so that said, I'm currently thinking it
might not be work it to do in a branch, however that is a very rough
estimate and difficult to really gage.
>   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.
That would make sense, though it may be difficult for people working on
such
may find it difficult to know when they would have a chance to finish
updating objects. I'm thinking something that might

Alex Schultz