bTrace.py review

[TrumanBlending, 2012-01-04 03:14:59]

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py
File btrace/bTrace.py (right):

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode42
btrace/bTrace.py:42: from bpy.props import *
Should explicitly import each property type you need rather than using *

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode46
btrace/bTrace.py:46: p = bpy.props
If each property type is imported you can use them directly rather than storing
bpy.props in p

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode537
btrace/bTrace.py:537: bl_description = "Connect selected objects with a curve
and add hooks to each node."
Warning about ending with "."

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode594
btrace/bTrace.py:594: for a in list : a.select = True
Preferably don't use single line for loops

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode626
btrace/bTrace.py:626: bl_description = "Creates a curve from each particle of a
system. Keeping particle amount under 250 will make this run faster."
Warning about ending description with "."

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode824
btrace/bTrace.py:824: v.co[u] += TRobjectnoise*(random.random()*2-1)
Could use random.uniform() instead

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode839
btrace/bTrace.py:839: for v in verts: li.append(v.index)
Preferably don't use single line for loop

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode843
btrace/bTrace.py:843: for px in li:
This appears to be used to find a minimum, but builtin function min() could be
used instead. Faster and cleaner.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode1038
btrace/bTrace.py:1038: print('Its curvalicious')
Odd print(), but not a big problem

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode1052
btrace/bTrace.py:1052: def select():
Indentation issues here

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode1058
btrace/bTrace.py:1058: bpy.selection=[]
Not sure about adding this to bpy, seems polluting.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode1065
btrace/bTrace.py:1065: if (sobj in bpy.selection)==False:
Should use: "if a not in selection" instead

[ideasman42, 2012-01-04 03:47:56]

agree with all trumanblending@gmail.com's comments, added some more.

Main error is with bpy.selection, others are more suggestions for better
practice.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py
File btrace/bTrace.py (right):

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode45
btrace/bTrace.py:45: class TracerProperties(bpy.types.PropertyGroup):
TR prefix of all attributes seems redundant, since the PropertyGroup is already
a collection of of related attrbutes.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode730
btrace/bTrace.py:730: spline.bezier_points[bFrames].co =
ps.particles[e].location
better to get the point otherwise access each time becomes slow eg:

bp = spline.bezier_points[bFrames]
pt = ps.particles[e]
bp.co = pt.location
...

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode758
btrace/bTrace.py:758: return (context.object and context.object.type in
['MESH','FONT','CURVE'])
Python3 optimizes for frozen set checking, eg:

context.object.type in {'MESH','FONT','CURVE'}

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode814
btrace/bTrace.py:814: bpy.ops.object.mode_set()
would be better to give an argument to make it explicit what mode is set.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode824
btrace/bTrace.py:824: v.co[u] += TRobjectnoise*(random.random()*2-1)
On 2012/01/04 03:14:59, TrumanBlending wrote:
> Could use random.uniform() instead

yep, v.co += Vector(random.uniform()) would be better.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode1004
btrace/bTrace.py:1004: bpy.context.scene.frame_set(actual)
possible inefficiency - not essential but worth looking into.

changing the frame can be a slow operation depending on the scene, It looks like
this might not even be needed since keyframe_insert takes a frame argument.

http://codereview.appspot.com/5502053/diff/1/btrace/bTrace.py#newcode1058
btrace/bTrace.py:1058: bpy.selection=[]
On 2012/01/04 03:14:59, TrumanBlending wrote:
> Not sure about adding this to bpy, seems polluting.

this should be avoided, blender just doesnt store selection order - you can
however use the active object with the selected objects - where you can think of
the active object as 'Last Selected'

Other notes -
- if you define a list at the top of the script it will be persistent just as
bpy.selected is, so no need to add attributes to bpy.

- storing the objects themselves will crash on undo since the pointers are
re-allocated. This should store their names instead if it is to be used at all.

- notice bpy.context.selected_objects is being accessed a lot, this should be
stored once before the loop starts since it has to make a new list every time,
notice you have 'sel' defined before, why not re-use?.