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

Pinhole resolution defaults to 1:1 #3852

Closed
nikolausWest opened this issue Oct 13, 2023 · 1 comment
Closed

Pinhole resolution defaults to 1:1 #3852

nikolausWest opened this issue Oct 13, 2023 · 1 comment
Assignees
Labels
🪳 bug Something isn't working 🍏 primitives Relating to Rerun primitives 🏎️ Quick Issue Can be fixed in a few hours or less 📺 re_viewer affects re_viewer itself
Milestone

Comments

@nikolausWest
Copy link
Member

The Pinhole archetype has resolution as optional, but defaults to the completely unusable value of 1:1. Given that it is optional most users would probably assume that resolution would be picked up from the image if they log that as well.

Update: while creating this issue I also realized that setting the pinhole incorrectly affects the 2D image view so that it only shows the image inside the pinhole resolution. It's a little unclear to me at time of writing whether that is a good behavior or not, but I think it turns this issue from an annoyance to a bug.

Repro: (remove width and height from pinhole docs example)

import numpy as np
import rerun as rr

rr.init("rerun_example_pinhole", spawn=True)
rng = np.random.default_rng(12345)

image = rng.uniform(0, 255, size=[3, 3, 3])
rr.log("world/image", rr.Pinhole(focal_length=3))
rr.log("world/image", rr.Image(image))

image

@nikolausWest nikolausWest added 🪳 bug Something isn't working 📺 re_viewer affects re_viewer itself 🍏 primitives Relating to Rerun primitives labels Oct 13, 2023
@emilk
Copy link
Member

emilk commented Oct 13, 2023

We should have no default resolution unless the user sets one

@emilk emilk added the 🏎️ Quick Issue Can be fixed in a few hours or less label Oct 13, 2023
@jleibs jleibs self-assigned this Oct 18, 2023
jleibs added a commit that referenced this issue Oct 20, 2023
### 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/)
@jleibs jleibs closed this as completed Oct 20, 2023
Wumpf pushed a commit that referenced this issue Aug 14, 2024
### What
* We regressed #3852 during
Tensor -> Image migration

### 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 examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7187?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7187?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
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/7187)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🍏 primitives Relating to Rerun primitives 🏎️ Quick Issue Can be fixed in a few hours or less 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

No branches or pull requests

3 participants