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

[ideasman42 at gmail.com]

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) {
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/intern/rna_tracking.c#newcode346
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/intern/rna_tracking.c#newcode426
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/intern/rna_tracking.c#newcode485
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/intern/rna_tracking.c#newcode562
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/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);
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/intern/rna_tracking.c#newcode678
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/intern/rna_tracking.c#newcode706
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/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#newcode166
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.

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