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

Tensor & depth image value ranges can now be configured, from UI & code #7549

Merged
merged 22 commits into from
Oct 1, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Sep 30, 2024

What

Depth images (and their clouds):

depth.image.range.editing.mov

Tensors:

tensor_data_range.mov

Noteworth ripple effects & considerations in this PR:

  • turns out depth image constructor on Python missed a lot of parameters, fixed that
  • went with a new ValueRange component rather than reusing Range1D because the semantics (and names in the ui) of the later were just too unclear
    • we need a tagged component mechanism really badly!
    • decided against putting this on the tensor view as some notes indicated was the plan
  • ended up refactoring a few interesting things around image/tensor handling:
    • colormap is no longer part of ImageInfo, we already didn't use it for hashing it. Having now a range made it even more clear that this doesn't belong there
    • finite_range on tensor & image is now always present, using the datatypes' finite range as fallback. The None case handling was awkward at best and non-existant (error out) usually

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

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

@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself 🟦 blueprint The data that defines our UI include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Sep 30, 2024
Copy link

github-actions bot commented Sep 30, 2024

Deployed docs

Commit Link
90eb9fd https://landing-do0mnbv92-rerun.vercel.app/docs

@emilk emilk self-requested a review October 1, 2024 09:00
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Very good!

My only concern is that we don't end up with a bad estimate and then stick with it. In particular, what is the estimated range of an image of a new space view, created when the time cursor is before the image was logged? Maybe it doesn't run the fallback/estimator in that case, and all is good?

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Very good!

My only concern is that we don't end up with a bad estimate and then stick with it. In particular, what is the estimated range of an image of a new space view, created when the time cursor is before the image was logged? Maybe it doesn't run the fallback/estimator in that case, and all is good?

@Wumpf
Copy link
Member Author

Wumpf commented Oct 1, 2024

My only concern is that we don't end up with a bad estimate and then stick with it. In particular, what is the estimated range of an image of a new space view, created when the time cursor is before the image was logged? Maybe it doesn't run the fallback/estimator in that case, and all is good?

The behavior shouldn't have changed to before: If no value is present, we evaluate the fallback and fallbacks are not sticky - they get evaluated every frame. This means that we'll always grab the image stats that are available for that frame and those (other than my change regarding how finite_range is defined) are unchanged to before. Which should give us the same behavior.
Notably, this also means that until someone either logs or overrides an image range, they will still get flickering behavior if their depth image or tensor range changes over time.

not 100% sure that answers the question - does it? :)

@emilk
Copy link
Member

emilk commented Oct 1, 2024

Oh, perfect answer - thank you!

@Wumpf Wumpf merged commit c4eb805 into main Oct 1, 2024
39 of 40 checks passed
@Wumpf Wumpf deleted the andreas/tensor-data-range branch October 1, 2024 13:09
@emilk emilk changed the title Tensor & depth image value ranges can now be configured (from ui & code) Tensor & depth image value ranges can now be configured, from UI & code Oct 16, 2024
@rasmusgo rasmusgo mentioned this pull request Oct 18, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI 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.

Adjustable depth image range Allow to specify a tensor's data range via component
2 participants