Code review - Issue 5274047: Camera sensor sizehttps://codereview.appspot.com/2011-11-01T18:44:51+00:00rietveld
Message from unknown
2011-10-15T16:46:23+00:00nazgulurn:md5:1f9d164eb7da1eb21fd2ae8a1663031a
Message from g.ulairi@gmail.com
2011-10-15T16:46:24+00:00nazgulurn:md5:8419ad234cb70e7b7cdc63a6655e8a84
Message from unknown
2011-10-15T18:33:18+00:00nazgulurn:md5:7137bbdce6a3a0c3a1c976b633a59425
Message from g.ulairi@gmail.com
2011-10-15T18:33:18+00:00nazgulurn:md5:7f1f3461253fc650c0eca51f974177d0
Updated patchset. Hope it'll work now
Message from brechtvanlommel@gmail.com
2011-10-19T13:14:15+00:00brechtvlurn:md5:88098070e4012d6f9def3139651520a7
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_math_rotation.h
File source/blender/blenlib/BLI_math_rotation.h (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/blenlib/BLI_math_rotation.h#newcode185
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_view3d/view3d_view.c
File source/blender/editors/space_view3d/view3d_view.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/editors/space_view3d/view3d_view.c#newcode971
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/intern/rna_camera.c
File source/blender/makesrna/intern/rna_camera.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/intern/rna_camera.c#newcode119
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/intern/rna_camera.c#newcode141
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/RAS_FramingManager.cpp
File source/gameengine/Rasterizer/RAS_FramingManager.cpp (right):
http://codereview.appspot.com/5274047/diff/4001/source/gameengine/Rasterizer/RAS_FramingManager.cpp#newcode52
source/gameengine/Rasterizer/RAS_FramingManager.cpp:52: * ^Deprecated Comment
Just remove or replace this comment?
Message from g.ulairi@gmail.com
2011-10-19T13:54:24+00:00nazgulurn:md5:7684a3aff0a55114ec7d96cd86e7e6d6
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_math_rotation.h
File source/blender/blenlib/BLI_math_rotation.h (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/blenlib/BLI_math_rotation.h#newcode185
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_view3d/view3d_view.c
File source/blender/editors/space_view3d/view3d_view.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/editors/space_view3d/view3d_view.c#newcode971
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/intern/rna_camera.c
File source/blender/makesrna/intern/rna_camera.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/makesrna/intern/rna_camera.c#newcode141
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/RAS_FramingManager.cpp
File source/gameengine/Rasterizer/RAS_FramingManager.cpp (right):
http://codereview.appspot.com/5274047/diff/4001/source/gameengine/Rasterizer/RAS_FramingManager.cpp#newcode52
source/gameengine/Rasterizer/RAS_FramingManager.cpp:52: * ^Deprecated Comment
Yeah, this comment means nothing now. Will remove it.
Message from brechtvanlommel@gmail.com
2011-10-19T14:09:09+00:00brechtvlurn:md5:681bcf690b6400896d7c2cb0cf2d9828
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.
Message from g.ulairi@gmail.com
2011-10-19T14:19:31+00:00nazgulurn:md5:60bb1556f1959af8f3b72ede6c5eba96
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.
Message from ideasman42@gmail.com
2011-10-20T12:43:41+00:00ideasman42urn:md5:b7e652261d5a904eb4c9296a10d9d5ff
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/intern/object.c
File source/blender/blenkernel/intern/object.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/blenkernel/intern/object.c#newcode3021
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/intern/MOD_uvproject.c
File source/blender/modifiers/intern/MOD_uvproject.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/intern/MOD_uvproject.c#newcode171
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.
Message from g.ulairi@gmail.com
2011-10-20T16:04:50+00:00nazgulurn:md5:a1c0832ffa58caf90f29ad5fc8a00d10
http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/intern/MOD_uvproject.c
File source/blender/modifiers/intern/MOD_uvproject.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/intern/MOD_uvproject.c#newcode171
source/blender/modifiers/intern/MOD_uvproject.c:171: sensor_x= camera->sensor_x;
I'm not sure what "referenced from the object" means.
Message from ideasman42@gmail.com
2011-10-20T21:45:13+00:00ideasman42urn:md5:dbbde528b13e20ae58406e54320f5040
Clarified comment about UV-project modifier.
http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/intern/MOD_uvproject.c
File source/blender/modifiers/intern/MOD_uvproject.c (right):
http://codereview.appspot.com/5274047/diff/4001/source/blender/modifiers/intern/MOD_uvproject.c#newcode171
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;
Message from brechtvanlommel@gmail.com
2011-11-01T18:44:51+00:00brechtvlurn:md5:50dbacf6d4e1475e9c5ee53872ea2b10
After latest changes in tomato branch, I'm not aware of any remaining issues, so: LGTM :)