-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes related to #13: Adds focal, fov #24
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -107,6 +107,7 @@ class implementation. | |||
repr=False, | ||||
init=False, | ||||
) | ||||
focal_length: Tuple[float, float] | ||||
|
||||
@property | ||||
def h(self) -> int: | ||||
|
@@ -118,14 +119,60 @@ def w(self) -> int: | |||
|
||||
@property | ||||
def hw(self) -> tuple[int, int]: | ||||
"""`(Height, Width)` in pixel (for usage in `(i, j)` coordinates).""" | ||||
"""`(Height, Width)` in pixel (for usage in `(u, v)` coordinates).""" | ||||
return (self.h, self.w) | ||||
|
||||
@property | ||||
def wh(self) -> tuple[int, int]: | ||||
"""`(Width, Height)` in pixel (for usage in `(u, v)` coordinates).""" | ||||
"""`(Width, Height)` in pixel (for usage in `(i, j)` coordinates).""" | ||||
return (self.w, self.h) | ||||
|
||||
@property | ||||
def fw(self) -> float: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Contrary to the resolution which is always constant across all camera stacked together, focal length can vary for individual cameras. This means we should wrap the property around visu3d/visu3d/dc_arrays/transformation.py Line 224 in 11c8f97
This would allow for example to vary the focal length across camera, like: cams = v3d.PinholeCamera(
resolution=(h, w),
focal_in_px=[200, 240, 260, 300],
)
cams.shape == (4,) # 4 cameras stacked together with focal 200, 240,... This also mean |
||||
"""Focal length (along x-axis) in pixels (for usage in `(i, j)` coordinates).""" | ||||
return self.focal_length[0] | ||||
|
||||
@property | ||||
def fh(self) -> float: | ||||
"""Focal length (along y-axis) in pixels (for usage in `(i, j)` coordinates).""" | ||||
return self.focal_length[1] | ||||
|
||||
@property | ||||
def focal_px_wh(self) -> tuple[float, float]: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems redundant with To keep the API minimal, I would remove this property and rename |
||||
"""Focal length in pixel (`(fw, fh)`).""" | ||||
return (self.fw, self.fh) | ||||
|
||||
@property | ||||
def focal_px(self) -> float: | ||||
"""Unique Focal length in pixels (when fw == fh).""" | ||||
|
||||
def _err_msg(): | ||||
return ( | ||||
'Cannot get `CameraSpec.focal_px` when fw and fh are ' | ||||
f'different: {self.focal_px_wh}' | ||||
) | ||||
|
||||
if self.fw != self.fh: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. focal is focal, so we should use |
||||
raise ValueError(_err_msg()) | ||||
|
||||
return self.fw | ||||
|
||||
@property | ||||
def fov_w(self) -> float: | ||||
"""Field of view (horizontal) in radians (for usage in `(i, j)` coordinates).""" | ||||
return 2 * self.xnp.arctan(self.w / (2 * self.fw)) | ||||
|
||||
@property | ||||
def fov_h(self) -> float: | ||||
"""Field of view (vertical) in radians (for usage in `(i, j)` coordinates).""" | ||||
return 2 * self.xnp.arctan(self.h / (2 * self.fh)) | ||||
|
||||
@property | ||||
def fov(self) -> float: | ||||
"""Field of view in radians (`(fov_w, fov_h)`).""" | ||||
|
||||
return (self.fov_w, self.fov_h) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rename |
||||
|
||||
# @abc.abstractmethod | ||||
@property | ||||
def px_from_cam(self) -> transformation.TransformBase: | ||||
|
@@ -300,8 +347,7 @@ def from_focal( | |||
[0, 0, 1], | ||||
]) | ||||
return cls( | ||||
K=K, | ||||
resolution=resolution, | ||||
K=K, resolution=resolution, focal_length=(focal_in_px, focal_in_px) | ||||
) | ||||
|
||||
@property | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think users will often need this property and this can easily be extracted with
cam.focal_px_wh[0]
. Which is more explicit thanfw
(e.g. as focal is sometimes defined in mm).So I would remove those
fw
,fh