Details

Reviewers

Julian Eisel (Severin)
Campbell Barton (campbellbarton)
Sybren A. Stüvel (sybren)

Maniphest Tasks

T43509: Advanced Camera Rigs Addon

Summary

this is all fixed and ready for inclusion.
all pep8 fixes are done.

T43509 review

Diff Detail

Campbell Barton (campbellbarton) retitled this revision from to Initial commit for T43509.

Had a quick test of the addon, initial notes.

One of the cameras is un-selectable by default, not sure this is really good since selecting cameras to make active is needed/useful.
Theres unnecessary lookups, getting an object's name, then accessing bpy.data.objects[some_name] isn't good practice (redundant).
It may be good to split out rig types from the operators - so only have one operator and one UI. Then use a rig "type", this is just a suggestion, more a long term thing, but I think its worth considering (rigify does this). Then you can have each rig-type as a submodule and people can more easily add their own.
I got an error adding a new rig while in pose-mode. Its easy to workaround but good to try account for this - probably the poll functions can just check for OBJECT mode.
This script assumes hard-coded names, again - more of a longer term change you could investigate, this makes the script more fragile. If for example - objects already exist with those names.

Comment on details inline.

Edit: WDGT_layers = [None] * 32 should be WDGT_layers = [False] * 32 (Phabricator does't support editing inline comments)

object_camera_rig_advanced/__init__.py
40–41	better not import `*` - see http://stackoverflow.com/questions/2386714/why-is-import-bad
68	Suggest: WDGT_layers = [None] * 32 WDGT_layers[9] = True ... for others too. less verbose & reads better.
74	Having `create_widget` return the object and `create_root_widget` not, is a bit confusing. If a function's purpose is to create some data, its good practice to return it. Same goes for `create_camera_widget` and other functions.
118	This will never be true (comparing one `Object` with the other objects name (`str`))
149–150	You don't need to use operatiors here. nicer not to infact. marker = context.scene.timeline_markers.new(name="cam_" + str(bpy.context.scene.frame_current)) marker.camera = active_cam # instead of bpy.ops.marker.camera_bind()
183	better off to get the object here to avoid lookups below.
184	`bpy.data.cameras[bpy.data.objects[active_cam].data.name]` can just be... `active_cam.data` if you do `active_cam = rig.children[0]` above.
285–292	suggest assigning vars to `bpy.context.object`, this is a bit too verbose.
729	prefer to have classes list, then register/unregister in loop, there are examples in our code already. classes = ( BuildDollyRig, BuildCraneRig, # ... etc ) for cls in classes: bpy.utils.register_class(cls) ... same of unregister