this is all fixed and ready for inclusion.
all pep8 fixes are done.
T43509 review
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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 ↗ | (On Diff #3782) | better not import * - see http://stackoverflow.com/questions/2386714/why-is-import-bad |
| 68 ↗ | (On Diff #3782) | Suggest: WDGT_layers = [None] * 32 WDGT_layers[9] = True ... for others too. less verbose & reads better. |
| 74 ↗ | (On Diff #3782) | 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 ↗ | (On Diff #3782) | This will never be true (comparing one Object with the other objects name (str)) |
| 149–150 ↗ | (On Diff #3782) | 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 ↗ | (On Diff #3782) | better off to get the object here to avoid lookups below. |
| 184 ↗ | (On Diff #3782) | 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 ↗ | (On Diff #3782) | suggest assigning vars to bpy.context.object, this is a bit too verbose. |
| 729 ↗ | (On Diff #3782) | 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 |
| object_camera_rig_advanced/__init__.py | ||
|---|---|---|
| 68 ↗ | (On Diff #3782) | Why use None here? Wouldn't WDGT_layers = [False] * 32 be better? |
Comment Actions
@Sybren A. Stüvel (sybren), [None] * 32 - was a typo, noted in main body already but there is no ability to edit inline comments.
Comment Actions
Somehow this diff is malformed. almost all code is removed, not sure whats going on here but the patch seems to be broken/incorrect.
Comment Actions
The diff is only showing newlines changes for layer assignment, this seems like something is not working making the diff.
Comment Actions
The update to this patch makes almost no changes. only line length. Please address all the points you're able to.