Camera sensor size

Implementation of configurable sensor size.

It's a bit simplified version of patch originally provided by Matt Ebb (iirc) to deal properly with current render pipeline. Implementing all that filmgate things discussed in vfx mailing list will need to be supported by render engines.

[nazgul, 2011-10-15 18:33:18]

Updated patchset. Hope it'll work now

[brechtvl, 2011-10-19 13:14:15]

Sorry to bring this up again, but I'm still a bit confused by the sensor stuff
regarding in the case of a "portrait" instead of "landscape" aspect ratio, see
comments.

http://codereview.appspot.com/5274047/diff/4001/source/blender/blenlib/BLI_ma...
File source/blender/blenlib/BLI_math_rotation.h (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/blenlib/BLI_ma...
source/blender/blenlib/BLI_math_rotation.h:185: float hfov_to_focallength(float
hfov, float sensor_x);
I wouldn't name these function specifically for horizontal values, they work
fine for vertical too.

http://codereview.appspot.com/5274047/diff/4001/source/blender/editors/space_...
File source/blender/editors/space_view3d/view3d_view.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/editors/space_...
source/blender/editors/space_view3d/view3d_view.c:971: float lens, sensor=32.f,
fac, x1, y1, x2, y2;
It might be good to replace these hardcoded 32 values in a few places with a
#define in DNA_camera_types.h. Also minor nitpick is that in some places it's
numbers are types as x.f while in others it's x.0f, the latter is the de facto
standard in the code.

http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_camera.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_camera.c:119: RNA_def_property_ui_text(prop,
"Field of View", "Camera lens horizontal field of view in degrees");
This description and the one for the sensor size doesn't seem to be actually
right. It's the horizontal or vertical fov depending on the aspect ratio.

http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_camera.c:141: prop= RNA_def_property(srna,
"sensor_width", PROP_FLOAT, PROP_NONE);
Similarly here, this is called sensor_width, but I guess that's not really
accurate because it is the sensor height for certain aspect ratios? Or maybe
it's a matter interpretation, where you assume that you'd physically rotate the
camera to get such images?

http://codereview.appspot.com/5274047/diff/4001/source/gameengine/Rasterizer/...
File source/gameengine/Rasterizer/RAS_FramingManager.cpp (right):

http://codereview.appspot.com/5274047/diff/4001/source/gameengine/Rasterizer/...
source/gameengine/Rasterizer/RAS_FramingManager.cpp:52: * ^Deprecated Comment
Just remove or replace this comment?

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

I'm not sure about "standard" pipeline -- when it's needed to know exact
settings of camera, but as we've discussed with Francois, when you're doing
motion tracking you can't always name sensor with, focal length and pixel aspect
-- sometimes you don't know some of parameters. It's easy to re-calculate them
in head, but it's not what artists wants to do. So for motion tracking vertical
sensor size makes more sense.

http://codereview.appspot.com/5274047/diff/4001/source/blender/blenlib/BLI_ma...
File source/blender/blenlib/BLI_math_rotation.h (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/blenlib/BLI_ma...
source/blender/blenlib/BLI_math_rotation.h:185: float hfov_to_focallength(float
hfov, float sensor_x);
You're right. Didn't notice this when when was cleaning up patch, Will rename
them.

http://codereview.appspot.com/5274047/diff/4001/source/blender/editors/space_...
File source/blender/editors/space_view3d/view3d_view.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/editors/space_...
source/blender/editors/space_view3d/view3d_view.c:971: float lens, sensor=32.f,
fac, x1, y1, x2, y2;
Sounds reasonable. But what about gameengine? It also uses 32 as default value
for sensor size but camera's dna isn't available there. Is it fine to keep this
value hardcoed in RAS_CameraData?

http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_camera.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_camera.c:141: prop= RNA_def_property(srna,
"sensor_width", PROP_FLOAT, PROP_NONE);
Yes, it's more about making settings more "physically". In original patch it was
also a way to define vertical fov, but it wasn't really used to i removed that
setting for a while, but run into complains from artists about missed option,

http://codereview.appspot.com/5274047/diff/4001/source/gameengine/Rasterizer/...
File source/gameengine/Rasterizer/RAS_FramingManager.cpp (right):

http://codereview.appspot.com/5274047/diff/4001/source/gameengine/Rasterizer/...
source/gameengine/Rasterizer/RAS_FramingManager.cpp:52: * ^Deprecated Comment
Yeah, this comment means nothing now. Will remove it.

[brechtvl, 2011-10-19 14:09:09]

Also mailed bf-vfx about aspect ratio question, I'll review the full sensor
patch, hopefully that can be solved quickly, but this could go in earlier if
needed.

> Sounds reasonable. But what about gameengine? It also uses 32 as default value
> for sensor size but camera's dna isn't available there. Is it fine to keep
this
> value hardcoed in RAS_CameraData?

Yes, that's ok.

[nazgul, 2011-10-19 14:19:31]

Yeah, saw your mail. Thanks for help here. It's not more about difficulty of
changes, more about understanding how users expects this things works. Anyway,
if we'll find it's difficult to solve in correct way, it wouldn't really be a
stopper for motion tracking stuff -- i can easily re-calculate focal length to
sensor size=32 when applying tracking stuff on blender's camera.

[ideasman42, 2011-10-20 12:43:41]

patch looks good, would rename all references to width / x --> size, some other
comments inline.

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

http://codereview.appspot.com/5274047/diff/4001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/object.c:3021: if(rd->xasp*winx >=
rd->yasp*winy)	viewfac= ((*lens) * winx) / (*sensor_x);
*picky* - looks like a sensor size rather than a sensor_x since it uses maximum
dimension, perhaps should be named sensor_size?

http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/inte...
File source/blender/modifiers/intern/MOD_uvproject.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_uvproject.c:171: sensor_x= camera->sensor_x;
Dont think its correct to use the scene camera here, instead it should use the
sensor size from the camera referenced from the object and fallback to 32 if its
not a camera.

[nazgul, 2011-10-20 16:04:50]

http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/inte...
File source/blender/modifiers/intern/MOD_uvproject.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_uvproject.c:171: sensor_x= camera->sensor_x;
I'm not sure what "referenced from the object" means.

[ideasman42, 2011-10-20 21:45:13]

Clarified comment about UV-project modifier.

http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/inte...
File source/blender/modifiers/intern/MOD_uvproject.c (right):

http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_uvproject.c:171: sensor_x= camera->sensor_x;
On 2011/10/20 16:04:50, nazgul wrote:
> I'm not sure what "referenced from the object" means. 

Each projector checks if its a camera type, when it is its sensor size should be
used, else if projecting from an empty - or any non camera - the default (32)
should be used.

Or put differently, the UV project modifier should not depend or act any
differently based on the scenes camera.

The camera 'cam's sensor size should be used (lines below).
----
if(projectors[i].ob->type == OB_CAMERA) {
    cam = (Camera *)projectors[i].ob->data;

[brechtvl, 2011-11-01 18:44:51]

After latest changes in tomato branch, I'm not aware of any remaining issues,
so: LGTM :)