Skip to content

Commit

Permalink
Fix incorrect bounding box calculation for camera view parts (#4640)
Browse files Browse the repository at this point in the history
### What
- Resolves: #4452

The wrong transform was being used for the bounding box calculation.
Clarify the transform naming / construction.
Also include the origin when the origin axes are turned on.


![image](https://github.com/rerun-io/rerun/assets/3312232/c0076200-927c-404a-894a-34d9750d26bd)


### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4640/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4640/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4640/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4640)
- [Docs
preview](https://rerun.io/preview/0faa6bf2ec93fdf2b93c71e6695a177fb0c6aa5f/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/0faa6bf2ec93fdf2b93c71e6695a177fb0c6aa5f/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
jleibs committed Jan 3, 2024
1 parent 486c0bc commit 2da0c09
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 24 deletions.
53 changes: 29 additions & 24 deletions crates/re_space_view_spatial/src/parts/cameras.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,6 @@ impl CamerasPart {
) {
let instance_key = InstanceKey(0);

// The transform *at* this entity path already has the pinhole transformation we got passed in!
// This makes sense, since if there's an image logged here one would expect that the transform applies.
// We're however first interested in the rigid transform that led here, so query the parent transform.
let parent_path = ent_path
.parent()
.expect("root path can't be part of scene query");
let Some(mut world_from_camera) = transforms.reference_from_entity(&parent_path) else {
return;
};

let frustum_length = *props.pinhole_image_plane_distance;

// If the camera is our reference, there is nothing for us to display.
Expand All @@ -82,27 +72,42 @@ impl CamerasPart {
return;
}

// There's one wrinkle with using the parent transform though:
// The entity itself may have a 3D transform which (by convention!) we apply *before* the pinhole camera.
// Let's add that if it exists.
if let Some(transform_at_entity) = transform_at_entity.clone() {
world_from_camera =
world_from_camera * transform_at_entity.into_parent_from_child_transform();
}
// We need special handling to find the 3D transform for drawing the
// frustum itself. The transform that would otherwise be in the
// transform context might include both a rigid transform and a pinhole. This
// makes sense, since if there's an image logged here one would expect
// both the rigid and the pinhole to apply, but here we're only interested
// in the rigid transform at this entity path, excluding the pinhole
// portion (we handle the pinhole separately later).
let world_from_camera_rigid = {
// Start with the transform to the entity parent, if it exists
let world_from_parent = ent_path
.parent()
.and_then(|parent_path| transforms.reference_from_entity(&parent_path))
.unwrap_or(macaw::Affine3A::IDENTITY);

// Then combine it with the transform at the entity itself, if there is one.
if let Some(transform_at_entity) = transform_at_entity {
world_from_parent * transform_at_entity.into_parent_from_child_transform()
} else {
world_from_parent
}
};

// If this transform is not representable as an `IsoTransform` we can't display it yet.
// This would happen if the camera is under another camera or under a transform with non-uniform scale.
let Some(world_from_camera_iso) = macaw::IsoTransform::from_mat4(&world_from_camera.into())
let Some(world_from_camera_rigid_iso) =
macaw::IsoTransform::from_mat4(&world_from_camera_rigid.into())
else {
return;
};

debug_assert!(world_from_camera_iso.is_finite());
debug_assert!(world_from_camera_rigid_iso.is_finite());

self.space_cameras.push(SpaceCamera3D {
ent_path: ent_path.clone(),
pinhole_view_coordinates,
world_from_camera: world_from_camera_iso,
world_from_camera: world_from_camera_rigid_iso,
pinhole: Some(pinhole.clone()),
picture_plane_distance: frustum_length,
});
Expand Down Expand Up @@ -157,7 +162,8 @@ impl CamerasPart {
// The frustum is setup as a RDF frustum, but if the view coordinates are not RDF,
// we need to reorient the displayed frustum so that we indicate the correct orientation in the 3D world space.
.world_from_obj(
world_from_camera * glam::Affine3A::from_mat3(pinhole_view_coordinates.from_rdf()),
world_from_camera_rigid
* glam::Affine3A::from_mat3(pinhole_view_coordinates.from_rdf()),
)
.outline_mask_ids(entity_highlight.overall)
.picking_object_id(instance_layer_id.object);
Expand All @@ -172,11 +178,10 @@ impl CamerasPart {
lines.outline_mask_ids(*outline_mask_ids);
}

// world_from_camera is the transform to the pinhole origin
self.data.extend_bounding_box_with_points(
std::iter::once(glam::Vec3::ZERO),
transform_at_entity
.unwrap_or(Transform3D::IDENTITY)
.into_parent_from_child_transform(),
world_from_camera_rigid,
);
}
}
Expand Down
3 changes: 3 additions & 0 deletions crates/re_space_view_spatial/src/ui_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,9 @@ pub fn view_3d(
axis_length,
re_renderer::OutlineMaskPreference::NONE,
);

// If we are showing the axes for the space, then add the space origin to the bounding box.
state.scene_bbox.extend(glam::Vec3::ZERO);
}

// Determine view port resolution and position.
Expand Down

0 comments on commit 2da0c09

Please sign in to comment.