[Bf-python] RE: CVS commit: blender/source/blender/python/api2_2x Object.c blender/source/blender/python/api2_2x/doc Object.py

[Stephen Swaney]

As mentioned on bf-committers, I have some problems with the
following commit:

=============================================================

campbellbarton (Campbell Barton) 2005/12/30 12:54:53 CET

  Modified files:
    blender/source/blender/python/api2_2x Object.c 
    blender/source/blender/python/api2_2x/doc Object.py 
  
  Log:
  Added the function Duplicate to the object module.
  Object.Duplicate(linked=1)
  
  Basicly a wrapper for adduplicate();
  
  There was previously no easy way to copy objects in python. even the ways 
  that 
  do exist dont take modifiers, particles etc into account.
  Uses the current selection.. epydocs included also.

=============================================================================

A couple are minor issues with the code, the others have to
do with the design and implementation.  Hopefully, this will not sound too harsh!


First the code.  Comments are delimited with # as in python.

/*****************************************************************************/	
/* Function:                      M_Object_Duplicate                          */	
/* Python equivalent:     Blender.Object.Duplicate                            */	
/*****************************************************************************/	
 	 static PyObject *M_Object_Duplicate( PyObject * self, PyObject * args )	
 	 {	

 	         int *linked=1;	
# linked is declared as a pointer to int, but is used simply as a flag.
# you *can* do this, but it is quite misleading and violates the principle
# that code should be 'obvious'.

 	 	
 	         if( !PyArg_ParseTuple( args, "|i", &linked ) )	
 	                 return EXPP_ReturnPyObjError( PyExc_TypeError,	
 	                                 "2 ints expected as arguments" );	
# Error msg is wrong. only 1 int is expected!

 	 	
 	         if (linked) {	
 	                 G.qual |= LR_ALTKEY;  /* patch to make sure we get a linked dupli */	
 	                 adduplicate(1);	
 	                 G.qual &= ~LR_ALTKEY;	
# this business of messing with global key flags is setting off alarm bells.
# I know it is done other places in blender code, but it makes me nervous doing
#  it in bpy since we sit alongside blender.  At the very least, we should
#  save the original state of G.qual and restore it when we are done.

 	         } else {	
 	                 adduplicate(1);	
 	         }	
 	         Py_RETURN_NONE;	
 	 }	
#### end of code #### 	 

Now for the more serious part: the design.

The big problem here is the results of the method depend on
the user preference settings.  In general, global variables
are Considered Harmful. (yes, I know they have their place.
This is not one of them!)  What makes it particularly bad in
this case is there is no nice way to set the preferences.

Also, from looking at the accompanying doc, it looks like
this method changes the set of selected objects.  I am not
sure about this part.  If so, it is not good changing the
environment under the user.

The first phase of the BPy project was providing access to
the Blender database for imports and exports.  To do that we
had to make some changes to both blender's code and design to
factor out code we needed access to and to expose what were
strictly internal functions and data.

The second phase will be access to Blender's interactive
functions.  Once again, to do this, we will need to make
modifications to Blender code to expose the things we need.

I am all in favor of convenience functions for complex tasks
like linking an Object and its ObData into a Scene or for 
duplicating objects.  I would like to see some thought given
to a how we go about it.

The right way to do this is to factor out the code and data
we need access to, not to blindly hook into Blender's
existing function calls.  I know this is more work, but it
will have payoffs in the long run as Blender and Python become
more integrated.

As I said, this will sound harsh, but I believe these
changes should be backed out until they can be integrated
properly.  

If someone needs this functionality as-is in their on
version, we can create a patch and add it to the
tracker. (Long Live Open Source!).

As always, discussion is welcome.