Skip to content

Commit

Permalink
Make execute_systems_for_view time passing more consistent (#8968)
Browse files Browse the repository at this point in the history
`execute_systems_for_view` took a latest_at time but then would also
assume that the active timeline came from the viewer's time control.
Which is a bit odd - if its configurable then why not take timeline &
time as a tandem as disembodied time doesn't make much sense.
As there's no need to pass in any other time than the current from the
timeline I just removed that argument now, simplifying some code

* [x] click around in the web viewer a bit just in case
  • Loading branch information
Wumpf authored Feb 7, 2025
1 parent bb18071 commit a9fbe2e
Show file tree
Hide file tree
Showing 7 changed files with 11 additions and 47 deletions.
7 changes: 1 addition & 6 deletions crates/viewer/re_view_dataframe/tests/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,7 @@ fn run_view_ui_and_save_snapshot(
let view_state = view_states.get_mut_or_create(view_id, view_class);

let (view_query, system_execution_output) =
re_viewport::execute_systems_for_view(
ctx,
&view_blueprint,
ctx.current_query().at(), // TODO(andreas): why is this even needed to be passed in?
view_state,
);
re_viewport::execute_systems_for_view(ctx, &view_blueprint, view_state);

view_class
.ui(ctx, ui, view_state, &view_query, system_execution_output)
Expand Down
8 changes: 2 additions & 6 deletions crates/viewer/re_view_graph/tests/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,8 @@ fn run_graph_view_and_save_snapshot(test_context: &mut TestContext, name: &str,
let mut view_states = test_context.view_states.lock();
let view_state = view_states.get_mut_or_create(view_id, view_class);

let (view_query, system_execution_output) = re_viewport::execute_systems_for_view(
ctx,
&view_blueprint,
ctx.current_query().at(), // TODO(andreas): why is this even needed to be passed in?
view_state,
);
let (view_query, system_execution_output) =
re_viewport::execute_systems_for_view(ctx, &view_blueprint, view_state);

view_class
.ui(ctx, ui, view_state, &view_query, system_execution_output)
Expand Down
7 changes: 1 addition & 6 deletions crates/viewer/re_view_spatial/tests/annotations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,7 @@ fn run_view_ui_and_save_snapshot(
let view_state = view_states.get_mut_or_create(view_id, view_class);

let (view_query, system_execution_output) =
re_viewport::execute_systems_for_view(
ctx,
&view_blueprint,
ctx.current_query().at(), // TODO(andreas): why is this even needed to be passed in?
view_state,
);
re_viewport::execute_systems_for_view(ctx, &view_blueprint, view_state);

view_class
.ui(ctx, ui, view_state, &view_query, system_execution_output)
Expand Down
7 changes: 1 addition & 6 deletions crates/viewer/re_view_spatial/tests/draw_order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,7 @@ fn run_view_ui_and_save_snapshot(
let view_state = view_states.get_mut_or_create(view_id, view_class);

let (view_query, system_execution_output) =
re_viewport::execute_systems_for_view(
ctx,
&view_blueprint,
ctx.current_query().at(), // TODO(andreas): why is this even needed to be passed in?
view_state,
);
re_viewport::execute_systems_for_view(ctx, &view_blueprint, view_state);

view_class
.ui(ctx, ui, view_state, &view_query, system_execution_output)
Expand Down
7 changes: 1 addition & 6 deletions crates/viewer/re_view_spatial/tests/transform_clamping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,7 @@ fn run_view_ui_and_save_snapshot(

let view_state = view_states.get_mut_or_create(view_id, view_class);
let (view_query, system_execution_output) =
re_viewport::execute_systems_for_view(
ctx,
&view_blueprint,
ctx.current_query().at(), // TODO(andreas): why is this even needed to be passed in?
view_state,
);
re_viewport::execute_systems_for_view(ctx, &view_blueprint, view_state);
view_class
.ui(ctx, ui, view_state, &view_query, system_execution_output)
.expect("failed to run view ui");
Expand Down
13 changes: 4 additions & 9 deletions crates/viewer/re_viewport/src/system_execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::collections::BTreeMap;
use ahash::HashMap;
use rayon::prelude::*;

use re_log_types::TimeInt;
use re_viewer_context::{
PerSystemDataResults, SystemExecutionOutput, ViewContextCollection, ViewId, ViewQuery,
ViewState, ViewStates, ViewerContext, VisualizerCollection,
Expand Down Expand Up @@ -56,7 +55,6 @@ fn run_view_systems(
pub fn execute_systems_for_view<'a>(
ctx: &'a ViewerContext<'_>,
view: &'a ViewBlueprint,
latest_at: TimeInt, // <- TODO(andreas): why not ctx.current_query().at()?
view_state: &dyn ViewState,
) -> (ViewQuery<'a>, SystemExecutionOutput) {
re_tracing::profile_function!(view.class_identifier().as_str());
Expand All @@ -80,12 +78,13 @@ pub fn execute_systems_for_view<'a>(
});
}

let current_query = ctx.rec_cfg.time_ctrl.read().current_query();
let query = re_viewer_context::ViewQuery {
view_id: view.id,
space_origin: &view.space_origin,
per_visualizer_data_results,
timeline: *ctx.rec_cfg.time_ctrl.read().timeline(),
latest_at,
timeline: current_query.timeline(),
latest_at: current_query.at(),
highlights,
};

Expand Down Expand Up @@ -121,10 +120,6 @@ pub fn execute_systems_for_all_views<'a>(
views: &'a BTreeMap<ViewId, ViewBlueprint>,
view_states: &mut ViewStates,
) -> HashMap<ViewId, (ViewQuery<'a>, SystemExecutionOutput)> {
let Some(time_int) = ctx.rec_cfg.time_ctrl.read().time_int() else {
return Default::default();
};

re_tracing::profile_wait!("execute_systems");

// During system execution we only have read access to the view states, so we need to ensure they exist ahead of time.
Expand All @@ -142,7 +137,7 @@ pub fn execute_systems_for_all_views<'a>(
return None;
};

let result = execute_systems_for_view(ctx, view, time_int, view_state);
let result = execute_systems_for_view(ctx, view, view_state);
Some((*view_id, result))
}),
egui_tiles::Tile::Container(_) => None,
Expand Down
9 changes: 1 addition & 8 deletions crates/viewer/re_viewport/src/viewport_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,13 +341,6 @@ impl<'a> egui_tiles::Behavior<ViewId> for TilesDelegate<'a, '_> {
return Default::default();
}

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

let (query, system_output) = self.executed_systems_per_view.remove(view_id).unwrap_or_else(|| {
// The view's systems haven't been executed.
// This may indicate that the egui_tiles tree is not in sync
Expand Down Expand Up @@ -385,7 +378,7 @@ impl<'a> egui_tiles::Behavior<ViewId> for TilesDelegate<'a, '_> {
}

let class = view_blueprint.class(self.ctx.view_class_registry);
execute_systems_for_view(ctx, view, latest_at, self.view_states.get_mut_or_create(*view_id, class))
execute_systems_for_view(ctx, view, self.view_states.get_mut_or_create(*view_id, class))
});

let class = view_blueprint.class(self.ctx.view_class_registry);
Expand Down

0 comments on commit a9fbe2e

Please sign in to comment.