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

[ideasman42 at gmail.com]

pedantic rna review


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

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

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#newcode139
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/intern/rna_tracking.c#newcode152
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/intern/rna_tracking.c#newcode269
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/intern/rna_tracking.c#newcode295
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/intern/rna_tracking.c#newcode346
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/intern/rna_tracking.c#newcode426
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/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");
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/intern/rna_tracking.c#newcode485
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/intern/rna_tracking.c#newcode562
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/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);
again, could this be made an attribute of 'tracks' ?

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);
pedantic, doesnt seem self consistant, 'scale_max' ?

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);
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/intern/rna_tracking.c#newcode706
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/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);
pedantic, call it `frame`

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);
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/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
*/
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/composite/nodes/node_composite_movieclip.c#newcode166
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.

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