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

Split ViewportBlueprint in its own crate #5421

Closed
abey79 opened this issue Mar 7, 2024 · 2 comments · Fixed by #6405
Closed

Split ViewportBlueprint in its own crate #5421

abey79 opened this issue Mar 7, 2024 · 2 comments · Fixed by #6405
Labels
🚜 refactor Change the code, not the functionality

Comments

@abey79
Copy link
Contributor

abey79 commented Mar 7, 2024

Attempting to add support context menu in the streams tree (re_time_panel) prompted a realisation that the data structure (and related operation) collectively implemented by {Viewport|Container|SpaceView}Blueprint are of much wider significance than just displaying the viewport and related tree.

For example, right-clicking on a streams' entity to add it to a new space view is a meaningful operation that implies a dependency of the streams view on the viewport blueprint.

After a quick glance, it appears that the following files/structs could be relatively painlessly be split of in a new re_viewport_blueprint (name TBD):

  • ViewportBlueprint (all of viewport_blueprint.rs)
  • ContainerBlueprint (all of container.rs)
  • all codegend objects (blueprint/*)
  • TreeAction

Edit: as discussed with @teh-cmc the codegen'ed stuff should go to a new re_blueprint_types crate that's lower in the hierarchy, so that re_query may access it.

Assuming the context menu code is further split off in yet another new crate, this would result in the following hierarchy (box may depend on lower box but not same-level box):

┌───────────────────────────────────────────────┐
│                   re_viewer                   │
└───────────────────────────────────────────────┘
┌──────────────────────┐ ┌──────────────────────┐
│     re_viewport      │ │    re_time_panel     │
└──────────────────────┘ └──────────────────────┘
             ┌──────────────────────┐            
             │   re_context_menu    │            
             └──────────────────────┘            
┌──────────────────────┐ ┌──────────────────────┐
│re_viewport_blueprint │ │      re_data_ui      │
└──────────────────────┘ └──────────────────────┘
             ┌──────────────────────┐            
             │    re_space_view     │            
             └──────────────────────┘            
┌───────────────────────────────────────────────┐
│              re_viewport_context              │
└───────────────────────────────────────────────┘

Stretch goal: I haven't deeply looked into that, but, given this refactor, it might be possible to further split off the blueprint tree UI in a separate crate. At the very least, viewport_blueprint_ui.rs should be renamed viewport_tree_ui.rs.

@abey79 abey79 added the 🚜 refactor Change the code, not the functionality label Mar 7, 2024
abey79 added a commit that referenced this issue Mar 8, 2024
### What

The streams tree now supports context menu. Currently, the only
available action is to create a space view with the selected entities.

Annoyingly, this PR introduces a dependency on `re_viewport` from
`re_time_panel`. However, a refactor is in order and will enable a scope
reduction for this new (otherwise inevitable) dependency:
- #5421

<img width="510" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/a5ec0e60-5a0e-47cb-9f19-94729d96b30e">


### 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/5422/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5422/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/5422/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
* [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/5422)
- [Docs
preview](https://rerun.io/preview/a62b1d874cda0237e5a9599627d34c19c889df99/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/a62b1d874cda0237e5a9599627d34c19c889df99/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@abey79 abey79 self-assigned this Mar 12, 2024
@abey79
Copy link
Contributor Author

abey79 commented Mar 12, 2024

This would help (or even unblock):

teh-cmc added a commit that referenced this issue Apr 8, 2024
Introduce very high-level APIs that make it possible to query, resolve,
deserialize and cache an entire archetype all at once.

The core trait allowing this is the newly introduced `ToArchetype<A>`
trait:
```rust
pub trait ToArchetype<A: re_types_core::Archetype> {
    fn to_archetype(&self, resolver: &crate::PromiseResolver) -> crate::PromiseResult<A>;
}
```

This trait is implemented for all builtins archetypes thanks to a new
codegen pass.

Implementing such a trait is tricky: one needs to know about this trait,
archetypes, queries, caches, etc all in one single place. This is a
recipe for a very nasty circular dependency chains.
This PR makes it work for all archetypes except archetypes that are
generated in `re_viewport`. These will need some special care in a
follow-up PR (likely moving them out of there, and only reexporting them
in `re_viewport`?).
Update: related:
- #5421

Here's how it looks in practice:
```rust
let caches = re_query_cache2::Caches::new(&store);

// First, get the results for this query.
//
// They might or might not already be cached. We won't know for sure until we try to access
// each individual component's data below.
let results: CachedLatestAtResults = caches.latest_at(
    &store,
    &query,
    &entity_path.into(),
    Points2D::all_components().iter().cloned(), // no generics!
);

// Then make use of the `ToArchetype` helper trait in order to query, resolve, deserialize and
// cache an entire archetype all at once.
use re_query_cache2::ToArchetype as _;

let arch: Points2D = match results.to_archetype(&resolver) {
    PromiseResult::Pending => {
        // Handle the fact that the data isn't ready appropriately.
        return Ok(());
    }
    PromiseResult::Ready(arch) => arch,
    PromiseResult::Error(err) => return Err(err.into()),
};

// With the data now fully resolved/converted and deserialized, some joining logic can be
// applied if desired.
//
// In most cases this will be either a clamped zip, or no joining at all.

let color_default_fn = || None;
let label_default_fn = || None;

let results = clamped_zip_1x2(
    arch.positions.iter(),
    arch.colors
        .iter()
        .flat_map(|colors| colors.iter().map(Some)),
    color_default_fn,
    arch.labels
        .iter()
        .flat_map(|labels| labels.iter().map(Some)),
    label_default_fn,
)
.collect_vec();

eprintln!("results:\n{results:?}");
```


---

Part of a PR series to completely revamp the data APIs in preparation
for the removal of instance keys and the introduction of promises:
- #5573
- #5574
- #5581
- #5605
- #5606
- #5633
- #5673
- #5679
- #5687
- #5755
- TODO
- TODO

Builds on top of the static data PR series:
- #5534
@teh-cmc teh-cmc self-assigned this Apr 8, 2024
teh-cmc added a commit that referenced this issue Apr 9, 2024
☝️ 

This is a prerequisite in order to implement `ToArchetype<A> for
QueryResults` for all blueprint related types without ending up in a
dependency cycle of doom.

---

- Part of #5421
- Followed by #5860
@teh-cmc teh-cmc removed their assignment Apr 11, 2024
@teh-cmc
Copy link
Member

teh-cmc commented Apr 11, 2024

The codegen'd types have been split into a dedicated crate (re_types_blueprint), re_viewport_blueprint still has to be done.

@emilk emilk added this to the Spring Cleaning milestone Apr 24, 2024
abey79 added a commit that referenced this issue May 23, 2024
…blueprint` crate (#6405)

### What

- Closes #5421
- Unlocks #6414 
- Unlocks  #6415

This enable more general access to manipulating the blueprint and unlock
further crate splitting.


![](https://static.rerun.io/crates/f0024a424aa35efab710ae88ba6a1ed741e7c248/1200w.png)

### TODO

- [x] delete the traits in `data_query.rs` and use concrete types
instead
- [x] move the UI function introduced by Andreas back to `re_space_view`

### 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/6405?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/6405?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/6405)
- [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
🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants