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

Open image and mesh files with drag-drop and File->Open #3116

Merged
merged 46 commits into from Aug 28, 2023
Merged

Conversation

emilk
Copy link
Member

@emilk emilk commented Aug 28, 2023

What

Supports mixing of:

  • .rrd over HTTP
  • File paths
  • WebSockets urls

in rerun command-line arguments. All will be streamed to the viewer in parallel.

File->Open and drag-and-dropping of files will use the same code path, and now supports meshes and images on native.

Saving for later PR:

  • Drag-and-drop of images and meshes on web
  • Loading images and meshes over HTTP

How

Created a new re_data_source crate with an enum DataSource plus all the functions to stream data from all these sources.

I also removed RemoteViewerApp which I originally created to have a custom GUI for selecting what server to connect to, but that GUI has been disabled since forever, so high time to remove it.

On web, care is taken to wake up the ui thread when loading data from web-sockets or HTTP.
On native, we just spin up another thread that notices new messages and wakes up the ui.

Checklist

@emilk emilk added 🕸️ web regarding running the viewer in a browser 📺 re_viewer affects re_viewer itself labels Aug 28, 2023
@github-actions
Copy link

Size changes

Name Previous Current Change
Wasm 13.71 MiB 14.40 MiB -4.79%

@emilk emilk marked this pull request as ready for review August 28, 2023 13:41
Copy link
Contributor

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

Great stuff! Lot's of the heavy lifting is beyond me, but I still got a few comments along the way (some may be me trying to understand more than anything else).

} else if uri.starts_with("ws://") || uri.starts_with("wss://") {
DataSource::WebSocketAddr(uri)

// Now we are into heuristics territory:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious as to what the use cases for that heuristic. Wouldn't it be cleaner to bail and produce a proper error? On CLI, it certainly feels so ("Argument XXX couldn't be resolved and was ignored"). Try to construct a Path from the input and checking for file existence feels like it should behave OK for CLI.

For drag and drop/File->Open, the OS should provide a well formed path by definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all for making the CLI life nicer.

rerun 127.0.0.1
rerun foo.jpg
rerun www.host.com/bar.rrd
…

it's nice not having to spell out the protocol for each of these

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess what I fail to understand here is the use of looks_like_a_file_path(). Isn't that handled by feeding whatever we have to Path first, and check for existence? The other heuristics do make sense to me.

crates/re_data_source/src/data_source.rs Outdated Show resolved Hide resolved
crates/re_data_source/src/data_source.rs Outdated Show resolved Hide resolved
crates/re_smart_channel/src/receive_set.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/app.rs Show resolved Hide resolved
crates/re_viewer/src/app.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/app_state.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

That welcome screen workflow is not done haunting us :)

crates/re_viewer/src/app.rs Outdated Show resolved Hide resolved
SmartChannelSource::File(_) | SmartChannelSource::RrdHttpStream { .. } => {
return true;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to cut it either, I should have been clearer earlier—sorry for that. This also breaks the demo workflow: the loading screen is shown initially, but if you then close the recording, we're back to the loading screen—but that time it should be the welcome screen. This is way I originally used is_connected().

I'm not quite sure how to proceed here. That "is_connected" trick might be valid only if this is the only stream. The hole we dug with that whole "when to show welcome screen" thing is only getting deeper now 😓

Maybe that's something we can revisit later (maybe when my brain fog dissipate), in this case an issue with 0.9 milestone will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh

I think #3125 covers it well

Co-authored-by: Antoine Beyeler <49431240+abey79@users.noreply.github.com>
@emilk emilk merged commit 315f87e into main Aug 28, 2023
20 checks passed
@emilk emilk deleted the emilk/channel-set branch August 28, 2023 17:11
emilk added a commit that referenced this pull request Aug 30, 2023
@emilk emilk mentioned this pull request Aug 30, 2023
3 tasks
emilk added a commit that referenced this pull request Aug 31, 2023
Broke in #3116

Tried to use tokio from a rayon thread, which doesn't work
out-of-the-box. But the rayon use is unnecessary - each call to
`data_source.stream` is non-blocking anyways.

### 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 [demo.rerun.io](https://demo.rerun.io/pr/3160) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3160)
- [Docs
preview](https://rerun.io/preview/11d5fea9d4982d50c808bc571299a9ded4d37177/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/11d5fea9d4982d50c808bc571299a9ded4d37177/examples)
<!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📺 re_viewer affects re_viewer itself 🕸️ web regarding running the viewer in a browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants