Skip to content

Commit

Permalink
Warn if no resolution provided to Pinhole (#3923)
Browse files Browse the repository at this point in the history
### What
Resolves:
 - #3852

Removes the setting of a default resolution if not provided and
generates warnings/exceptions as appropriate.

Also uses the TensorData to derived a resolution if none was provided
when rendering the pinhole frustum.

### 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 [demo.rerun.io](https://demo.rerun.io/pr/3923) (if
applicable)
* [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/3923)
- [Docs
preview](https://rerun.io/preview/48f02eadacd8ac83ad9e5b19dbb8ad44ef840144/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/48f02eadacd8ac83ad9e5b19dbb8ad44ef840144/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
  • Loading branch information
jleibs authored Oct 20, 2023
1 parent 06dd483 commit 851e059
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 16 deletions.
18 changes: 17 additions & 1 deletion crates/re_space_view_spatial/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod ui;
mod ui_2d;
mod ui_3d;

use re_types::components::{Resolution, TensorData};
pub use space_view_2d::SpatialSpaceView2D;
pub use space_view_3d::SpatialSpaceView3D;

Expand All @@ -33,6 +34,20 @@ mod view_kind {
}
}

fn resolution_from_tensor(
store: &re_arrow_store::DataStore,
query: &re_arrow_store::LatestAtQuery,
entity_path: &re_log_types::EntityPath,
) -> Option<Resolution> {
store
.query_latest_component::<TensorData>(entity_path, query)
.and_then(|tensor| {
tensor
.image_height_width_channels()
.map(|hwc| Resolution([hwc[1] as f32, hwc[0] as f32].into()))
})
}

/// Utility for querying a pinhole archetype instance.
///
/// TODO(andreas): It should be possible to convert [`re_query::ArchetypeView`] to its corresponding Archetype for situations like this.
Expand All @@ -48,7 +63,8 @@ fn query_pinhole(
image_from_camera: image_from_camera.value,
resolution: store
.query_latest_component(entity_path, query)
.map(|c| c.value),
.map(|c| c.value)
.or_else(|| resolution_from_tensor(store, query, entity_path)),
camera_xyz: store
.query_latest_component(entity_path, query)
.map(|c| c.value),
Expand Down
35 changes: 20 additions & 15 deletions rerun_py/rerun_sdk/rerun/archetypes/pinhole_ext.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,27 @@ def __init__(

# TODO(andreas): Use a union type for the Pinhole component instead ~Zof converting to a matrix here
if image_from_camera is None:
# Resolution is needed for various fallbacks/error cases below.
if resolution is None:
resolution = [1.0, 1.0]
resolution = Vec2D(resolution)
width = cast(float, resolution.xy[0])
height = cast(float, resolution.xy[1])
if resolution is not None:
res_vec = Vec2D(resolution)
width = cast(float, res_vec.xy[0])
height = cast(float, res_vec.xy[1])
else:
width = None
height = None

if focal_length is None:
_send_warning_or_raise("either image_from_camera or focal_length must be set", 1)
focal_length = (width * height) ** 0.5 # a reasonable default
if height is None or width is None:
raise ValueError("Either image_from_camera or focal_length must be set")
else:
_send_warning_or_raise("Either image_from_camera or focal_length must be set", 1)
focal_length = (width * height) ** 0.5 # a reasonable best-effort default

if principal_point is None:
principal_point = [width / 2, height / 2]
if height is not None and width is not None:
principal_point = [width / 2, height / 2]
else:
raise ValueError("Must provide one of principal_point, resolution, or width/height")

if type(focal_length) in (int, float):
fl_x = focal_length
fl_y = focal_length
Expand All @@ -109,17 +118,13 @@ def __init__(
fl_x = focal_length[0] # type: ignore[index]
fl_y = focal_length[1] # type: ignore[index]
except Exception:
_send_warning_or_raise("Expected focal_length to be one or two floats", 1)
fl_x = width / 2
fl_y = fl_x
raise ValueError("Expected focal_length to be one or two floats")

try:
u_cen = principal_point[0] # type: ignore[index]
v_cen = principal_point[1] # type: ignore[index]
except Exception:
_send_warning_or_raise("Expected principal_point to be one or two floats", 1)
u_cen = width / 2
v_cen = height / 2
raise ValueError("Expected principal_point to be one or two floats")

image_from_camera = [[fl_x, 0, u_cen], [0, fl_y, v_cen], [0, 0, 1]] # type: ignore[assignment]
else:
Expand Down
4 changes: 4 additions & 0 deletions rerun_py/tests/unit/test_expected_warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ def test_expected_warnings() -> None:
rr.log("test_transform", rr.TranslationAndMat3x3(translation=[1, 0, 0])), # type: ignore[arg-type]
"Expected an object implementing rerun.AsComponents or an iterable of rerun.ComponentBatchLike, but got",
),
(
rr.log("world/image", rr.Pinhole(focal_length=3)),
"Must provide one of principal_point, resolution, or width/height)",
),
]

assert len(warnings) == len(expected_warnings)
Expand Down

0 comments on commit 851e059

Please sign in to comment.