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

[g.ulairi at gmail.com]

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


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

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/intern/rna_tracking.c#newcode152
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/intern/rna_tracking.c#newcode269
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/intern/rna_tracking.c#newcode295
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/intern/rna_tracking.c#newcode346
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/intern/rna_tracking.c#newcode426
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/intern/rna_tracking.c#newcode449
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/intern/rna_tracking.c#newcode485
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/intern/rna_tracking.c#newcode562
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/intern/rna_tracking.c#newcode599
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/intern/rna_tracking.c#newcode611
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/intern/rna_tracking.c#newcode678
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/intern/rna_tracking.c#newcode706
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/intern/rna_tracking.c#newcode708
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/intern/rna_tracking.c#newcode742
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/composite/nodes/node_composite_movieclip.c
File source/blender/nodes/composite/nodes/node_composite_movieclip.c
(right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composite/nodes/node_composite_movieclip.c#newcode68
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/composite/nodes/node_composite_movieclip.c#newcode166
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..

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