[Bf-codereview] Camera tracking integration (issue 5285047)

[brechtvanlommel at gmail.com]

Reviewed editors module changes, this was the last module I didn't
review yet.


http://codereview.appspot.com/5285047/diff/18002/release/scripts/presets/tracking_track_color/default.py
File release/scripts/presets/tracking_track_color/default.py (right):

http://codereview.appspot.com/5285047/diff/18002/release/scripts/presets/tracking_track_color/default.py#newcode2
release/scripts/presets/tracking_track_color/default.py:2: track =
bpy.context.edit_movieclip.tracking.active_track
This property doesn't exist anymore, gives error when selecting preset.

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

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_operators/clip.py#newcode27
release/scripts/startup/bl_operators/clip.py:27: class
CLIP_OT_track_to_empty(Operator):
The operators in this file are all missing descriptions.

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_ui/space_clip.py
File release/scripts/startup/bl_ui/space_clip.py (right):

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_ui/space_clip.py#newcode162
release/scripts/startup/bl_ui/space_clip.py:162:
col.operator("clip.clear_solution")
For consistency, put the operator buttons at the top of the panel.

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_ui/space_clip.py#newcode186
release/scripts/startup/bl_ui/space_clip.py:186:
layout.operator("clip.clean_tracks")
For consistency, put the operator button at the top of the panel.

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_ui/space_clip.py#newcode235
release/scripts/startup/bl_ui/space_clip.py:235: col.prop(settings,
"distance")
I'd move this property below the operator and use col =
layout.column(align=True)

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_ui/space_clip.py#newcode306
release/scripts/startup/bl_ui/space_clip.py:306: row.prop(act_track,
"use_blue_channel", text="Blue")
These buttons don't fit well in the default region width. I would name
these "R", "G" and "B", and make them toggle buttons (toggle=True).

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c
File source/blender/blenkernel/intern/tracking.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode1187
source/blender/blenkernel/intern/tracking.c:1187: static int
retrive_libmv_reconstruct(MovieTracking *tracking, struct
libmv_Reconstruction *libmv_reconstruction)
retrive => retrieve

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode1568
source/blender/blenkernel/intern/tracking.c:1568: static void
retrive_libmv_features(MovieTracking *tracking, struct libmv_Features
*features,
retrive => retrieve

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c
File source/blender/editors/space_clip/tracking_ops.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode775
source/blender/editors/space_clip/tracking_ops.c:775: static int
mouse_select(bContext *C, float co[2], int extend)
When clicking on already selected marker, it seems nothing happens. The
marker does not become active and others are not deselected. When you
click right outside of it, it works?

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode1366
source/blender/editors/space_clip/tracking_ops.c:1366: while(framenr !=
efra) {
If I understand this right, if you start forwards tracking past the end
frame, or backwards tracking before the start frame, this will loop
forever.

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode1402
source/blender/editors/space_clip/tracking_ops.c:1402:
if(track_count_markers(sc, clip)==0)
It might be nice to show a little information report here.

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode1770
source/blender/editors/space_clip/tracking_ops.c:1770:
WM_event_add_notifier(C, NC_SPACE|ND_SPACE_VIEW3D, NULL);
An NC_OBJECT|ND_TRANSFORM notifier here seems more appropriate. Same for
the following operators.

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode1779
source/blender/editors/space_clip/tracking_ops.c:1779: ot->description=
"Set active marker as origin";
It might be good to mention in the description of this and the following
operators that this modifies the camera object transform.

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode1942
source/blender/editors/space_clip/tracking_ops.c:1942:
/********************** set origin operator *********************/
set origin => set axis

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode1954
source/blender/editors/space_clip/tracking_ops.c:1954:
BKE_report(op->reports, RPT_ERROR, "Track with bundle should be selected
to define X-axis");
This error message could more clearly indicate that one marker should be
selected. Maybe also the operator description could be clearer as well,
it's not really obvious how you from a single marker to an axis.

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode2868
source/blender/editors/space_clip/tracking_ops.c:2868: ok&= error == 0.f
|| (track->flag&TRACK_HAS_BUNDLE)==0  || track->error < error;
I'd replace &= by &&= on these two lines, for clarity.

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode2923
source/blender/editors/space_clip/tracking_ops.c:2923:
{TRACKING_CLEAN_DELETE_SEGMENT, "DELETE_SEGMENTS", 0, "Delete Segments",
"Delete un-clean segments of tracks"},
un-clean => unclean

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space_clip/tracking_ops.c#newcode2942
source/blender/editors/space_clip/tracking_ops.c:2942:
RNA_def_float(ot->srna, "error", 0.0f, 0.f, FLT_MAX, "Reprojection
Error", "Affect on tracks with have got larger reprojection error", 0.f,
100.0f);
Affect => Effect

http://codereview.appspot.com/5285047/diff/18002/source/blender/makesrna/intern/rna_tracking.c
File source/blender/makesrna/intern/rna_tracking.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/makesrna/intern/rna_tracking.c#newcode511
source/blender/makesrna/intern/rna_tracking.c:511:
RNA_def_property_ui_text(prop, "Bundle", "Position of bundle
reconstructed from this tarck");
tarck => track

http://codereview.appspot.com/5285047/