Skip to content

Commit

Permalink
Use new egui_tiles active_tiles api to simplify space view multithr…
Browse files Browse the repository at this point in the history
…eading (#4864)

### What

Removes a workaround where we had track the tiles that were visible last
frame. Resolved now due to

* rerun-io/egui_tiles#42


### 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/4864/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4864/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/4864/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/4864)
- [Docs
preview](https://rerun.io/preview/56cae2d11d8d4c5bd8df2a07e2814992c2d200f7/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/56cae2d11d8d4c5bd8df2a07e2814992c2d200f7/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
Wumpf committed Jan 18, 2024
1 parent 083a851 commit ebc49a7
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 40 deletions.
23 changes: 15 additions & 8 deletions crates/re_viewport/src/system_execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ pub fn create_and_run_space_view_systems(

pub fn execute_systems_for_space_views<'a>(
ctx: &'a ViewerContext<'a>,
mut space_views_to_execute: Vec<SpaceViewId>,
tree: &egui_tiles::Tree<SpaceViewId>,
space_views: &'a BTreeMap<SpaceViewId, SpaceViewBlueprint>,
) -> HashMap<SpaceViewId, (ViewQuery<'a>, SystemExecutionOutput)> {
let Some(time_int) = ctx.rec_cfg.time_ctrl.read().time_int() else {
Expand All @@ -69,13 +69,20 @@ pub fn execute_systems_for_space_views<'a>(

re_tracing::profile_wait!("execute_systems");

space_views_to_execute
.par_drain(..)
.filter_map(|space_view_id| {
let space_view_blueprint = space_views.get(&space_view_id)?;
let highlights = highlights_for_space_view(ctx, space_view_id);
let output = space_view_blueprint.execute_systems(ctx, time_int, highlights);
Some((space_view_id, output))
tree.active_tiles()
.into_par_iter()
.filter_map(|tile_id| {
tree.tiles.get(tile_id).and_then(|tile| match tile {
egui_tiles::Tile::Pane(space_view_id) => {
space_views.get(space_view_id).map(|space_view_blueprint| {
let highlights = highlights_for_space_view(ctx, *space_view_id);
let output =
space_view_blueprint.execute_systems(ctx, time_int, highlights);
(*space_view_id, output)
})
}
egui_tiles::Tile::Container(_) => None,
})
})
.collect::<HashMap<_, _>>()
}
46 changes: 14 additions & 32 deletions crates/re_viewport/src/viewport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use re_viewer_context::{
use crate::{
space_view_entity_picker::SpaceViewEntityPicker,
space_view_heuristics::default_created_space_views,
space_view_highlights::highlights_for_space_view,
system_execution::execute_systems_for_space_views, SpaceInfoCollection, SpaceViewBlueprint,
ViewportBlueprint,
};
Expand All @@ -39,11 +38,6 @@ pub struct PerSpaceViewState {
pub struct ViewportState {
space_view_entity_window: SpaceViewEntityPicker,
space_view_states: HashMap<SpaceViewId, PerSpaceViewState>,

/// List of all space views that were visible *on screen* (excluding e.g. unselected tabs) the last frame.
///
/// TODO(rerun-io/egui_tiles#34): This is needed because we don't know which space views will be visible until we have drawn them.
space_views_displayed_last_frame: Vec<SpaceViewId>,
}

static DEFAULT_PROPS: Lazy<EntityPropertyMap> = Lazy::<EntityPropertyMap>::new(Default::default);
Expand Down Expand Up @@ -213,11 +207,8 @@ impl<'a, 'b> Viewport<'a, 'b> {
&mut self.tree
};

let executed_systems_per_space_view = execute_systems_for_space_views(
ctx,
std::mem::take(&mut state.space_views_displayed_last_frame),
&blueprint.space_views,
);
let executed_systems_per_space_view =
execute_systems_for_space_views(ctx, tree, &blueprint.space_views);

ui.scope(|ui| {
ui.spacing_mut().item_spacing.x = re_ui::ReUi::view_padding();
Expand All @@ -230,7 +221,6 @@ impl<'a, 'b> Viewport<'a, 'b> {
space_views: &blueprint.space_views,
maximized: &mut maximized,
edited: false,
space_views_displayed_current_frame: Vec::new(),
executed_systems_per_space_view,
};

Expand All @@ -251,8 +241,6 @@ impl<'a, 'b> Viewport<'a, 'b> {

// TODO(#4687): Be extra careful here. If we mark edited inappropriately we can create an infinite edit loop.
self.edited |= tab_viewer.edited;

state.space_views_displayed_last_frame = tab_viewer.space_views_displayed_current_frame;
});

self.blueprint.set_maximized(maximized, ctx);
Expand Down Expand Up @@ -472,11 +460,6 @@ struct TabViewer<'a, 'b> {
/// List of query & system execution results for each space view.
executed_systems_per_space_view: HashMap<SpaceViewId, (ViewQuery<'a>, SystemExecutionOutput)>,

/// List of all space views drawn this frame.
///
/// TODO(rerun-io/egui_tiles#34): It should be possible to predict which space views will be drawn.
space_views_displayed_current_frame: Vec<SpaceViewId>,

/// The user edited the tree.
edited: bool,
}
Expand All @@ -499,22 +482,24 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
return Default::default();
}

let Some(latest_at) = self.ctx.rec_cfg.time_ctrl.read().time_int() else {
if self.ctx.rec_cfg.time_ctrl.read().time_int().is_none() {
ui.centered_and_justified(|ui| {
ui.weak("No time selected");
});
return Default::default();
};

// TODO(rerun-io/egui_tiles#34): If we haven't executed the system yet ahead of time, we should do so now.
// This is needed because we merely "guess" which systems we are going to need.
let (query, system_output) =
if let Some(result) = self.executed_systems_per_space_view.remove(space_view_id) {
result
} else {
let highlights = highlights_for_space_view(self.ctx, *space_view_id);
space_view_blueprint.execute_systems(self.ctx, latest_at, highlights)
};
let Some((query, system_output)) =
self.executed_systems_per_space_view.remove(space_view_id)
else {
// The space view's systems haven't been executed.
// This should never happen, but if it does anyways we can't display the space view.
re_log::error_once!(
"Visualizers for space view {:?} haven't been executed prior to display. This should never happen, please report a bug.",
space_view_blueprint.display_name_or_default()
); // TODO(#4433): This should go to analytics
return Default::default();
};

let PerSpaceViewState {
auto_properties: _,
Expand All @@ -525,9 +510,6 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
space_view_blueprint.class_identifier(),
);

self.space_views_displayed_current_frame
.push(space_view_blueprint.id);

space_view_blueprint.scene_ui(
space_view_state.as_mut(),
self.ctx,
Expand Down

0 comments on commit ebc49a7

Please sign in to comment.