[Bf-codereview] io_mesh_xyz addon (issue 6815052)

Tue Oct 30 05:25:36 CET 2012

Reviewers: blendphys_root-1.de, ideasman42, bf-codereview_blender.org,

Message:
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#newcode180
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#newcode209
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#newcode233
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#newcode243
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#newcode306
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#newcode733
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#newcode757
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#newcode790
io_mesh_xyz/import_xyz.py:790: bpy.ops.object.lamp_add (type = 'POINT',
view_align=False,
again, better not use operators here.



Please review this at http://codereview.appspot.com/6815052/

Affected files:
   M     io_mesh_xyz/__init__.py
   M     io_mesh_xyz/export_xyz.py
   M     io_mesh_xyz/import_xyz.py


Index: io_mesh_xyz/__init__.py
===================================================================

--- io_mesh_xyz/__init__.py	(revision 3901)
+++ io_mesh_xyz/__init__.py	(working copy)
@@ -981,3 +981,4 @@
  if __name__ == "__main__":

      register()
+
Index: io_mesh_xyz/export_xyz.py
===================================================================
--- io_mesh_xyz/export_xyz.py	(revision 3901)
+++ io_mesh_xyz/export_xyz.py	(working copy)
@@ -101,4 +101,3 @@
      xyz_file_p.close()

      return True
-
Index: io_mesh_xyz/import_xyz.py
===================================================================
--- io_mesh_xyz/import_xyz.py	(revision 3901)
+++ io_mesh_xyz/import_xyz.py	(working copy)
@@ -963,6 +963,3 @@
          element.data.shape_keys.key_blocks[number-1].value = 0.0
          element.data.shape_keys.key_blocks[number].keyframe_insert("value")
           
element.data.shape_keys.key_blocks[number-1].keyframe_insert("value")
-
-
-


