[Bf-committers] Modifier modularity/[plugability] - writefile

[Chad Fraleigh]

With the goal of blender [eventually/one day] having dynamically
loadable modifiers, there would be a long road of steps needed to get
there. Some of them, like making the modifier code base more modular,
appears to be relatively easy (moving the same code [for the most
part] around). Even without the loadable modifiers goal, doing this
seems worthy (less monolithic do-everythings-implementation code,
better cohesion). So with this sub-goal in mind, I would like to start
with relocating modifier-specific portions of  the writefile.c code to
the module files themselves. Other cohesion areas would follow later.

The initial pass would involve mostly just moving the code from the
monolithic writefile.c to each module's .c file and have a call to the
modular function be left in it's place. A later phase would be to add
a 'writeData' hook to ModifierTypeInfo, and then it would be the same
as the other polymorphic modifier hooks. However in the first part the
new function wrappers are written in a generic/common way so switching
to ModifierTypeInfo hook form doesn't require any changes except maybe
making the function static (and even that is only because it no longer
has to be global).

In the example (see below), C++ style comments ('//') are used as
example notations and not literal "would be" comments in the code.

Another issue also dealt with (since "fixing" it lines up well with
the changes anyway).. Currently little (if any) of the writefile code
does/handles errors. So if there is an I/O error mid-way, or invalid
data structure detected while writing (e.g. a NULL field that should
_never_ be NULL), it just ignores the error and keeps on writing more
elements. This could easily lead to corrupted save files with no
feedback to the user that there was a problem. So in the new functions
a status code is always returned, which eventually the calling code
would check (and also return any errors upstream). In these examples I
use an 'int' status code (where 0 means error, and non-0 is success)
for a "success boolean" type value, but this could potentially use a
typedef-ed enum if more than just pass/fail is needed (but at the
price of a simple "if(!xxxxx(...))" calling expression).  Another fix
is using const for arguments that point to data that won't be altered
by the function (like [blo_]write_struct() in the example).

There are some local write_XXXXX() functions used in the existing
code. To minimize the risk of hypothetical side breakage I add a
"public" wrapper using the blo_ prefix (and including a status code
placeholder) for those that would need to be called from relocated
"modifier" code. Eventually once nothing references the legacy
function(s) directly, they would be merged with/changed to be the
public version (and ideally return a real status code, if available).

I also wasn't sure which case convention to use for function prefixes
(i.e. mod_ vs MOD_ or blo_ vs BLO_) as existing code doesn't always
use one form consistently.

I would be willing to do most of the conversion leg-work.. Move/update
the code from writefile to the appropriate modules, add the
[available] error checking, etc.. and submit the patch files. Then
someone with commit would simply have to look it over and commit it,
or reject it because I missed something in that conversion, or reject
it if the patch didn't apply cleanly (i.e. the svn code changed in
between). In the later case I would just create and submit a new patch
from the updated code (as not to waste the commiter's time with
[messy?] merge work). For each modifier conversion I would first send
out a message to bf-committers a day ahead with the planned files to
change, in case anyone has significant commits pending in those areas
(which another large change might interfere with), so they can veto
it. In situations like that I would just skip/delay that modifier and
go to another. After submitting the patch to the tracker, I would post
the reference to bf-committers for a commiter to [hopefully] handle
[more] quickly (and reduce the chance of a patch conflict). The other
option, if some commiter was willing, I could just email the patches
directly to that person and skip the tracker and bf-committers
notification parts (the bf-committers heads up would still go out).
Also, presumably these efforts/changes would be done in the period
starting when the codebase is unfrozen after a release, which I assume
the the best time for more "radical" (but not really that radical)
changes.

The example conversion [fragments] (below) is of the "hook" and
"cloth" modifiers (written back in December [wow, has it really been
that long], so if any changes where made since, they will not be
reflected here).

-Chad


>> blender/source/blender/blenloader/BLO_writefile.h

// Forward declaration (private otherwise)

struct _WriteData;

// Add some "public" API declarations

typedef struct _WriteData WriteData;


int blo_write_data(WriteData *wd, int filecode, int len, const void *addr);

int blo_write_pointcaches(WriteData *wd, ListBase *ptcaches);

int blo_write_struct(WriteData *wd, int filecode, const char
*structname, int count, const void *addr);



----

>> blender/source/blender/blenloader/intern/writefile.c


// Replace typedef name with structure name

struct _WriteData {
	.
	.
	.
};


// Declare external modifier specific handlers (until done via ModifierTypeInfo)

int mod_cloth_write(ModifierData *md, WriteData *wd);
int mod_hook_write(ModifierData *md, WriteData *wd);


// Tie in the "public" API functions (eventually private ones just become these)

int
blo_write_data(WriteData *wd, int filecode, int len, const void *addr)
{
	writedata(wd, filecode, len, addr);
	return 1;
}


int
blo_write_pointcaches(WriteData *wd, ListBase *ptcaches)
{
	write_pointcaches(wd, ptcaches);
	return 1;
}


int
blo_write_struct(WriteData *wd, int filecode, const char *structname,
int count, const void *addr)
{
	writestruct(wd, filecode, structname, count, (void *) addr);
	return 1;
}


// Change to call "modifier" code instead

static void write_modifiers(WriteData *wd, ListBase *modbase)
{
        ModifierData *md;

        if (modbase == NULL) return;
        for (md=modbase->first; md; md= md->next) {
                ModifierTypeInfo *mti = modifierType_getInfo(md->type);
                if (mti == NULL) return;

                writestruct(wd, DATA, mti->structName, 1, md);

		// Eventually do:
		//   if(mti->writeData)
		//      mti->writeData(md, wd);
		//   else [[legacy 'else if(md->type == ...)' list here]]

                if (md->type==eModifierType_Hook) {
			// Changed:
			mod_hook_write(md, wd);
                }
                else if (md->type==eModifierType_Cloth) {
			// Changed:
			mod_cloth_write(md, wd);
                }
			.
			.
			.


----

>> blender/source/blender/modifiers/intern/MOD_cloth.c


int
mod_cloth_write(ModifierData *md, WriteData *wd)
{
	ClothModifierData *clmd = (ClothModifierData*) md;


	if(!blo_write_struct(wd, DATA, "ClothSimSettings", 1, clmd->sim_parms))
		return 0;

	if(!blo_write_struct(wd, DATA, "ClothCollSettings", 1, clmd->coll_parms))
		return 0;

	if(!blo_write_struct(wd, DATA, "EffectorWeights", 1,
clmd->sim_parms->effector_weights))
		return 0;

	return blo_write_pointcaches(wd, &clmd->ptcaches);
}


----

>> blender/source/blender/modifiers/intern/MOD_hook.c


int
mod_hook_write(ModifierData *md, WriteData *wd)
{
	HookModifierData *hmd = (HookModifierData*) md;


	return blo_write_data(wd, DATA, sizeof(int) * hmd->totindex, hmd->indexar);
}