Camera tracking integration

Camera tracking integration patch applied on current trunk. Not compilable, but ok for discussion.

Additional project info is there: http://wiki.blender.org/index.php/User:Nazg-gul/GSoC-2011

[nazgul, 2011-10-15 18:37:16]

Corrected patch set

[brechtvl, 2011-10-19 14:38:07]

Just a few comments to get start, it's a big patch, will have to do this in a
few steps.

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/constraint.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/constraint.c:3938: /* ----------- Follow Track
------------- */
The names of these constraints don't indicate clearly to me that they are
related to the tracker. Especially Follow Track / Follow Path is a quite
similar. Just a suggestion, maybe they could be called Follow Reconstruction and
Camera Reconstruction?

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/constraint.c:4021: /* calculate lens and sensor
size depends on object type */
Can the code here be refactored such that it shares this with
object_camera_matrix()?

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/object.c:2196: void where_is_object_mat(Scene
*scene, Object *ob, float obmat[4][4])
This function gives the object matrix without constraints, is that intentional?

[nazgul, 2011-10-19 14:58:41]

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/constraint.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/constraint.c:3938: /* ----------- Follow Track
------------- */
"Reconstruction" is libmv-side term, for artists "Solve" or "Solution" is more
familiar. And "Follow Track" does nothing with solving/reconstruciton -- track
is a 2d moving point on the footage, it exists even if you don't want to solve
something. Probably it can be "Follow Motion Track" or something like this. Got
no idea how "Camera Solver" can be renamed in convenient for artists way.

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/constraint.c:4021: /* calculate lens and sensor
size depends on object type */
Yes, think this can be moved to function which will be shared with
object_camera_matrix.

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/object.c:2196: void where_is_object_mat(Scene
*scene, Object *ob, float obmat[4][4])
Yes, as comment for this function tells, it was intentional. This kind of stuff
is needed because motion-tracking stuff like bundles are calculated in camera
space. But in "original" camera space -- before applying camera solver
constraint, so when you move camera you see bundles following the camera, but
when start animation bundles stay "stable" and only camera moves. In fact, i
need function which will calculate camera matrix without camera solver
constraint. But can't see any real usecase when camera which is used for solving
has got other constraints, so imo it's almost fine.

[brechtvl, 2011-10-19 17:47:53]

Regarding the naming, it seems to me there are various terms used: Movie Clip,
Reconstruction, Track, Solver. It would be nice if it the naming made it clearer
that the scene panel, 3d view display options, constraints and movie clip editor
fit together. Maybe all should have Track/Tracker/Tracking in the name, I'm not
sure.

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

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_scene.py:223: bl_label = "Movie Clip"
I'd suggest to call this panel "Tracker". Also I'm not sure if the concept of an
active movie clip makes much sense outside of the tracker, perhaps it should
renamed to indicate it's only used for that?

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

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:42: sub.menu("CLIP_MT_select")
For consistency with other editors, the menus should be ordered view - select -
clip.

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:62: class CLIP_PT_tools(Panel):
I'd leave out this panel, this is already possible from the header.

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:380: class CLIP_PT_display(Panel):
There's not enough space here for two columns, in the default configuration
various words here are cut off, maybe split it up? Or make the region wider.

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:664: class CLIP_MT_clip(Menu):
The order in this menu should be reversed I think, usually open is at the bottom
and nested menus at the top.

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:799:
layout.operator("clip.select_all", text="Inverse").action = 'INVERT'
The order of this menu feels a bit strange compared to others. Not all select
menus are consistent, but perhaps you can make this one consistent with the 3d
view select menu?

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
File source/blender/nodes/composite/nodes/node_composite_transform.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
source/blender/nodes/composite/nodes/node_composite_transform.c:39: static
bNodeSocketTemplate cmp_node_stabilize2d_in[]= {
This variable should be renamed to cmp_node_transform_in, same for the output
sockets.

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
source/blender/nodes/composite/nodes/node_composite_transform.c:43:
{	SOCK_FLOAT,		1,	"Degr",			0.0f, 0.0f, 0.0f, 0.0f, -10000.0f, 10000.0f},
Usually such a property would be called "Angle", but it seems the Rotate node is
inconsistent at this as well. This socket should be set as PROP_ANGLE.

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
source/blender/nodes/composite/nodes/node_composite_transform.c:75:
mul_serie_m4(mat, lmat, smat, cmat, rmat, icmat, NULL, NULL, NULL);
In other places in Blender the order of transformations is
scale-rotate-translate, this is rotate-scale-translate. Any particular reason to
do it this way?

[brechtvl, 2011-10-19 17:55:02]

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

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_object_constraint.py:757:
layout.prop(con, "use_default_clip")
I think this property should be named "active clip" rather than "default clip"?

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/BKE...
File source/blender/blenkernel/BKE_movieclip.h (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_movieclip.h:54: void
BKE_movieclip_acquire_size(struct MovieClip *clip, struct MovieClipUser *user,
int *width, int *height);
These functions are called 'acquire', but i think they should just be 'get'.
Reason is that acquire/release naming is used for thread safe access with locks.

http://codereview.appspot.com/5285047/diff/3001/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_movieclip.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_movieclip.c:160: static void
rna_def_moviecliUuser(BlenderRNA *brna)
Small typo here.

[nazgul, 2011-10-19 21:31:08]

Corrected (i hope) all current issues. New patchset would be sent here soon.

The only thing i'm not sure about -- using names Track/Tracker/Tracking instead
of Movie Clip/
Reconstruction/Track/Solver. 

"Track" is a set of merkers which point the same feature on the image.
"Tracker" is a sub-program, which tracks marker across image sequence to make it
points the same feature.
"Tracking" is more like a process when you're making marker be followed feature.
"Solver" is a sub-program, which solves motion of camera using tracks user
created during "tracking".
"Reconstruction" is more like filling 3d scene with models of objects from real
world.

So it's just different things. I'm not sure about only one case -- using
reconstruction in 3d viewport display settings, it's not actually
reconstruction, more like a result of solving.. Not sure Solution would be nicer
here.

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

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_object_constraint.py:757:
layout.prop(con, "use_default_clip")
Yeah, you're right. Would correlate with scene settings better.

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

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_scene.py:223: bl_label = "Movie Clip"
This active clip is needed to define relation between movie clip used as main
motion-tracking source and current scene. You can have several clips solved, but
normally scene can deal only with one of tracked/solved clip. Other scenes can
use other clips. This clip is used as default "source" for camera solver and
follow track constraints. Normally you wouldn't want to change clip used by that
constraints to something else, only in case you've got witness cameras that
settings in costraints can make real sense..

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

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:62: class CLIP_PT_tools(Panel):
Yes, it's a rudiment which i thought i've dropped away. Dropped now for sure.
Also dropped away panel with Clip-related operators when there's no clip. Now
toolbox seems to be really abandoned when there's no clip opened..

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:380: class CLIP_PT_display(Panel):
Don't want this region be wider. Probably one column would be also easier to
find needed option. Most used options have got hotkey to be toggled, so
shouldn't be real issue with extra height of this panel.

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:664: class CLIP_MT_clip(Menu):
Indeed. Fixed in up-coming patchset.

http://codereview.appspot.com/5285047/diff/3001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_clip.py:799:
layout.operator("clip.select_all", text="Inverse").action = 'INVERT'
Ok, changed to match 3d view select menu.

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/constraint.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/constraint.c:4021: /* calculate lens and sensor
size depends on object type */
Done, would be uploaded in new patchset soon. Probably name for function isn't
ideal..

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
File source/blender/nodes/composite/nodes/node_composite_transform.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
source/blender/nodes/composite/nodes/node_composite_transform.c:39: static
bNodeSocketTemplate cmp_node_stabilize2d_in[]= {
Opps, better not only avoid using copy-pasteing, but also copying source files
:)

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
source/blender/nodes/composite/nodes/node_composite_transform.c:43:
{	SOCK_FLOAT,		1,	"Degr",			0.0f, 0.0f, 0.0f, 0.0f, -10000.0f, 10000.0f},
Ok, renamed and marked as PROP_ANGLE

http://codereview.appspot.com/5285047/diff/3001/source/blender/nodes/composit...
source/blender/nodes/composite/nodes/node_composite_transform.c:75:
mul_serie_m4(mat, lmat, smat, cmat, rmat, icmat, NULL, NULL, NULL);
Not really. It was just stabilization parameters were calculated. Probably
scaling around image center before rotation would be better (so scaled/rotated
image would stay on center of "canvas"). It's a bit late here, hope i didn't
flipped order in translate-rotate-scale :)

[nazgul, 2011-10-19 22:24:13]

Didn't notice last to notes, pardon. Easy to rename, but don't want to
re-generate patch again this night.

[nazgul, 2011-10-19 22:24:43]

Fixes most of pointed issues

[nazgul, 2011-10-20 09:52:50]

Updated to tomato rev41142

[brechtvl, 2011-10-21 13:53:39]

Reviewed RNA changes, mostly just naming/description comments.

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/object.c:2207: if(ob->partype & PARSLOW) {
Also this code should not be duplicated I think, better make this a common
function.

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_movieclip.c (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_movieclip.c:174: RNA_def_struct_ui_text(srna,
"Movie Clip User", "Parameters defining how an MovieClip datablock is used by
another datablock");
an MovieClip => a MovieClip

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_movieclip.c:180:
RNA_def_property_ui_text(prop, "Current Frame", "Get frame number user is points
to in clip");
Description is unclear

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_nodetree.c (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_nodetree.c:2451:
RNA_def_property_ui_text(prop, "Filter", "Method to use to filter rotation");
rotation => stabilization?

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_nodetree.c:2474:
RNA_def_property_ui_text(prop, "Distortion", "Distoriton to use to filter
image");
Distorition => Distortion

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_nodetree.c:2491:
RNA_def_property_ui_text(prop, "Filter", "Method to use to filter rotation");
rotation => transform

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_nodetree_types.h (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_nodetree_types.h:124: DefNode(
CompositorNode, CMP_NODE_STABILIZE2D,    def_cmp_stabilize2d,    "STABILIZE2D", 
  Stabilize,        "Stabelize 2D",      ""              )
Stabelize => Stabilize

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_scene.c (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_scene.c:3749: prop= RNA_def_property(srna,
"clip", PROP_POINTER, PROP_NONE);
Rename to active_clip perhaps?

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_space.c (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_space.c:856: static BGpic
*rna_BackgroundImage_add(View3D *v3d)
Please deduplicate this code with background_image_add, adding e.g.
ED_view3D_background_image_add()

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_space.c:1565: RNA_def_property_ui_text(prop,
"Show Camera Path", "Show reconstructed path of ameraip");
ameraip => camera

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

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:240: {TRACKING_CLEAN_SELECT,
"SELECT", 0, "Select", "Select un-clean tracks"},
un-clean => unclean

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:247: RNA_def_struct_ui_text(srna,
"Movie tracking settings", "Match-moving tracking settings");
Match-moving tracking settings => Match moving settings

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:253:
RNA_def_property_ui_text(prop, "tracker", "Tracking algorithm to use");
tracker => Tracker

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:259:
RNA_def_property_ui_text(prop, "Speed", "Speed to make tracking with");
This option is confusing to me, is it a quality setting? An option like
"Realtime" seems to imply it will track in realtime, but that would make
tracking give different quality results on different  runs/computers?

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:273:
RNA_def_property_ui_text(prop, "Adjust Frames", "Automatically re-adjust marker
position using position on each N frames. 0 means only keyframed position is
sued");
sued => used

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:315:
RNA_def_property_ui_text(prop, "Tracked Frames", "Affect on tracks which are
tracked less than specified amount of frames");
Affect => Effect, less than => less than the

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:322:
RNA_def_property_ui_text(prop, "Reprojection Error", "Affect on tracks with have
got larger reprojection error");
Affect => Effect, with have got => which have a

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:356:
RNA_def_property_ui_text(prop, "Focal Length", "Camera's focal length in
pixels");
Leave out "in pixels"

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:378:
RNA_def_property_ui_text(prop, "K1", "");
Add in description that this is a radial distortion parameter? Same for next two
properties.

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:411: prop= RNA_def_property(srna,
"pos", PROP_FLOAT, PROP_TRANSLATION);
These properties are typically called "co".

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:459:
RNA_def_property_ui_text(prop, "Pattern Min", "Left-bottom corner of pattern
area in unified coordinates relative to marker position");
unified => normalized?

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:530: prop= RNA_def_property(srna,
"hidden", PROP_BOOLEAN, PROP_NONE);
"hidden" => "hide" (convention)

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:536: prop= RNA_def_property(srna,
"locked", PROP_BOOLEAN, PROP_NONE);
"locked" => "lock" (convention)

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:681:
RNA_def_property_ui_text(prop, "Reconstructed", "Is tracking data contsains
valid reconstruction information");
contsains => contains

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_ui_api.c (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_ui_api.c:403:
RNA_def_function_ui_description(func, "Item(s). User interface for selecting
movie clips and their source paths.");
Descriptions should not end with "." for these functions

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_ui_api.c:413:
RNA_def_function_ui_description(func, "Item. A movie-marker widget to control
which depends on frame number.");
Don't understand this sentence

[ideasman42, 2011-10-22 13:50:26]

Just looked over python scripts and added some notes, will look over C source
next.

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

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_o...
release/scripts/startup/bl_operators/clip.py:49: if
bpy.data.objects[track.name].type == 'Empty':
This cant work - its called 'EMPTY'. Also, matching object/track names seems a
bit flaky,

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_o...
release/scripts/startup/bl_operators/clip.py:95: mesh.vertices.add(1)
Adding verts one by one like this realloc's the array every time, better to
build the list first and add all verts at once. Though in this case its probably
not a perfomance hit.

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

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/space_clip.py:70: layout.label(text="Average solve
error: %.4f" % \
You dont need '\' when surrounded by brackets in python. (other places use this
too)

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/space_clip.py:351: op =
row.operator("clip.camera_preset_add", text="", icon="ZOOMOUT")
normally this var is called 'props' since its operator properties. (other cases
like this in this file)

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/space_clip.py:651: ratios = [[1, 8], [1, 4], [1,
2], [1, 1], [2, 1], [4, 1], [8, 1]]
suggest to use tuples in cases when the values dont need to be modified later.

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/space_clip.py:803: class
CLIP_MT_select_grouped(Menu):
Not sure this menu is needed at all, there are UI functions to make a menu from
an operators enum.

[nazgul, 2011-10-22 15:21:29]

Updated to tomato rev41195

[nazgul, 2011-10-22 15:23:30]

Hopefully i didn't missed some comments and included all patches into new
patchset.

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/5285047/diff/3001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/object.c:2207: if(ob->partype & PARSLOW) {
Moved into function.

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

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_o...
release/scripts/startup/bl_operators/clip.py:49: if
bpy.data.objects[track.name].type == 'Empty':
Hrm, indeed. It'll be more clear to always create object. Removed this matching.

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

http://codereview.appspot.com/5285047/diff/26003/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/space_clip.py:351: op =
row.operator("clip.camera_preset_add", text="", icon="ZOOMOUT")
I can't tel it's really normally called props. Can see plenty of places where
it's called op. Even more, can see some files mixed using op and props =\
Changed to props in my file.

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_space.c (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_space.c:856: static BGpic
*rna_BackgroundImage_add(View3D *v3d)
Wasn't sure which file is correct to add this function. Placed it into
view3d_edit.c

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

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:259:
RNA_def_property_ui_text(prop, "Speed", "Speed to make tracking with");
It's nothing to do with quality, just for a visual feedback.

[ideasman42, 2011-10-24 14:53:37]

pedantic rna review

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_space.c (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_space.c:2823: 
prefer; show_marker_small

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

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:139:
if(camera->units==CAMERA_UNITS_MM) {
same comment as below.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:152:
if(camera->units==CAMERA_UNITS_MM) {
eeh, units should really be a display setting and not effect RNA.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:269: prop= RNA_def_property(srna,
"adjust_frames", PROP_INT, PROP_NONE);
frames_ could be prefix? - like frames_limit, for scene frames is a prefix for
eg.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:295: prop= RNA_def_property(srna,
"min_correlation", PROP_FLOAT, PROP_NONE);
min as suffix instead?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:346: prop= RNA_def_property(srna,
"sensor_width", PROP_FLOAT, PROP_NONE);
again, sensor_size ?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:426: prop= RNA_def_property(srna,
"enabled", PROP_BOOLEAN, PROP_NONE);
when updating api names we tried to avoid enabled/disabled named.

options...
- use_marker
- hide
- show_marker_frame

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:449:
RNA_def_property_string_funcs(prop, "rna_trackingTrack_name_get",
"rna_trackingTrack_name_length", "rna_trackingTrack_name_set");
think get & length funcs can be removed and only have set which works
differently to the default funcs.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:485: prop= RNA_def_property(srna,
"markers_count", PROP_INT, PROP_NONE);
This should not be needed, scripts can do len(markers), which can internally
access markersnr.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:562: func= RNA_def_function(srna,
"get_marker", "rna_trackingTrack_marker_get");
Im trying to avoid get/set in function names, would "marker_find" be ok?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:599: prop= RNA_def_property(srna,
"active_track_index", PROP_INT, PROP_NONE);
again, could this be made an attribute of 'tracks' ?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:611: prop= RNA_def_property(srna,
"max_scale", PROP_FLOAT, PROP_FACTOR);
pedantic, doesnt seem self consistant, 'scale_max' ?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:678: prop= RNA_def_property(srna,
"is_reconstructed", PROP_BOOLEAN, PROP_NONE);
could call it 'is_valid', which is used a bit elsewhere, not sure long name
helps here.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:706: func= RNA_def_function(srna,
"add", "rna_tracking_tracks_add");
with add functions I try not to add arguments which can be edited after, not is
this the case with frame numbers?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:708: RNA_def_int(func, "framenr",
1, MINFRAME, MAXFRAME, "Frame", "Frame number to add tracks on", MINFRAME,
MAXFRAME);
pedantic, call it `frame`

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:742: prop= RNA_def_property(srna,
"active_track", PROP_POINTER, PROP_NONE);
This should really be a property of the 'tracks' collection.
So `tracks.active` rather than `active_track`.

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
File source/blender/nodes/composite/nodes/node_composite_movieclip.c (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
source/blender/nodes/composite/nodes/node_composite_movieclip.c:68: /* now we
need a float buffer from the image with matching color management */
looks like its copied from node_composite_image.c, could be made a shared
funcion.

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
source/blender/nodes/composite/nodes/node_composite_movieclip.c:166: node->id=
G.main->movieclip.first;
Noticed this in a few places, while its not really bad it does seem a bit
arbitrary to set the movie clip only if there is one. I dont think this is done
anywhere else in blender.

[nazgul, 2011-10-24 16:36:08]

Fixed issues pointed by Campbell. Hope new patchset would be submitted soon.

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

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:152:
if(camera->units==CAMERA_UNITS_MM) {
Can display RNA setting be run-time so i can edit value in both of millimeters
and pixels? And is there such option already?
Also this can be done with two properties: focal_length_mm and focal_length_px..

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:269: prop= RNA_def_property(srna,
"adjust_frames", PROP_INT, PROP_NONE);
Ok

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:295: prop= RNA_def_property(srna,
"min_correlation", PROP_FLOAT, PROP_NONE);
Also ok

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:346: prop= RNA_def_property(srna,
"sensor_width", PROP_FLOAT, PROP_NONE);
Eek. Can we discuss this kind of question after i'll publish new sensor size
patch?
As was discussed in ML, both of sensor sizes are needed and it'll be just an
option to use automatic size (width or height depending on resolution) for FOV,
or use user-specified dimension to calculate FOV.
But this place isn't really related on this sensor patch. It's a real width of
rel sensor. This sensor also has got height which should be added here after
that new sensor patch is ready, so it'll be fully correlate real camera
intrinsics.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:426: prop= RNA_def_property(srna,
"enabled", PROP_BOOLEAN, PROP_NONE);
Renamed to "enable". But not sure about use_marker, hide and show_marker_frame.
Is it just samples or i've used bad naming somewhere else?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:449:
RNA_def_property_string_funcs(prop, "rna_trackingTrack_name_get",
"rna_trackingTrack_name_length", "rna_trackingTrack_name_set");
Ok, but this change means some other place should be also changed. I've used
logic from some other place, but can't remember which..

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:485: prop= RNA_def_property(srna,
"markers_count", PROP_INT, PROP_NONE);
Think it wasn't actually used, so removed (it was mostly for exporters which i
heard nothing about yet). It it something special needed to make len(markers)
return markersnr instead of possible iteration+incrementing counter or it'll
happen automatically?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:562: func= RNA_def_function(srna,
"get_marker", "rna_trackingTrack_marker_get");
made it marker_find_frame due to it returns marker for specified frame. Is it ok
for you?

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:599: prop= RNA_def_property(srna,
"active_track_index", PROP_INT, PROP_NONE);
Don't think so. It's index of active track in list of tracks used for 2d
stabilization. Making it property of tracks would mean that it's more general
property which i don't think be nice to share for other needs.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:611: prop= RNA_def_property(srna,
"max_scale", PROP_FLOAT, PROP_FACTOR);
Ok.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:678: prop= RNA_def_property(srna,
"is_reconstructed", PROP_BOOLEAN, PROP_NONE);
Long name is annoying, just wasn't sure about shorter name. is_valid sounds fine
for py api, but don;t think TRACKING_RECONSTRUCTED would be nice to be changed.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:706: func= RNA_def_function(srna,
"add", "rna_tracking_tracks_add");
If i understand you correct, you're talking about frame here. It can't be edited
atm.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:708: RNA_def_int(func, "framenr",
1, MINFRAME, MAXFRAME, "Frame", "Frame number to add tracks on", MINFRAME,
MAXFRAME);
Ok

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:742: prop= RNA_def_property(srna,
"active_track", PROP_POINTER, PROP_NONE);
In this case i'm ok with making it property of tracks :)

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
File source/blender/nodes/composite/nodes/node_composite_movieclip.c (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
source/blender/nodes/composite/nodes/node_composite_movieclip.c:68: /* now we
need a float buffer from the image with matching color management */
Indeed. Hopefully, made it in right way.

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
source/blender/nodes/composite/nodes/node_composite_movieclip.c:166: node->id=
G.main->movieclip.first;
Not sure if it's bad thing to do. I can tell that it helps a lot for artists..

[nazgul, 2011-10-24 16:56:11]

Updated to tomato rev41255

[ideasman42, 2011-10-24 23:02:55]

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

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:152:
if(camera->units==CAMERA_UNITS_MM) {
On 2011/10/24 16:36:08, nazgul wrote:
> Can display RNA setting be run-time so i can edit value in both of millimeters
> and pixels? And is there such option already?
> Also this can be done with two properties: focal_length_mm and
focal_length_px..

Currently we do both (which isn't great), ideally IMHO this would be done by the
UI as we do now with rotations, but this is tricky to setup, otherwise I'd
prefer 2 rna attributes as we have with camera lense / angle now. Just be sure
to set animateable for 1 only else 2 fcurves can fight :)

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:346: prop= RNA_def_property(srna,
"sensor_width", PROP_FLOAT, PROP_NONE);
On 2011/10/24 16:36:08, nazgul wrote:
> Eek. Can we discuss this kind of question after i'll publish new sensor size
> patch?
> As was discussed in ML, both of sensor sizes are needed and it'll be just an
> option to use automatic size (width or height depending on resolution) for
FOV,
> or use user-specified dimension to calculate FOV.
> But this place isn't really related on this sensor patch. It's a real width of
> rel sensor. This sensor also has got height which should be added here after
> that new sensor patch is ready, so it'll be fully correlate real camera
> intrinsics.

ok

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:426: prop= RNA_def_property(srna,
"enabled", PROP_BOOLEAN, PROP_NONE);
On 2011/10/24 16:36:08, nazgul wrote:
> Renamed to "enable". But not sure about use_marker, hide and
show_marker_frame.
> Is it just samples or i've used bad naming somewhere else?

Ehh. this is annoying, problem was that a while back we had way too many terms
for the same thing.

Eg -
enabele/disable/enabled/disabled/hide/hidden/hide_blah/show/show_blah/mute/use/use_blah/do_blah

In this case what about 'mute' ? - used for constraints, nla, shape key &
sequencer.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:485: prop= RNA_def_property(srna,
"markers_count", PROP_INT, PROP_NONE);
On 2011/10/24 16:36:08, nazgul wrote:
> Think it wasn't actually used, so removed (it was mostly for exporters which i
> heard nothing about yet). It it something special needed to make len(markers)
> return markersnr instead of possible iteration+incrementing counter or it'll
> happen automatically?

Collections have a length function which can be defined, it only needs to return
this value, similar examples exist elsewhere in blender.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:562: func= RNA_def_function(srna,
"get_marker", "rna_trackingTrack_marker_get");
On 2011/10/24 16:36:08, nazgul wrote:
> made it marker_find_frame due to it returns marker for specified frame. Is it
ok
> for you?

Yep

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:599: prop= RNA_def_property(srna,
"active_track_index", PROP_INT, PROP_NONE);
On 2011/10/24 16:36:08, nazgul wrote:
> Don't think so. It's index of active track in list of tracks used for 2d
> stabilization. Making it property of tracks would mean that it's more general
> property which i don't think be nice to share for other needs.

Ok

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:678: prop= RNA_def_property(srna,
"is_reconstructed", PROP_BOOLEAN, PROP_NONE);
On 2011/10/24 16:36:08, nazgul wrote:
> Long name is annoying, just wasn't sure about shorter name. is_valid sounds
fine
> for py api, but don;t think TRACKING_RECONSTRUCTED would be nice to be
changed.

think its fine to have them different but this depends on weather you think its
likely some other setting will be added later which also effects the valid
state.

I'd guess is that in this case its ok.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:706: func= RNA_def_function(srna,
"add", "rna_tracking_tracks_add");
On 2011/10/24 16:36:08, nazgul wrote:
> If i understand you correct, you're talking about frame here. It can't be
edited
> atm.

ok.

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
File source/blender/nodes/composite/nodes/node_composite_movieclip.c (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
source/blender/nodes/composite/nodes/node_composite_movieclip.c:166: node->id=
G.main->movieclip.first;
On 2011/10/24 16:36:08, nazgul wrote:
> Not sure if it's bad thing to do. I can tell that it helps a lot for artists..

Thinking about this more, and the closest comparison I can think of is assigning
a scene to a new scene node when there is only 1, same for an image.

And from thinking about it more IMHO it should be removed, otherwise if you try
to automate node creation you could end up having checks for ...

if len(bpy.data.movieclips) == 1: ...

Personally I like to keep low level functions like this predictable,  callers
shouldn't have to second-guess what blender is doing by checking the state.

When functionality like this is handy I think its better put in the operator.

[brechtvl, 2011-10-25 16:01:36]

Blenkernel changes review.

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesdna/DNA_...
File source/blender/makesdna/DNA_movieclip_types.h (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_movieclip_types.h:91: int ok;							/* 1 means
scopes are ok and re-calculaito is unneeded */
typo: re-calculaito

http://codereview.appspot.com/5285047/diff/32002/source/blender/blenkernel/BK...
File source/blender/blenkernel/BKE_tracking.h (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/blenkernel/BK...
source/blender/blenkernel/BKE_tracking.h:103: /* Distoriton/Undistortion */
Distoriton => Distortion

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

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/depsgraph.c:2034: /* also all objects which are
parented to tracking data should be re-calculated */
This doesn't seem right, looks like it's bypassing the depsgraph system and just
recalculating these objects always, even when nothing related changed?

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

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:82: strncpy(num,
full_name+head_len, numlen);
Use BLI_strncpy, strncpy does not do null termination of strings if they are too
long.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:262: /* regular moive cache */
moive => movie

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:415: /* exists? */
BLI_exists can be used instead.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:459: static void
real_ibuf_size(MovieClip *clip, MovieClipUser *user, ImBuf *ibuf, int *width,
int *height)
If the size of the original image is not a multiple of 4, this isn't accurate
for proxies. Not sure if this would be a problem in practice.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:531: imb_freerectfloatImBuf(ibuf);
I think the problem here may be that IMB_dupImBuf does not copy the
ibuf->profile flag. This looks like a bug in that function and might cause
problems in other places as well.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:581:
BLI_lock_thread(LOCK_MOVIECLIP);
This lock doesn't really make the movie cache thread safe? The reason being that
after return the ibuf, that ibuf might still get removed from the cache in
another thread. If the cache is large enough it's just less likely to happen.

It seems the tracking code calls this from threads, if they're all requesting
the same ibuf, it would be ok. But in that case the lock should be placed there
rather than here, because it would only be threadsafe because of the logic
there.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:1002: void unlink_movieclip(Main
*bmain, MovieClip *clip)
This doesn't seem to set all clip references to NULL. Scene, constraints and 3d
view bgpic are not checked, maybe more. It's not really required for a datablock
to support this, but if it is it should be complete.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:1008: /* text space */
text space => clip space

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

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:730: float *rrgbf= ibuf->rect_float
+ pixel*4;
tmpibuf => ibuf

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:732: *fp= (0.2126*rrgbf[0] +
0.7152*rrgbf[1] + 0.0722*rrgbf[2])/255;
This division by 255 seems wrong.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:752: unsigned char *pixels, *fp;
Small nitpick, fp is usually used for floats, cp for chars.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:770: float *rrgbf= ibuf->rect_float
+ pixel*4;
ibuf => tmpibuf

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:772: *fp= (0.2126*rrgbf[0] +
0.7152*rrgbf[1] + 0.0722*rrgbf[2]);
Missing scaling here, use FTOCHAR() here.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:1500: static unsigned char
*get_ucharbuf(ImBuf *ibuf)
The code here and in get_search_bytebuf could be deduplicated, also contains
same errors.

[nazgul, 2011-10-25 19:04:58]

Fixed most of issues reported by Campbell and Brecht. Not sure when updated
patchset would be commited, but there are some comments i wand to get answers.

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

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:426: prop= RNA_def_property(srna,
"enabled", PROP_BOOLEAN, PROP_NONE);
Hrm. It's current naming convention vs. how artists habited to name some
option.. I can see "enable" in some places too, but if you think it's not nice
it can be renamed.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:485: prop= RNA_def_property(srna,
"markers_count", PROP_INT, PROP_NONE);
Question was more about if i should use such callback or not. Added it to be
sure..

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_tracking.c:678: prop= RNA_def_property(srna,
"is_reconstructed", PROP_BOOLEAN, PROP_NONE);
Well, "valid" doesn't make much sense ad bit flag of MovieTracking. It'll make
sense if it'll be flag of MovieTracking->reconstrucion. I just need to know if
reconstruction was performed or not (display some extra information like average
error for reconstructed movies)

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
File source/blender/nodes/composite/nodes/node_composite_movieclip.c (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composi...
source/blender/nodes/composite/nodes/node_composite_movieclip.c:166: node->id=
G.main->movieclip.first;
Well, i'll agree and disagree with you here. It's nice to have core functions
have determined behavior, but in this case it's was question of "determined
behavior of core functions" vs. "concentrating node's logic in it's
implementation file". And i'm not sure that automated creation can cause issues
here. Most probably you'll just set node.movieclip to needed source and wouldn't
make any checks here.
There's already some confusing things with setting scene to RenderLayer node in
node_add_node and setting other settings to some node types in
ED_node_set_active. Also, i can't find operator for adding node -- stack tells
this function was called from ui_apply_but_funcs_after (via some other
functions). If we'll add this movieclip setting to node_add_node it wouldn't
"solve" issue with that automated creation, if we'll add this setting to other
function we'll have:
- Functions which weren't supposed to do such kind of things are changing node
settings
- De-centralization of node settings stuff. Different settings for different
nodes in different situations would happen from totally different parts of code.
I don't like this =\

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

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/depsgraph.c:2034: /* also all objects which are
parented to tracking data should be re-calculated */
Hrm, yes it looks crappy. Can't remember why this was added, probably for some
crappy constraints setup. Can't make things failed without this crappy code, so
lets delete it and see if it'll cause problems.

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

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:82: strncpy(num,
full_name+head_len, numlen);
Argh, always forgot about this.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:415: /* exists? */
Was thinking about this. BLI_exists behaves a bit differently:
- AFAIK, it doesn't check if file name passed to it is a file or it's a
directory
- It doesn't check if file can be read (e.g. you've got permission to read it).
Not sure if it's so critical, and it's also how add_image works =\

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:459: static void
real_ibuf_size(MovieClip *clip, MovieClipUser *user, ImBuf *ibuf, int *width,
int *height)
Was thinking about this. In fact, it's not so big issue doe to tracks coords are
in unified space (0..1). Or normalized, as you want.
It can be original size stored in MovieClip. Easy to change and it's not so
hight priority, imo.

P.S. You're commonly using movies and they'll fail if it's original size is not
a multiple of 16 (see #26837 i.e. ;))

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:531: imb_freerectfloatImBuf(ibuf);
Did you read comment above? It _is_ bug, and i know about it. It was _me_ who
reported it months ago. This workaround can be removed when that bug is fixed,
but until this the whole clip editor becomes almost useless when starting
working with compositor.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:581:
BLI_lock_thread(LOCK_MOVIECLIP);
Hrm. In fact problem is not cause by cache size. The only issue i can see here
is that IMB_freeImBuf isn't therad-safe. If we'll add IMB_freeImBuf_threadsafe
function which will just call IMB_freeImBuf from critical section and switch
movie/sequencer to use this function, this stuff would be totally thread safe.
Not sure if there's more simple solutions for this, but don't think it's really
critical issue. The same thing can happen with sequencer before my refactor here
(aka it's not new issue).

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/movieclip.c:1002: void unlink_movieclip(Main
*bmain, MovieClip *clip)
You're right. Totally forgot to maintain this function.

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

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:772: *fp= (0.2126*rrgbf[0] +
0.7152*rrgbf[1] + 0.0722*rrgbf[2]);
Right. Strange that still nobody noticed this. Does it mean float buffers aren't
needed when doing motion tracking? =\

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
source/blender/blenkernel/intern/tracking.c:1500: static unsigned char
*get_ucharbuf(ImBuf *ibuf)
Right.

[brechtvl, 2011-10-25 20:03:00]

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

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

http://codereview.appspot.com/5285047/diff/18002/release/scripts/presets/trac...
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_o...
File release/scripts/startup/bl_operators/clip.py (right):

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_o...
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_u...
File release/scripts/startup/bl_ui/space_clip.py (right):

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_u...
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_u...
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_u...
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_u...
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/in...
File source/blender/blenkernel/intern/tracking.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
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/in...
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...
File source/blender/editors/space_clip/tracking_ops.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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/inte...
File source/blender/makesrna/intern/rna_tracking.c (right):

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

[brechtvl, 2011-10-25 20:23:08]

> Well, i'll agree and disagree with you here. It's nice to have core functions
> have determined behavior, but in this case it's was question of "determined
> behavior of core functions" vs. "concentrating node's logic in it's
> implementation file". And i'm not sure that automated creation can cause
issues
> here. Most probably you'll just set node.movieclip to needed source and
wouldn't
> make any checks here.

I think this is ok behavior for the operator. Maybe the 'new' function in the
API could leave this as NULL (rna_NodeTree_node_new), but I also don't feel very
strongly about that, don't think it's an issue for scripts in practice.

> Was thinking about this. BLI_exists behaves a bit differently:
> - AFAIK, it doesn't check if file name passed to it is a file or it's a
> directory
> - It doesn't check if file can be read (e.g. you've got permission to read
it).
> Not sure if it's so critical, and it's also how add_image works =\

Ah right, this checks if it's a file, can leave it as is then.

> Was thinking about this. In fact, it's not so big issue doe to tracks coords
are
> in unified space (0..1). Or normalized, as you want.
> It can be original size stored in MovieClip. Easy to change and it's not so
> hight priority, imo.

Right, it doesn't seem like it would be much of an issue in practice.

> Did you read comment above? It _is_ bug, and i know about it. It was _me_ who
> reported it months ago. This workaround can be removed when that bug is fixed,
> but until this the whole clip editor becomes almost useless when starting
> working with compositor.

Ok, it wasn't clear to me how this particular function related to the
compositing problem in the bug report.

> Hrm. In fact problem is not cause by cache size. The only issue i can see here
> is that IMB_freeImBuf isn't therad-safe. If we'll add IMB_freeImBuf_threadsafe
> function which will just call IMB_freeImBuf from critical section and switch
> movie/sequencer to use this function, this stuff would be totally thread safe.
> Not sure if there's more simple solutions for this, but don't think it's
really
> critical issue. The same thing can happen with sequencer before my refactor
here
> (aka it's not new issue).

The threading issue I'm referring to is mostly when you use pixels from the ibuf
after getting it, since the cache could free it. I.e. one thread might get an
ibuf, another might get a different ibuf, forcing the first ibuf out of the
cache and freeing it.

Anyway, this might not happen in practice in this code so it's a minor issue. If
I wrote this code I'd put the locks in the tracking code to indicate that's it's
because of the the logic there that this is thread safe, but it's your choice.

[nazgul, 2011-10-25 20:32:00]

> > Hrm. In fact problem is not cause by cache size. The only issue i can see
here
> > is that IMB_freeImBuf isn't therad-safe. If we'll add
IMB_freeImBuf_threadsafe
> > function which will just call IMB_freeImBuf from critical section and switch
> > movie/sequencer to use this function, this stuff would be totally thread
safe.
> > Not sure if there's more simple solutions for this, but don't think it's
> really
> > critical issue. The same thing can happen with sequencer before my refactor
> here
> > (aka it's not new issue).
> 
> The threading issue I'm referring to is mostly when you use pixels from the
ibuf
> after getting it, since the cache could free it. I.e. one thread might get an
> ibuf, another might get a different ibuf, forcing the first ibuf out of the
> cache and freeing it.
> 
> Anyway, this might not happen in practice in this code so it's a minor issue.
If
> I wrote this code I'd put the locks in the tracking code to indicate that's
it's
> because of the the logic there that this is thread safe, but it's your choice.

That issue wouldn't happen if thread-safe IMB_freeImBuf function would be used:
cache increments ref count for buffer it returns, and IMB_freeImBuf destroys
actual data only when ref counter is zero, otherwise it'll just decrease ref
count. AFAIR, when buffer gets deleted fomr cache due to limit, it'll also be
just de-referenced instead of "silent" destroy when going out of cache size
limit.

[nazgul, 2011-10-26 07:47:48]

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

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

http://codereview.appspot.com/5285047/diff/18002/release/scripts/presets/trac...
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_o...
File release/scripts/startup/bl_operators/clip.py (right):

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_o...
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_u...
File release/scripts/startup/bl_ui/space_clip.py (right):

http://codereview.appspot.com/5285047/diff/18002/release/scripts/startup/bl_u...
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_u...
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_u...
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_u...
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/in...
File source/blender/blenkernel/intern/tracking.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/in...
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...
File source/blender/editors/space_clip/tracking_ops.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/editors/space...
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...
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...
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...
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...
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...
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.

[mierle, 2011-10-26 08:12:00]

I haven't looked through the reconstruction part yet.

http://codereview.appspot.com/5285047/diff/32002/extern/libmv/libmv-capi.cpp
File extern/libmv/libmv-capi.cpp (right):

http://codereview.appspot.com/5285047/diff/32002/extern/libmv/libmv-capi.cpp#...
extern/libmv/libmv-capi.cpp:42: 
Nitpick: What's the reason for the blank line here?

http://codereview.appspot.com/5285047/diff/32002/extern/libmv/libmv-capi.cpp#...
extern/libmv/libmv-capi.cpp:64: typedef struct libmv_RegionTracker {
Consider documenting why these three objects are glued in here. Also consider
documenting who owns the pointers in the struct.

Why not use a destructor?

http://codereview.appspot.com/5285047/diff/32002/extern/libmv/libmv-capi.cpp#...
extern/libmv/libmv-capi.cpp:87: void libmv_initLogging(const char *argv0)
Thanks for keeping this in place.

http://codereview.appspot.com/5285047/diff/32002/extern/libmv/libmv-capi.cpp#...
extern/libmv/libmv-capi.cpp:94: V3D::optimizerVerbosenessLevel = 0;
I can't wait until we can zap this guy.

[brechtvl, 2011-10-26 11:44:06]

> 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.

For me it's more clear after this change, but maybe it's more a matter of
intuition, I don't really have much to back this up, in 2.49 there were more
examples of this. I just expected to have the operator first as a 'title' and
then the properties of the operator below that.
 
> 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.

Ok, looks clear now.
 
> 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).

Outside might not be the right term, it's more when you click near the center
One case were this happened to me was, press Detect Features (all markers now
selected), then RMB click near the center of one of the markers. As far as I can
see, nothing happens then?

Actually, this seems to be because of the slide marker check. When you do a
mouse press on a marker and drag, it will move all markers. This seems strange
to me but maybe I'm thinking too much about how objects work.

Also noticed now the extend behavior is different than on objects in the 3d
view. There if you click on an object that is selected but not active, it will
make it active but not deselect it. Only when clicking a second time it will get
deselected.

> 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.

I was confused for a moment when I clicked this button and it did nothing, and
turned out I didn't know markers had to be selected for this operation. The
message shouldn't really get in the way in normal workflow, because you wouldn't
intentionally click the button knowing that there is nothing selected. Not a big
deal though, it's also clear after reading the tooltip.

> 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.

Ok, maybe it's just me :) Latest description is clear now for me.

[brechtvl, 2011-11-01 18:48:39]

After discussion with Sergey at the conference, I think the threading thing is
actually not an issue due to the cache referencing the imbuf and free
unreferencing it. Also the selection problems were fixed. So I don't have any
remaining comments :)