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

Update the dataframe view to use re_dataframe and egui_table #7380

Merged
merged 53 commits into from
Sep 10, 2024

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Sep 9, 2024

What

Major update to the dataframe view

TODO:

image

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.

crates/viewer/re_viewer/src/web.rs Outdated Show resolved Hide resolved
}
}

fn get(&self, start: u64, num_rows: u64) -> Vec<RecordBatch> {
Copy link
Member

Choose a reason for hiding this comment

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

I think using rows: Range<u64> here (and elsewhere) instead would be more idiomatic and less error prone

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I'll make a separate, follow-up PR for visibility, as it may affect @jleibs

crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs Outdated Show resolved Hide resolved
Comment on lines +170 to +171
row_id_times,
row_id_counters,
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: but "times" and "counters" only makes sense for RowId as TUID. If we switch to UUID these names becomes nonsense. We also don't treat them as times and counters anywhere in this file, so perhaps it would be better to name time row_id_hi and row_id_lo or something? 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Also, why are we storing RowId as two arrays of u64 instead of a single array of [u8; 16] ? 🤔

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'm basically following what is being done in re_datastore. I expect this to be carved in stone with the sorbet spec, so let's adjust the implementation when we have that (fundamentally, the purpose of DisplayRecordBatch and friends is to decode a sorbet-compliant RecordBatch for the purpose of ui).

Copy link
Member Author

Choose a reason for hiding this comment

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

@abey79 abey79 merged commit e9b02e3 into main Sep 10, 2024
35 checks passed
@abey79 abey79 deleted the antoine/use-re-dataframe branch September 10, 2024 12:29
jprochazk pushed a commit that referenced this pull request Sep 11, 2024
### What

- Closes #7279

Major update to the dataframe view
- display the data return by the new `re_dataframe` crate
  - the PoV entity/component is now actually used
  - entities are now always columns
    - see #7379 
- use [`egui_table`](https://github.com/rerun-io/egui_table) for the
table
  - hierarchical header
  - sticky columns
  - and much more...

TODO:
- [x] fix after merging #7383

<img width="2670" alt="image"
src="https://github.com/user-attachments/assets/cf09b69b-3c82-4ba9-9425-bf60622efae4">


### 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/7380?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/7380?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/7380)
- [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`.

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>
@emilk emilk added exclude from changelog PRs with this won't show up in CHANGELOG.md and removed include in changelog labels Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md feat-dataframe-view Everything related to the dataframe view ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataframe view: add PoV to range query
3 participants