[crossfire] Code restructuring

[Yann Chachkoff]

Le mardi 18 juillet 2006 07:10, Mark Wedel a écrit :
> 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, currently, adding a new object type involves adding small bits of code 
everywhere. It seems much easier to me to regroup all the functionality 
specific to an object type in a central place, so adding a new object would 
basically be as simple as filling a unique source file template.

>   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.)
>
I agree with this - if several types behave the same, there is no need to 
duplicate code.

>   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[].  
>
You misread the proposal, I think. As I understood it, callbacks were to be 
bound with archetypes, not on a per-object basis. It sounds logical, as the 
archetype is supposed to be the "model" a given object is based on.

> 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?)
>
Updating the callbacks is something you'd do exceptionally - it should 
normally occur only when the archetypes are initially loaded; so that point 
really sounds like a non-issue.

>   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.
>
No, it would be 64bytes/archetype. 10,000 archetypes would thus give a little 
less than 640KB - I doubt such an amount can be considered as a problem, 
knowing the amount of memory most computers now have.

>   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 don't see how this is "easier" - both solutions look equally understandable 
to me.

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

>   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.
>
Indeed, it is a possible issue. My guess is that we shouldn't allow a plugin 
to change callbacks "on the fly", to keep things simple. 
My guess is that either an archetype mentions no plugin (and in that case, the 
default callbacks are bound to it), or it does (and in that case, the 
specified plugin is responsible for all callbacks of that archetype, 
eventually delegating some to the server default ones). Such a system means 
that you cannot manage an archetype by more than a single plugin - but I 
hardly see this as a big drawback.

> - 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.
>
I don't like the idea of using a numerical type identification - this is 
running the risk of type id clash.

>   I think such a restructure should wait until post 2.0 (I suppose work
> could start now in a branch).
>
Given that this is obviously an important change, I don't see why it should 
wait post-2.0. Quite the contrary, I think that an improved code structure is 
a primary goal that the next major CF version should take into account.

>   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).
>
The drawback being that this is a rather important change of the code; I think 
that doing it in the main branch is a bad idea, as it will probably make 
other changes harder to achieve. If this is going to be done, I'd suggest to 
make a "stable" branch, which can be maintained for the duration of the 
transition as a "bugfixes-only" code base (so basically, the main branch 
would be "Crossfire 2.0 code", while the "stable" one would be "1.9.x", and 
would get deprecated once the goals defined for "2.0" are met.

-- 
Yann Chachkoff
-----------------------
GPG Key 2006: http://keyserver.veridis.com:11371/export?id=-43113965597490782
Fingerprint : C224 F1F9 9025 4FC7 987D 05BB FF66 D413 A3B4 01A2
-----------------------
Reality is a nice place, but I wouldn't want to live there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mailman.metalforge.org/pipermail/crossfire/attachments/20060718/787af040/attachment.pgp