[Bf-codereview] Operator to setup scene for camera tracking (issue 5432048)

[ideasman42 at gmail.com]

suggested edits for patch.


http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py
File release/scripts/startup/bl_operators/clip.py (right):

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode30
release/scripts/startup/bl_operators/clip.py:30: def
_set_background(space_v3d, clip, user):
dont think _ prefix is useful here, funcs in funcs are always private.

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode353
release/scripts/startup/bl_operators/clip.py:353: sc =
context.space_data
(picky - but poll runs a lot) may as well add a the top and avoid
multiple getattrs

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode358
release/scripts/startup/bl_operators/clip.py:358: def _setupScene(self,
context):
utility funcs that dont use self can be @staticmethod's, same for other
util funcs which dont use self

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode403
release/scripts/startup/bl_operators/clip.py:403: # Remove all
constraints to be sure motion is fine
perhaps we should have an api function .clear(), py has this for dict's

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode417
release/scripts/startup/bl_operators/clip.py:417: def
_findRenderLayer(self, scene, name):
think this function isn't needed, or replae with 1 liner.

   return scene.render.layers.get(name)

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode470
release/scripts/startup/bl_operators/clip.py:470: if node.type in
['MOVIECLIP', 'MOVIEDISTORTION']:
(picky) use a set - {'MOVIECLIP', 'MOVIEDISTORTION'}

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode479
release/scripts/startup/bl_operators/clip.py:479: b.location[0] += 40
b.location += Vector((40.0, 20.0))

same for uses below.

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode497
release/scripts/startup/bl_operators/clip.py:497: while len(tree.links):
# use this, py api optimizes for truth test on a collection.
while tree.links:
...

Another case where .clear() would be nice.

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode673
release/scripts/startup/bl_operators/clip.py:673: if ob.type == 'MESH'
and ob.name == 'Ground':
Im not much a fan of doing name checks - perhaps chech mesh type and
that it has 4 verts and 1 face?

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode679
release/scripts/startup/bl_operators/clip.py:679: all_layers = []
(picky) prefer LC's here, 1 liner...

return [(layers_a[i] | layers_b[i]) for i in range(len(layers_a))]

http://codereview.appspot.com/5432048/diff/1/release/scripts/startup/bl_operators/clip.py#newcode701
release/scripts/startup/bl_operators/clip.py:701: faces = [[0, 1, 2, 3],
(picky), prefer tuples for immutable seq's (unless you intend to modify
later)

http://codereview.appspot.com/5432048/