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

New DepthImage archetype #6915

Merged
merged 50 commits into from
Jul 17, 2024
Merged

New DepthImage archetype #6915

merged 50 commits into from
Jul 17, 2024

Conversation

emilk
Copy link
Member

@emilk emilk commented Jul 16, 2024

What

This changes DepthImage from a tensor to a byte-blob + resolution + data-type.
It adds new code for hovering images and uploading images to GPU, both general enough to also work for color images and segmentation images. This means those PRs will be a lot smaller.

Since we still need to support color and segmentation images encoded as tensors, the code is quite complex, with duplicated paths here and there. It will all get cleaned up in later PRs.

I'm reusing a lot of stuff from tensors, like TensorMeaning, TensorStats TensorElement, but these will be renamed and improved before #6386 is done.

TODO

  • Update depth_image_ext.cpp
  • Update depth_image_ext.py
  • Fix hovering depth images in 3D view

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide
  • Test a few local examples

To run all checks from main, comment on the PR with @rerun-bot full-check.

@emilk emilk added 📺 re_viewer affects re_viewer itself include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Jul 16, 2024
Copy link

github-actions bot commented Jul 16, 2024

Deployed docs

Commit Link
562ea11 https://landing-2bjuuqyh5-rerun.vercel.app/docs

@emilk emilk force-pushed the emilk/new-depth-image branch from 22263df to 35cf95c Compare July 16, 2024 21:28
@rerun-io rerun-io deleted a comment from github-actions bot Jul 17, 2024
@emilk
Copy link
Member Author

emilk commented Jul 17, 2024

@rerun-bot full-check

Copy link

@emilk emilk marked this pull request as ready for review July 17, 2024 10:15
@Wumpf Wumpf self-requested a review July 17, 2024 10:42
/// The width and height of a 2D image.
struct Resolution2D (
"attr.python.aliases": "npt.NDArray[np.int], Sequence[int], Tuple[int, int]",
"attr.python.array_aliases": "npt.NDArray[np.int], Sequence[int]",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include Sequence[aliases], but maybe codegen does this?

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much stuff :O

///
/// Return `None` if out-of-bounds.
#[inline]
pub fn get_xyc(&self, x: u32, y: u32, channel: u32) -> Option<TensorElement> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming is a bit strange, it's not xyc we're getting. Why not get_element?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to be explicit with the parameter order, since for tensors it is usually yxc

@emilk
Copy link
Member Author

emilk commented Jul 17, 2024

@rerun-bot full-check

Copy link

@emilk emilk merged commit 9fdbd69 into main Jul 17, 2024
72 of 77 checks passed
@emilk emilk deleted the emilk/new-depth-image branch July 17, 2024 14:01
emilk added a commit that referenced this pull request Jul 18, 2024
### What
* Part of #6386
* Part of #6844
* Very similar to #6915

### 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/6928?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/6928?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/6928)
- [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
include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants