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

[g.ulairi at gmail.com]

Fixed issues pointed by Brecht in editors area. New patchset is
preparing...


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
Indeed. active is now a property of tracking.tracks. Fixed.

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):
Added 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")
I'm not sure about this. That two keyframes are properties for Solve
Camera operator, not to both of them. Moved this buttons just to see if
it's still clear now.

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")
The same thing as above.

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")
Personally, i dislike way in which slider is getting joined to button
when using such kind of things. Moved button above the property and
added separator above the button to make it clear property belongs to
only Set Scale operator.

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")
It's really nice. Didn't know about toggle=True, thx :)
Not sure if it's nicer to place them into a simple row or to row with
Align=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)
Always typing this work incorrectly =\

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)
Yes for not deselecting when clicking just outside. It's just a bit
difficult to say what means "outside". If you think it should be
deselected, can think about way of doing it without confusing behavior
of overlapped markers.

Not sure what do you mean nothing happens for already selected marker.
If extend flag is set, the you'll be deselecting parts of marker
(pattern/search/anchor). Otherwise all areas of markers becomes selected
(you can click on partially selected marker).

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 can be a bit annoying to have such kind of things. Nobody complained
about missing feedback for such cases. I don't have strong opinion here,
if you think it's really needed -- easy to add.

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);
Well, yes.

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";
Done

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");
Tried to explain it, but it's behavior is based on feedback from several
motion-tracking artists who said it's obvious how such kind of things
should work.

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;
Neither me nor gcc know about &&=. Replaced with ok=(...) && (...)
expression.

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