Page MenuHome

Tracking Pies for the Movie Clip Editor
AbandonedPublic

Authored by Campbell Barton (campbellbarton) on Feb 6 2015, 10:25 AM.

Details

Summary

Tracking Pies for Code Review

Diff Detail

Event Timeline

Sebastian Koenig (sebastian_k) retitled this revision from to Tracking Pies for the Movie Clip Editor.
Sebastian Koenig (sebastian_k) updated this object.
Sebastian Koenig (sebastian_k) set the repository for this revision to rBAC Blender Add-ons Contrib.

New diff for tracking pies, forgot to remove property register.

first pass review

tracking_pies.py
25

This is bad practice and not allowed in our code, as for why - see:
http://stackoverflow.com/a/2386740/432509

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
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
Campbell Barton (campbellbarton) requested changes to this revision.Feb 6 2015, 11:10 AM
Campbell Barton (campbellbarton) edited edge metadata.
This revision now requires changes to proceed.Feb 6 2015, 11:10 AM
Sebastian Koenig (sebastian_k) edited edge metadata.

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.

Campbell Barton (campbellbarton) requested changes to this revision.Feb 9 2015, 3:35 PM
Campbell Barton (campbellbarton) edited edge metadata.
Campbell Barton (campbellbarton) added inline comments.
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.

This revision now requires changes to proceed.Feb 9 2015, 3:35 PM

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.

I still think this should be changed CLIP_default_settings_from_track isn't doing a direct copy, whereas this code is.

Sebastian Koenig (sebastian_k) edited edge metadata.

(Hopefully) fixed all requested changes.
Also check for active track before drawing pie props for it (line 273), it produced errors in the console.

Campbell Barton (campbellbarton) edited edge metadata.

Looks good, generally fine and think this is OK to be split up and moved into addons & clip.py.

Some minor comments.

tracking_pies.py
149

*super-picky* 8 spaces indent (extra 4, see other scripts).

326

space around =

354

space around = (theres more below too)

This revision is now accepted and ready to land.Feb 11 2015, 6:06 PM

I have split up the diff into ui_pie_menus_official.py and clip.py.
Here's the diffs for them


This revision now requires review to proceed.May 6 2015, 4:58 PM

This is in master now, closing.