Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Achint08
Copy link
Contributor

@Achint08 Achint08 commented May 30, 2022

  1. Adds properties related to:
  • Focus
  • Field of view.
  1. Adds tests related to changes.

@Achint08
Copy link
Contributor Author

@Conchylicultor Please review!

Copy link
Collaborator

@Conchylicultor Conchylicultor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

def fov(self) -> float:
"""Field of view in radians (`(fov_w, fov_h)`)."""

return (self.fov_w, self.fov_h)
Copy link
Collaborator

@Conchylicultor Conchylicultor May 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename fov -> fov_hw (as .fov sounds like it should be a single value)

return (self.w, self.h)

@property
def fw(self) -> float:
Copy link
Collaborator

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 than fw (e.g. as focal is sometimes defined in mm).

So I would remove those fw, fh

f'different: {self.focal_px_wh}'
)

if self.fw != self.fh:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

focal is focal, so we should use isclose

return (self.w, self.h)

@property
def fw(self) -> float:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 @vectorize_method similarly to the scale property in

def scale(self):

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 should likely be a FloatArray['*shape 2']

return self.focal_length[1]

@property
def focal_px_wh(self) -> tuple[float, float]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant with focal_length defined line 110.

To keep the API minimal, I would remove this property and rename focal_length -> focal_px_wh in the dataclass attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants