Skip to content

create extrinsic from eye point #65

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

Conversation

davegreenwood
Copy link
Contributor

@davegreenwood davegreenwood commented Feb 16, 2020

Create extrinsic parameters from eye point.
Create the rotation and translation from an eye point, look-at point and up vector.
see:
https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/gluLookAt.xml

It is arguably easier to initialise a camera position as a point in the world rather than an angle.

BlendParams background_color is immutable , type hint as a sequence allows setting new values in constructor.
return R, t from a camera eye point and look at target.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 16, 2020
@nikhilaravi
Copy link
Contributor

@davegreenwood, as per the the guidelines in Contributing.md please add:

  • a description of the changes in the Pull Request and why they are necessary.
  • if you've added code that should be tested, add tests.

T = -torch.bmm(R.transpose(1, 2), eye[:, :, None])[:, :, 0]
return R, T


def look_at_view_transform(
dist,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a new function, I would suggest passing in the camera_position as an input to look_at_view_transform. Then we can check if either the spherical angles have been provided or the camera position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikhilaravi - that's a good idea - I'll make that change.

Comment on lines 689 to 690
if __name__ == "__main__":
unittest.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these two lines.

@nikhilaravi
Copy link
Contributor

@davegreenwood thanks for making the updates. I added a few more comments to the tests. Please make these changes and then I think we can merge this PR.

@nikhilaravi nikhilaravi self-assigned this Feb 24, 2020
Comment on lines 142 to 146
R, t = look_at_view_transform(dist, elev, azim)
# Using default dist=1.0, elev=0.0, azim=0.0
R_default, t_default = look_at_view_transform()
# R should be identity, t should be (0, 0, 1)
R_expected = torch.tensor([np.eye(3)], dtype=torch.float32)
Copy link
Contributor

Choose a reason for hiding this comment

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

For this test it is sufficient to just check the default matches the expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, I trimmed those unnecessary lines.

@nikhilaravi
Copy link
Contributor

@davegreenwood apologies for the delay, this looks good - I will merge the PR!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@nikhilaravi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

dist = math.sqrt(2)
elev = math.pi / 4
azim = 0.0
eye = ((0.0, 1.0, -1.0), )
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the camera_position_from_spherical_angles function has been modified so that z is positive, you will need to change eye to eye = ((0.0, 1.0, 1.0), ) in order for the tests to pass.

Copy link
Contributor Author

@davegreenwood davegreenwood Mar 17, 2020

Choose a reason for hiding this comment

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

hi @nikhilaravi, I've made that required change. The test does not pass in my branch, but if I did a rebase it would, due to line 947 in cameras.py.

@nikhilaravi
Copy link
Contributor

@davegreenwood can you please make the one small change requested? After that I can merge your PR.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@nikhilaravi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@nikhilaravi merged this pull request in 2480723.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants