Tracking Pies for Code Review
Details
Diff Detail
Event Timeline
first pass review
| tracking_pies.py | ||
|---|---|---|
| 25 | This is bad practice and not allowed in our code, as for why - see: | |
| 31 | These kinds of ad-hoc logging functions are useful for development, but for now better not add into releases. (python has logging module, currently we dont make use of it though, operators can report warnings, otherwise some print can be OK as long as not flooding the output, in that case better comment them and uncomment for development) | |
| 35 | If this is called a lot, probably better to get the size once and pass it in. (caller has size anyway) | |
| 45 | no need for bpy.context (atm its basically same, but happens to be passed in as an arg) | |
| 63 | if speed is a concern, you could cache this info, your getting it twice every time. once to check they exist and again later. def track_markers(track):
marker_curr = track.markers.find_frame(frame)
if marker_curr:
marker_prev = track.markers.find_frame(frame - 1)
if marker_prev:
return track, marker_curr, marker_prev
return None
relevant_tracks = filter(None, (track_markers(track) for track in clip.tracking.tracks))... you could convert to a list too if needed list(filter(...)) | |
| 116 | Better use getattr/setattr here, very similar example: | |
| 154 | Style - convention here is: frame_start = IntProperty(
name="Start Frame",
description="Start frame for baking",
min=0, max=300000,
default=1,
) | |
| 156 | also pep8, so spaces around keyword = | |
| 217 | for enums convention here is to use single quotes. Convention is followed only in some places. | |
| 265 | This variable isn't used below (referencing clip.tracking.tracks.active still) Also, could be a bit more verbose track_active, for eg. since blender has lots of different active data. | |
| 270 | op name is misleading, This is in fact operator properties, in Blender's code we use the name props | |
| 280 | We dont have strict conventions here, but would be inclined not to have both text & checkbox... eg: pie.prop( active, "use_normalization", text="Normalization", icon='CHECKBOX_HLT' if active.use_normalization else 'CHECKBOX_DEHLT', ) | |
| 283 | missing ) :) | |
| 458 | in this case prefer to have a class list. | |
Implemented most of the requested changes.
I couldn't get the suggested change for line 62 (about speed improvement) to work, so I just kept it as is. I don't think speed would be a big issue here.
I also didn't change the suggested change for line 115 (setattr), because in official clip.py the function CLIP_default_settings_from_track looks pretty much the same, so it is a bit consistent now at least.
| tracking_pies.py | ||
|---|---|---|
| 52 | Blenders start/end frames are inclusive. rendering frames (1-10) will render frame 1 and frame 10. Python's range doesn't include the last value, so you should probably do frame_end + 1 to include the last frame. | |
| 63 | Just Vector((0.0, 0.0)) - no need to convert 3d -> 2d | |
| 68 | no need to convert to float (thats py2), also, should have spaces around operators. | |
| 463 | Spaces should have been kept around = here, these aren't keyword args. | |
I still think this should be changed CLIP_default_settings_from_track isn't doing a direct copy, whereas this code is.
(Hopefully) fixed all requested changes.
Also check for active track before drawing pie props for it (line 273), it produced errors in the console.
I have split up the diff into ui_pie_menus_official.py and clip.py.
Here's the diffs for them