io_mesh_xyz addon

[ideasman42, 2012-10-30 04:25:36]

Initial review of xyz file support by Clemens Barth.

functionality review here:
http://lists.blender.org/pipermail/bf-python/2012-October/005971.html

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py
File io_mesh_xyz/__init__.py (right):

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode64
io_mesh_xyz/__init__.py:64: ATOM_XYZ_ERROR = ""
This is used to show as a global setting that is used when calling:
bpy.ops.atom_xyz.error_dialog()

Im not sure why this is needed, in all the places its used, I think
self.report({'ERROR'}, "message") can replace it.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode98
io_mesh_xyz/__init__.py:98: bpy.context.scene.atom_xyz.add()
draw functions should not manipulate blend file data.

This should be done by operators called from the UI.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode278
io_mesh_xyz/__init__.py:278: class CLASS_atom_xyz_create_command(Operator):
Having this operator at all is odd, an operator to create a batch render script
is fine, but I dont think it belongs in a spesific import/export script.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode295
io_mesh_xyz/__init__.py:295: for element in import_xyz.STRUCTURE:
poll() functions are for quick checks and should avoid looping over data.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode308
io_mesh_xyz/__init__.py:308: scn = bpy.context.scene
The variable name 'scn' is used for both:

scn = bpy.context.scene
and...
scn = bpy.context.scene.atom_xyz[0]

This makes for confusion, each should have its own name eg.
scene = bpy.context.scene
atom_xyz_info = bpy.context.scene.atom_xyz[0]

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode314
io_mesh_xyz/__init__.py:314: if file_blend == "":
better use 'bpy.data.is_saved'

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode330
io_mesh_xyz/__init__.py:330: bpy.context.scene.camera = cameras[0]
don't think the exporter should be assigning cameras, just report and error if
no active camera is set and finish.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode345
io_mesh_xyz/__init__.py:345: bpy.ops.wm.save_mainfile()
Not sure why this is needed? - exporters should not have to save the blend file
they are exporting from.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode350
io_mesh_xyz/__init__.py:350: 
Writing a script and then executing it is not really a common thing to do, Id
prefer addons dont attempt stuff like this, however if this is done theres no
need to have it write a batch/shell file. pythons `subprocess` module can handle
it.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode352
io_mesh_xyz/__init__.py:352: execute = (blender_exe+" -b \'"+file_blend+"\' -x 1
-o //"+file_movie+
this string escaping can go wrong, better use: `subprocess.list2cmdline`

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode376
io_mesh_xyz/__init__.py:376: class CLASS_atom_xyz_render(Operator):
again, I think this is outside the scope of what an import/export script should
do... 

Most comments on 'CLASS_atom_xyz_create_command' apply here too, Ill skip
reviewing this class.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode464
io_mesh_xyz/__init__.py:464: class CLASS_atom_xyz_delete_keys(Operator):
Why is this operator needed? Couldn't the load operator below that loads
animation into shape keys just overwrite existing shape keys?

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode496
io_mesh_xyz/__init__.py:496: if element.name == '':
having objects named to an empty string is not allowed, if you try assign this
it will name the object 'Untitled'. What is the purpose?

Giving a name a special meaning is error prone too, you may have an object
already in another scene with this name.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode518
io_mesh_xyz/__init__.py:518: # If no object is in the scene, do nothing (return
False).
these poll functions are exact duplicates, could de-duplicate this.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode548
io_mesh_xyz/__init__.py:548: if element.name == '':
see above, empty names are not going to happen.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode586
io_mesh_xyz/__init__.py:586: if len(obj.children) != 0:
.children is a convenience accessor, that loops over _all_ items in
bpy.data.objects, if you need to use it multiple times, assign it to a variable.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode588
io_mesh_xyz/__init__.py:588: if child.type == "SURFACE" or child.type  ==
"MESH":
convention for blender is to write:
if child.type in {'SURFACE', 'MESH'}:

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode627
io_mesh_xyz/__init__.py:627: scale = obj.children[0].scale
again, multiple access to .children is slow, better assign a var.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode681
io_mesh_xyz/__init__.py:681: class CLASS_atom_xyz_distance_button(Operator):
another operator I think is outside the scope of an import/export script.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode770
io_mesh_xyz/__init__.py:770: class
CLASS_atom_xyz_error_dialog(bpy.types.Operator):
as stated above, this class should be removed and use operator reports instead.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/__init__.py#newcode889
io_mesh_xyz/__init__.py:889: import_xyz.ATOM_XYZ_FILEPATH =
bpy.path.abspath(self.filepath)
better pass this as an argument to import_xyz.DEF_atom_xyz_main

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/export_xyz.py
File io_mesh_xyz/export_xyz.py (right):

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/export_xyz.py#newcode31
io_mesh_xyz/export_xyz.py:31: ATOM_XYZ_XYZTEXT  = (  "This XYZ file has been
created with Blender "
the "+" here can be removed, python does multi-line strings like this.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/export_xyz.py#newcode49
io_mesh_xyz/export_xyz.py:49: if "Stick" in obj.name:
think it would be better to use some custom property on the object rather then
relying on object names.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/export_xyz.py#newcode52
io_mesh_xyz/export_xyz.py:52: if obj.type != "SURFACE" and obj.type != "MESH":
as before in {'MESH', 'SURFACE'} is a convention we use.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/export_xyz.py#newcode90
io_mesh_xyz/export_xyz.py:90: xyz_file_p.write(str(counter)+"\n")
str(counter)+"\n"

can be:

"%d\n" % counter

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py
File io_mesh_xyz/import_xyz.py (right):

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:180: class CLASS_atom_xyz_Elements(object):
prefixing class names with CLASS_ is a bit odd. Pep8 suggests camel case.

something like AtomXYZElement ?

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:209: def DEF_atom_xyz_distance():
as said before, think this should be removed.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:233: if bpy.context.scene.layers[i] == True:
better do this as...

layers = [True] * 20
bpy.context.scene.layers = layers

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:243: if len(obj.children) != 0:
again, multiple access to children is not optimal.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:306: 
many of these functions loop through objects the same way. this could be moved
into its own function that returns a list, or a generator.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:733: bpy.ops.object.camera_add(view_align=False,
enter_editmode=False,
its better to add a camera by bpy.data.cameras.new() then
bpy.data.objects.new(cam_data)

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:757:
bpy.ops.transform.rotate(value=(90.0*2*math.pi/360.0),
its better to set the camera objects transform matrix 'matrix_world' directly.

http://codereview.appspot.com/6815052/diff/1/io_mesh_xyz/import_xyz.py#newcod...
io_mesh_xyz/import_xyz.py:790: bpy.ops.object.lamp_add (type = 'POINT',
view_align=False,
again, better not use operators here.