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

Better Camera unproject #2001

Merged
merged 12 commits into from
Feb 21, 2023
Merged

Better Camera unproject #2001

merged 12 commits into from
Feb 21, 2023

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Feb 8, 2023

Motivation and Context

Previously camera unproject function would return a normalized ray which was useful for distance calculations and raycasting, but could not be used directly with depth image values (on view plane) for depth unprojection into point clouds.

This PR adds two features:

  1. Added normalized variable to unproject (default true)
  2. When normalized==False, unproject now returns a correctly scaled ray direction in world units to get a precise 3D location from a depth pixel value.

How Has This Been Tested

Modified unit test to validate normalized variable.
New unit tests demonstrate depth unprojection and leverage Bullet collision detection to validate the feature.

Example images from optional visual test output showing a sphere object for a random depth unprojected pixel:
tmpabvkkg6z
tmpftysrtyg

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 8, 2023
ray.origin)
.normalized();
ray.origin) /
1000.0;
Copy link
Contributor

@jturner65 jturner65 Feb 8, 2023

Choose a reason for hiding this comment

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

Why scaled by 1/1000? Should this be a defined constant somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the intent of this change, perhaps some documentation regarding rescaling the normalized camera ray to fit the desired use would be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, this is the far plane distance.
I updated the code and docs such that the scaling is on the user side.
Also added a test to validate this.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about we get the far distance by doing this right there? @mosra does this look appropriate?

auto zFar = Frustum::fromMatrix(projectionMatrix()*cameraMatrix()).far();
auto dist = zFar(3)/zFar.xyz().length();

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Frustum class is unnecessary overhead -- it calculates also all other planes and you need just one of them, and only its distance.

The "most obvious" way to calculate the distance of the far plane is by doing an inverse transform of a point lying on the far plane (i.e., calculating what point gets projected to (0, 0, 1)). Something like this:

projectionMatrix().inverted().transformPoint(Vector3::zAxis(1.0f)).z()

But that's imprecise and not fast either due to the inversion. After looking at the Matrix4::perspectiveProjection() docs and a bit of math suffering I think the equation is like this:

near = projection[3][2]/(projection[2][2] - 1.0f);
far = projection[3][2]/(1.0f + projection[2][2]);

I could add those directly on the Matrix4 class I think, just need to do a bit of testing and correctly handle infinite far plane.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mosra ! Do we not need the product of the projection & camera transforms for the far plane calculation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes -- the equations I have give you the distance to where far Z plane is relative to the camera. So then if you transform the camera somewhere, you don't need to do the complicated extraction of the far plane again, only transforming that "far vector" with the camera matrix. I think. My math is rusty.

// Negative Z is forward
Vector3 cameraRelativeZIsHere = cameraMatrix().transformPoint(Vector3::zAxis(-far));

Copy link
Contributor

@jturner65 jturner65 left a comment

Choose a reason for hiding this comment

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

LGTM, except for pesky pylint :) .

src/esp/gfx/RenderCamera.cpp Outdated Show resolved Hide resolved

if (normalized) {
ray.direction = ray.direction.normalized();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Again ignore if this utility isn't meant to be used in performance-sensitive scenarios.)

A better way would be to have this split into two functions, one unproject() that doesn't normalize, and an unprojectNormalized() that calls unproject(), normalizes its result and returns it. That way there's no branch. (Though that way it'll be a breaking change, because unproject() will not normalize anymore, so you might want to invent different naming of the two overloads.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thanks. I thought about this also, but didn't want a breaking change or to add more bulk to the API. I think this one can stay for now.

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

LGTM! Left some minor remarks.

@@ -30,6 +31,7 @@
"equirect_semantic_sensor": False,
"seed": 1,
"physics_config_file": "data/default.physics_config.json",
"enable_physics": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the consequences of enabling physics by default here? Do we want to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. This means any Simulator initialized from the default settings util will have access to Bullet API (if installed).
Given that tests are passing, I don't think this is problem.

Some justification:

  1. If user installed bullet compatible Habitat build, they should get Bullet API by default.
  2. This incurs very little overhead for static scenes (builds collision meshes, but doesn't do any heavy compute), so it shouldn't reduce performance for non-physics use cases most of the time (e.g. PointNav with HM3D).
  3. This is a fairly new utility Dict for quick-start and is not currently used downstream by Habitat-lab (outside of one test for the rearrange task which is physics based), so it is not likely a breaking change.

We could add the breaking change tag to the PR and mention this in the description.
If you think this warrants more discussion I could move this change to its own PR as it is not necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I think we can proceed with it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aclegg3 @0mdc What's the verdict on this change? Should we add the breaking change tag? Otherwise, this PR looks good to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it would be preferable to add this change to another PR for traceability. However, I don't feel strongly either way.

random.randint(0, render_camera.viewport[0] - 1),
random.randint(0, render_camera.viewport[1] - 1),
)
# NOTE: use un-normlized rays scaled to unit z distance for this application
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: normlized

@jturner65 jturner65 merged commit 8648a23 into main Feb 21, 2023
@jturner65 jturner65 deleted the better-unproject branch February 21, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants