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

Visible time ranges are now specified per timeline, not per timeline type #6204

Merged
merged 13 commits into from
May 3, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented May 2, 2024

What

Previously, visible time ranges was internally saved for the two types of timelines, time timelines and sequence timelines. This was done as a minimum-viable product since time values from different timelines aren't transferable. Now that we build user facing data structures it became clear that this is both limiting and strange to use.

The python blueprint api is still not final, but happens to look a lot better now:

rrb.Blueprint(
    rrb.TimeSeriesView(
        name="Classification",
        origin="/classification",
        time_ranges=rrb.VisibleTimeRange(
            "frame_nr",
            rr.TimeRange(
                start=rr.TimeRangeBoundary(rr.TimeRangeBoundaryKind.RelativeToTimeCursor, -10),
                end=rr.TimeRangeBoundary(rr.TimeRangeBoundaryKind.RelativeToTimeCursor, 10),
            ),
        ),
    )
).connect("rerun_example_plot")

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!

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 labels May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Deployed docs

Commit Link
e21c508 https://landing-z6xqnwpjz-rerun.vercel.app/docs

Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Tracking per timeline is a huge improvement.

The API itself still seems very verbose to me. I think something that ended up looking like:

time_range=rrb.VisibleTimeRange(
  "frame_nr",
  start=rr.TimeRangeBoundary.CursorRelative(-10),
  end=rr.TimeRangeBoundary.CursorRelative(10)
)

would be quite a bit more usable. I would recommend ditching the intermediate TimeRange datatype between VisibleTimeRange and TimeRangeBoundary.

Comment on lines +61 to +62
/// Time range to use for this timeline.
range: rerun.datatypes.TimeRange (order: 200);
Copy link
Member

Choose a reason for hiding this comment

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

The type-nesting here doesn't seem to add much value/clarity to the user-facing API relative to inlining start/end directly. Just creates more typing and rightward drift.

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 do use this intermediate type internally though where its quite useful. It's easy to eliminate from Python via custom constructors but will ofc haunt us in C++ and Rust then

Copy link
Member Author

Choose a reason for hiding this comment

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

also to be fair it makes it easy to use the same time range on several timelines because then you can just reuse that range value with several strings

crates/re_log_types/src/absolute_time_range.rs Outdated Show resolved Hide resolved
@Wumpf
Copy link
Member Author

Wumpf commented May 3, 2024

decided to keep the TimeRange type - if it is constructable from tuples and arrays it's fine I think. The advantage of being able to pass it in for several timelines and the uses internally are too big to ignore imho

@Wumpf Wumpf force-pushed the andreas/range-per-timeline branch from 79a37e4 to ed65e29 Compare May 3, 2024 08:30
@emilk
Copy link
Member

emilk commented May 3, 2024

Could you please add a code-example to the PR description showing how you would specify time ranges for multiple timelines? 🙏

@Wumpf
Copy link
Member Author

Wumpf commented May 3, 2024

no point in it since I still want to significantly change what it looks like. I'm working on doing that and providing examples

@Wumpf Wumpf merged commit 45fecf1 into main May 3, 2024
40 checks passed
@Wumpf Wumpf deleted the andreas/range-per-timeline branch May 3, 2024 09:41
@Wumpf Wumpf mentioned this pull request May 17, 2024
5 tasks
emilk pushed a commit that referenced this pull request May 20, 2024
### What

The mechanism for identifying Rerun clients was implemented in #6204.
However, during the release I accidentally "fixed" the build of `cargo
build -p re_viewer --no-default-features` twice:
* The seemingly correct fix was to depend on `re_sdk_comms` with the
`server` feature always enabled.
* The definitely wrong thing that slipped through is to feature guard
the section that needs the `re_sdk_comms` server errors with
`#[cfg(feature = "server")]`. But `re_viewer` doesn't have a `server`
feature, disabling this effectively. This was reverted in
#6368

It's up to users of `re_viewer` to allow the serve feature, making
`re_viewer` agonistic of the presence of this error type in its smart
channel. Therefore I now moved out the error type to `re_sdk_comms`
lib.rs so that `re_viewer` doesn't have to depend on the
`re_sdk_comms`'s server feature.


### 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/6369?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/6369?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)!

- [PR Build Summary](https://build.rerun.io/pr/6369)
- [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
🟦 blueprint The data that defines our UI include in changelog 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants