[Bf-committers] Modifier modularity/[plugability] - writefile
Campbell Barton
ideasman42 at gmail.com
Sat Mar 2 07:44:10 CET 2013
Hi Chad, just a note that these kinds of proposals for design changes
could be better collected on a wiki, where they can be updated/edited,
Then discussed here when we intend to implement them, Most of the
proposals we did for 2.5x were written up on wiki pages.
Eg:
http://wiki.blender.org/index.php/Dev:2.5/Source/Architecture/RNA
http://wiki.blender.org/index.php/Dev:2.5/Source/Python/API/Py3.1_Migration
more here...
http://wiki.blender.org/index.php/Dev:2.5/Source
On Thu, Feb 28, 2013 at 12:09 PM, Chad Fraleigh <chadf at triularity.org> wrote:
> 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).
A while back I made sure saving files on a disk with not enough room
gave propper error messages to the user,
While the code could probably do more fine grained error checks, are
there some steps you found that cause a bug?
(if so tracker report would be good)
> 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.
One concern with going ahead with this is that when a modifier isnt
supported its data is lost - this is what happens already in blender
if you load a new file in an old blender version.
But just doesn't happen much because modifiers aren't modular and
normally people just use the latest blender for a single project.
Are you sure your proposal can handle version patching (do_versions in
readfile.c), Endian switching too.
Its difficult because to give feedback on this because I don't want to
encourage someone to spend time on a patch that may not be accepted,
my personal advice is to try work on improving blenders modifiers
first - bugfixes, features which all devs can accept as improvements,
and keep this kind of project on the back-burner (Other devs feel free
to disagree with me on this! :) ).
> 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);
> }
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
--
- Campbell
More information about the Bf-committers
mailing list