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.

Sign up for GitHub

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

Jump to bottom

Web: Support multiple .rrd URLs #4740

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Web: Support multiple .rrd URLs #4740

merged 7 commits into from
Jan 8, 2024

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Jan 8, 2024 •
edited by github-actions bot
Loading

What

Fixes #4630

New API can be seen here: https://github.com/rerun-io/web-viewer-react-example/blob/main/src/App.tsx

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

@jprochazk jprochazk added 🕸️ web regarding running the viewer in a browser include in changelog labels Jan 8, 2024
jprochazk
jprochazk commented Jan 8, 2024
View reviewed changes
crates/re_viewer/src/store_hub.rs
self.store_bundle.entity_dbs.retain(|_, db| {
let Some(data_source) = &db.data_source else {
// no data source, keep
return true;
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this make sense? I'm not sure how it's possible to have a database without a data source, and if it should be handled differently here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- I'd love for us to refactor things to make this not possible. Mostly this shows up in unit-tests with programmatically created stores.

Any data being streamed into the app should have it's store set based on the channel-source though:
See: https://github.com/rerun-io/rerun/blob/main/crates/re_viewer/src/app.rs#L819-L821

jprochazk
jprochazk commented Jan 8, 2024
View reviewed changes
crates/re_log/src/setup.rs
if LOG_INIT.load(Ordering::SeqCst) {
return;
}
LOG_INIT.store(true, Ordering::SeqCst);
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling new WebViewer twice would cause a panic here, because the logger would be set by the previous call. Not sure if this is the best way to fix this, as a panic during initialization will cause the logger to not work anymore.

@jprochazk jprochazk marked this pull request as ready for review January 8, 2024 17:23
jleibs
jleibs approved these changes Jan 8, 2024
View reviewed changes
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.

This looks pretty reasonable -- definitely need to make sure it's well tested though.

crates/re_log_encoding/src/stream_rrd_from_http.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/store_hub.rs
self.store_bundle.entity_dbs.retain(|_, db| {
let Some(data_source) = &db.data_source else {
// no data source, keep
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- I'd love for us to refactor things to make this not possible. Mostly this shows up in unit-tests with programmatically created stores.

Any data being streamed into the app should have it's store set based on the channel-source though:
See: https://github.com/rerun-io/rerun/blob/main/crates/re_viewer/src/app.rs#L819-L821

rerun_js/web-viewer/index.js Outdated Show resolved Hide resolved
rerun_js/web-viewer/index.js Outdated Show resolved Hide resolved
crates/re_viewer/src/web.rs
Comment on lines +100 to +102
if let Some(store_hub) = app.store_hub.as_mut() {
store_hub.remove_recording_by_uri(url);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there any situation where we want to remove the receiver, but keep the recording that has been received so far? I'm imagining a case where a user is connected to a live stream and they want to stop getting updates but still inspect the data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. The APIs exposed here were made just to faciliate the React-style magic. I guess we could expose a separate disconnect API that just closes the stream, and preserves the recording.

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'll leave it out of here for now, I don't know what that would look like on the React side

@jprochazk jprochazk merged commit d204936 into main Jan 8, 2024
40 checks passed
@jprochazk jprochazk deleted the jan/multi-rrd-js branch January 8, 2024 21:23
jprochazk added a commit that referenced this pull request Jan 9, 2024
@jprochazk
Web: Support multiple .rrd URLs (#4740)
27f9eb8 
### What

Fixes #4630

New API can be seen here:
https://github.com/rerun-io/web-viewer-react-example/blob/main/src/App.tsx

### 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 newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4740/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4740/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4740/index.html?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

- [PR Build Summary](https://build.rerun.io/pr/4740)
- [Docs
preview](https://rerun.io/preview/165842ec644787f1b959e93fd14071c3ac7b6986/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/165842ec644787f1b959e93fd14071c3ac7b6986/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleibs jleibs jleibs approved these changes

Assignees
No one assigned
Labels
include in changelog 🕸️ web regarding running the viewer in a browser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support opening multiple rrd files programmatically with rerun_js
2 participants
@jprochazk @jleibs