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
4 changes: 2 additions & 2 deletions src/esp/bindings/GfxBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ void initGfxBindings(py::module& m) {
"height"_a, "znear"_a, "zfar"_a, "scale"_a)
.def(
"unproject", &RenderCamera::unproject,
R"(Unproject a 2D viewport point to a 3D ray with its origin at the camera position.)",
"viewport_point"_a)
R"(Unproject a 2D viewport point to a 3D ray with its origin at the camera position. Ray direction is optionally normalized. Non-normalized rays originate at the camera location and terminate at a view plane one unit down the Z axis.)",
"viewport_point"_a, "normalized"_a = true)
.def_property_readonly("node", nodeGetter<RenderCamera>,
"Node this object is attached to")
.def_property_readonly("object", nodeGetter<RenderCamera>,
Expand Down
16 changes: 13 additions & 3 deletions src/esp/gfx/RenderCamera.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,8 @@ size_t RenderCamera::filterTransforms(DrawableTransforms& drawableTransforms,
return drawableTransforms.size();
}

esp::geo::Ray RenderCamera::unproject(const Mn::Vector2i& viewportPosition) {
esp::geo::Ray RenderCamera::unproject(const Mn::Vector2i& viewportPosition,
bool normalized) {
esp::geo::Ray ray;
ray.origin = object().absoluteTranslation();

Expand All @@ -209,11 +210,20 @@ esp::geo::Ray RenderCamera::unproject(const Mn::Vector2i& viewportPosition) {
Magnum::Vector2{1.0f},
1.0};

// compute the far plane
auto zFar =
Mn::Frustum::fromMatrix(projectionMatrix() * cameraMatrix()).far();
float farDistance = zFar[3] / zFar.xyz().length();

ray.direction =
((object().absoluteTransformationMatrix() * projectionMatrix().inverted())
aclegg3 marked this conversation as resolved.
Show resolved Hide resolved
.transformPoint(normalizedPos) -
ray.origin)
.normalized();
ray.origin) /
farDistance;

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.

return ray;
}

Expand Down
9 changes: 7 additions & 2 deletions src/esp/gfx/RenderCamera.h
Original file line number Diff line number Diff line change
Expand Up @@ -223,16 +223,21 @@ class RenderCamera : public MagnumCamera {
* following rendering pass, otherwise false
*/
bool useDrawableIds() const { return useDrawableIds_; }

/**
* @brief Unproject a 2D viewport point to a 3D ray with origin at camera
* position.
* position. Ray direction is optionally normalized. Non-normalized rays
* originate at the camera location and terminate at a view plane one unit
* down the Z axis.
*
* @param viewportPosition The 2D point on the viewport to unproject
* ([0,width], [0,height]).
* @param normalized If true(default), normalize ray direction.
* @return a @ref esp::geo::Ray with unit length direction or zero direction
* if failed.
*/
esp::geo::Ray unproject(const Mn::Vector2i& viewportPosition);
esp::geo::Ray unproject(const Mn::Vector2i& viewportPosition,
bool normalized = true);

/**
* @brief Query the cached number of Drawables visible after frustum culling
Expand Down
8 changes: 8 additions & 0 deletions src_python/habitat_sim/utils/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"default_agent": 0,
"sensor_height": 1.5,
"hfov": 90,
"zfar": 1000.0,
"color_sensor": True,
"semantic_sensor": False,
"depth_sensor": False,
Expand All @@ -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.

}
# [/default_sim_settings]

Expand Down Expand Up @@ -79,6 +81,7 @@ def create_camera_spec(**kw_args):
color_sensor_spec = create_camera_spec(
uuid="color_sensor",
hfov=settings["hfov"],
far=settings["zfar"],
sensor_type=habitat_sim.SensorType.COLOR,
sensor_subtype=habitat_sim.SensorSubType.PINHOLE,
)
Expand All @@ -88,6 +91,7 @@ def create_camera_spec(**kw_args):
depth_sensor_spec = create_camera_spec(
uuid="depth_sensor",
hfov=settings["hfov"],
far=settings["zfar"],
sensor_type=habitat_sim.SensorType.DEPTH,
channels=1,
sensor_subtype=habitat_sim.SensorSubType.PINHOLE,
Expand All @@ -98,6 +102,7 @@ def create_camera_spec(**kw_args):
semantic_sensor_spec = create_camera_spec(
uuid="semantic_sensor",
hfov=settings["hfov"],
far=settings["zfar"],
sensor_type=habitat_sim.SensorType.SEMANTIC,
channels=1,
sensor_subtype=habitat_sim.SensorSubType.PINHOLE,
Expand All @@ -107,6 +112,7 @@ def create_camera_spec(**kw_args):
if settings["ortho_rgba_sensor"]:
ortho_rgba_sensor_spec = create_camera_spec(
uuid="ortho_rgba_sensor",
far=settings["zfar"],
sensor_type=habitat_sim.SensorType.COLOR,
sensor_subtype=habitat_sim.SensorSubType.ORTHOGRAPHIC,
)
Expand All @@ -115,6 +121,7 @@ def create_camera_spec(**kw_args):
if settings["ortho_depth_sensor"]:
ortho_depth_sensor_spec = create_camera_spec(
uuid="ortho_depth_sensor",
far=settings["zfar"],
sensor_type=habitat_sim.SensorType.DEPTH,
channels=1,
sensor_subtype=habitat_sim.SensorSubType.ORTHOGRAPHIC,
Expand All @@ -124,6 +131,7 @@ def create_camera_spec(**kw_args):
if settings["ortho_semantic_sensor"]:
ortho_semantic_sensor_spec = create_camera_spec(
uuid="ortho_semantic_sensor",
far=settings["zfar"],
sensor_type=habitat_sim.SensorType.SEMANTIC,
channels=1,
sensor_subtype=habitat_sim.SensorSubType.ORTHOGRAPHIC,
Expand Down
102 changes: 94 additions & 8 deletions tests/test_gfx.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# This source code is licensed under the MIT license found in the
# LICENSE file in the root directory of this source tree.

import random
from os import path as osp

import magnum as mn
Expand All @@ -19,13 +20,21 @@
not osp.exists("data/scene_datasets/habitat-test-scenes/skokloster-castle.glb"),
reason="Requires the habitat-test-scenes",
)
def test_unproject():
@pytest.mark.skipif(
not habitat_sim.built_with_bullet,
reason="Bullet physics used for validation.",
)
@pytest.mark.parametrize("zfar", [500, 1000, 1500])
def test_unproject(zfar):
cfg_settings = habitat_sim.utils.settings.default_sim_settings.copy()

# configure some settings in case defaults change
cfg_settings["scene"] = "data/scene_datasets/habitat-test-scenes/apartment_1.glb"
cfg_settings["width"] = 101
cfg_settings["height"] = 101
cfg_settings["enable_physics"] = True
cfg_settings["depth_sensor"] = True
cfg_settings["zfar"] = zfar
cfg_settings["width"] = 501
cfg_settings["height"] = 501
cfg_settings["sensor_height"] = 0
cfg_settings["color_sensor"] = True

Expand All @@ -37,22 +46,99 @@ def test_unproject():
sim.agents[0].scene_node.translation = mn.Vector3(0.5, 0, 0)

# setup camera
far_plane = sim._sensors["depth_sensor"]._sensor_object.far_plane_dist
assert zfar == far_plane
render_camera = sim._sensors["color_sensor"]._sensor_object.render_camera
depth_camera = sim._sensors["depth_sensor"]._sensor_object.render_camera

# test unproject
# test unproject with known values
center_ray = render_camera.unproject(
mn.Vector2i(50, 50)
mn.Vector2i(250, 250), normalized=False
) # middle of the viewport
center_ray_normalized = render_camera.unproject(mn.Vector2i(250, 250))
assert np.allclose(
center_ray_normalized.direction,
center_ray.direction.normalized(),
atol=0.07,
)
assert np.allclose(center_ray.origin, np.array([0.5, 0, 0]), atol=0.07)
assert np.allclose(center_ray.direction, np.array([0, 0, -1.0]), atol=0.02)
assert np.allclose(
center_ray_normalized.direction, np.array([0, 0, -1.0]), atol=0.02
)

# NOTE: viewport y==0 is at the top
test_ray_2 = render_camera.unproject(
mn.Vector2i(100, 100)
mn.Vector2i(500, 500), normalized=False
) # bottom right of the viewport
test_ray_2_normalized = render_camera.unproject(mn.Vector2i(500, 500))
assert np.allclose(
test_ray_2_normalized.direction,
test_ray_2.direction.normalized(),
atol=0.07,
)
assert np.allclose(
test_ray_2.direction, np.array([0.569653, -0.581161, -0.581161]), atol=0.07
test_ray_2_normalized.direction,
np.array([0.569653, -0.581161, -0.581161]),
atol=0.07,
)

# add a primitive sphere object to the world
obj_template_mgr = sim.get_object_template_manager()
rigid_obj_mgr = sim.get_rigid_object_manager()
sphere_prim_handle = obj_template_mgr.get_template_handles("uvSphereSolid")[0]
sphere_template = obj_template_mgr.get_template_by_handle(sphere_prim_handle)
sphere_template.scale = [0.03, 0.03, 0.03]
obj_template_mgr.register_template(sphere_template, "scaled_sphere")
sphere_prim_handle = obj_template_mgr.get_template_handles("scaled_sphere")[0]
sphere_obj = rigid_obj_mgr.add_object_by_template_handle(sphere_prim_handle)

# validate that random unprojected points scaled by depth camera distance are actually on the render mesh
# do this by creating a small collision object at the unprojected point and test it against scene geometry
num_samples = 10
# move the camera, test a random pixel
cur_sample = 0
while cur_sample < num_samples:
# move agent
sim.agents[0].scene_node.translation = np.random.random(3)
# rotate agent
sim.agents[0].scene_node.rotation = mn.Quaternion.rotation(
mn.Rad(random.random() * mn.math.tau), mn.Vector3(0, 1, 0)
)
# tilt the camera
render_camera.node.rotation = mn.Quaternion.rotation(
mn.Rad(random.random()), mn.Vector3(1, 0, 0)
)
depth_camera.node.rotation = render_camera.node.rotation

# do the unprojection from depth image
view_point = mn.Vector2i(
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

ray = render_camera.unproject(view_point, normalized=False)
depth_obs: np.ndarray = sim.get_sensor_observations()["depth_sensor"]
# NOTE: (height, width) for buffer access
depth = depth_obs[view_point[1]][view_point[0]]

if depth == 0.0:
# skip depths of 0 which represent empty/background pixels
continue

# update the collision test object
depth_point = ray.origin + ray.direction * depth
sphere_obj.translation = depth_point

# optionally render the frames for debugging
# import habitat_sim.utils.viz_utils as vut
# c_image = vut.observation_to_image(sim.get_sensor_observations()["color_sensor"], "color")
# c_image.show()

assert (
sphere_obj.contact_test()
), "Must be intersecting with scene collision mesh."
cur_sample += 1


@pytest.mark.parametrize(
"sensor_type",
Expand Down