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

Bounding box not taking transformation hierarchy for rr.Pinhole correctly into account #4452

Closed
roym899 opened this issue Dec 7, 2023 · 1 comment · Fixed by #4640
Closed
Assignees
Labels
🪳 bug Something isn't working 📺 re_viewer affects re_viewer itself
Milestone

Comments

@roym899
Copy link
Collaborator

roym899 commented Dec 7, 2023

Describe the bug
It seems like the current 3D bounding box is not properly taking a pinhole's transform hierarchy into account.

If the camera is at the origin of its parent frame, the global origin will always be part of the bounding box. However, the transformed origin is also taken into account.

When there's both a parent transform and a child transform the bounding box calculation seems completely incorrect.

To Reproduce

import numpy as np
import rerun as rr

rr.init("bounding_box", spawn=True)
rr.log("parent", rr.Transform3D(translation=[5.0, 5.0, 5.0]))
rr.log("parent/camera", rr.Pinhole(width=640, height=480, focal_length=320))
# rr.log("parent/camera", rr.Transform3D(translation=[1.0,1.0,1.0]))  # this will affect the bounding box, but not in the correct way
# rr.log("parent/points", rr.Points3D(np.random.rand(10, 3)))  # uncomment to avoid collapsing bounding box (not needed with the current behavior)

Screenshots
Camera at global origin, no transforms at all -> collapsing bounding box (I assume this is the correct behavior)
1

Camera at local origin (bounding box seems to align with frustum origin + world origin)
2

Camera additionally locally transformed (bounding box changes but completely incorrectly (now the frustum is completely outside)
3

Expected Behavior
Not entirely sure what the consensus is about frustum being part or not part of the bounding box. But at least the origin should properly be taken into account. I guess in that case, for all the above cases the bounding box should collapse. When also logging the points in the above example, the bounding box should include the pinhole origin and the points.

Desktop:

  • OS: Ubuntu 20.04

Rerun version

rerun_py 0.12.0-alpha.1+dev [rustc 1.74.0 (79e9716c9 2023-11-13), LLVM 17.0.4] x86_64-unknown-linux-gnu
@roym899 roym899 added 🪳 bug Something isn't working 👀 needs triage This issue needs to be triaged by the Rerun team 📺 re_viewer affects re_viewer itself labels Dec 7, 2023
@Wumpf Wumpf removed the 👀 needs triage This issue needs to be triaged by the Rerun team label Dec 8, 2023
@Wumpf Wumpf added this to the 0.12 milestone Dec 8, 2023
@emilk
Copy link
Member

emilk commented Jan 2, 2024

Let's test if this is a new or existing bug (if new we should fix it before 0.12)

@jleibs jleibs self-assigned this Jan 2, 2024
jleibs added a commit that referenced this issue Jan 3, 2024
### 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants