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

Improved automatic view creation heuristic, major speedup for scenes with many entities #4874

Merged
merged 29 commits into from
Jan 28, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a05fd3f
New SpaceViewSpawnHeuristics, implemented for all space views individ…
Wumpf Jan 18, 2024
cd925bc
wire in the new heuristics to be used for default_created_space_views
Wumpf Jan 19, 2024
2a0a609
wip
Wumpf Jan 19, 2024
9d33bea
use indicator components more intensively in space view spawn heuristics
Wumpf Jan 19, 2024
2cde52b
fix space view 3d heuristic checking for 2d stuff
Wumpf Jan 19, 2024
cfc0eb1
missing profiler macros
Wumpf Jan 19, 2024
796a04a
remove debug log
Wumpf Jan 19, 2024
c4f7149
update many entities test a bit
Wumpf Jan 19, 2024
05d1353
Remove old AutoSpawnHeuristic
Wumpf Jan 19, 2024
7af18e4
indicator component for bar chart spawning now required (tensor compo…
Wumpf Jan 19, 2024
a61f193
Merge remote-tracking branch 'origin/main' into andreas/distributed-s…
Wumpf Jan 23, 2024
4008161
Merge remote-tracking branch 'origin/main' into andreas/distributed-s…
Wumpf Jan 24, 2024
52a8c02
Fix code-example gen causing clippy warnings on windows
Wumpf Jan 24, 2024
fcd8130
reintroduce image bucketing for 2d space view spawn heuristics
Wumpf Jan 24, 2024
4d7b109
use split root heuristic for 3d spaces as well, take ViewCoordinates …
Wumpf Jan 24, 2024
051745f
objectron should now work without logging workaround
Wumpf Jan 24, 2024
d5a3096
typo fixes
Wumpf Jan 24, 2024
c449c14
'...' replacements, missing docs
Wumpf Jan 24, 2024
7129867
Merge remote-tracking branch 'origin/main' into andreas/distributed-s…
Wumpf Jan 24, 2024
17ab438
optimize `bucket_images_in_subspace`
Wumpf Jan 24, 2024
c6c9f6b
even more '…'
Wumpf Jan 24, 2024
c6ffce0
take applicability into account where we only checked for indicator s…
Wumpf Jan 24, 2024
54631d5
clarify & rename recommend_space_views_for_all_default_visualized_en…
Wumpf Jan 28, 2024
b898864
commentary on child-of-root heuristics
Wumpf Jan 28, 2024
3c81785
return single item buckets
Wumpf Jan 28, 2024
056bf07
only bucket images by size
Wumpf Jan 28, 2024
7606657
clarifying comments, missing empty check
Wumpf Jan 28, 2024
31a812f
rename indicator_matching_entities_per_visualizer to indicated_entiti…
Wumpf Jan 28, 2024
ce72c01
Merge remote-tracking branch 'origin/main' into andreas/distributed-s…
Wumpf Jan 28, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/re_data_store/src/store_write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ impl DataStore {
} else {
// Cache miss! Craft a new instance keys from the ground up.

// ...so we create it manually instead.
// so we create it manually instead.
let values =
arrow2::array::UInt64Array::from_vec((0..num_instances as u64).collect_vec())
.boxed();
Expand Down
6 changes: 3 additions & 3 deletions crates/re_entity_db/src/entity_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,9 +563,9 @@ impl EntityTree {
subtree_recursive(self, path.as_slice())
}

// Invokes visitor for `self` all children recursively.
pub fn visit_children_recursively(&self, visitor: &mut impl FnMut(&EntityPath)) {
visitor(&self.path);
// Invokes visitor for `self` and all children recursively.
pub fn visit_children_recursively(&self, visitor: &mut impl FnMut(&EntityPath, &EntityInfo)) {
visitor(&self.path, &self.entity);
for child in self.children.values() {
child.visit_children_recursively(visitor);
}
Expand Down
14 changes: 14 additions & 0 deletions crates/re_log_types/src/path/entity_path_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,20 @@ impl EntityPathFilter {
filter
}

/// Creates a new entity path filter that includes only a single entity.
pub fn single_entity_filter(entity_path: &EntityPath) -> Self {
let mut filter = Self::default();
filter.add_exact(entity_path.clone());
filter
}

/// Creates a new entity path filter that includes a single subtree.
pub fn subtree_entity_filter(entity_path: &EntityPath) -> Self {
let mut filter = Self::default();
filter.add_subtree(entity_path.clone());
filter
}

pub fn add_rule(&mut self, effect: RuleEffect, rule: EntityPathRule) {
self.rules.insert(rule, effect);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/re_sdk/src/recording_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1158,7 +1158,7 @@ impl RecordingStream {
// Get the current time on all timelines, for the current recording, on the current
// thread…
let mut now = self.now();
// ...and then also inject the current recording tick into it.
// and then also inject the current recording tick into it.
now.insert(Timeline::log_tick(), tick.into());

// Inject all these times into the row, overriding conflicting times, if any.
Expand Down
10 changes: 3 additions & 7 deletions crates/re_space_view/src/data_query_blueprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,7 @@ impl<'a> QueryExpressionEvaluator<'a> {
if self
.indicator_matching_entities_per_visualizer
.get(visualizer)
.map_or(false, |matching_list| {
matching_list.contains(&entity_path.hash())
})
.map_or(false, |matching_list| matching_list.contains(entity_path))
{
Some(*visualizer)
} else {
Expand Down Expand Up @@ -404,7 +402,7 @@ impl DataQueryPropertyResolver<'_> {
let mut prop_map = self.auto_properties.clone();

if let Some(tree) = blueprint.tree().subtree(override_root) {
tree.visit_children_recursively(&mut |path: &EntityPath| {
tree.visit_children_recursively(&mut |path: &EntityPath, _| {
if let Some(props) = blueprint
.store()
.query_latest_component_quiet::<EntityPropertiesComponent>(path, query)
Expand Down Expand Up @@ -667,9 +665,7 @@ mod tests {
.map(|(id, entities)| {
(
*id,
IndicatorMatchingEntities(
entities.0.iter().map(|path| path.hash()).collect(),
),
IndicatorMatchingEntities(entities.0.iter().cloned().collect()),
)
})
.collect(),
Expand Down
55 changes: 55 additions & 0 deletions crates/re_space_view/src/heuristics.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
use re_log_types::EntityPathFilter;
use re_viewer_context::{
ApplicableEntities, IdentifiedViewSystem, RecommendedSpaceView, SpaceViewClass,
SpaceViewSpawnHeuristics, ViewerContext, VisualizerSystem,
};

/// Spawns a space view for each single entity which is visualizable & indicator-matching for a given visualizer.
Wumpf marked this conversation as resolved.
Show resolved Hide resolved
pub fn recommend_space_views_for_all_default_visualized_entities_for<TVisualizer>(
ctx: &ViewerContext<'_>,
space_view: &impl SpaceViewClass,
) -> SpaceViewSpawnHeuristics
where
TVisualizer: VisualizerSystem + IdentifiedViewSystem + Default,
{
re_tracing::profile_function!();

let Some(indicator_matching_entities) = ctx
.indicator_matching_entities_per_visualizer
.get(&TVisualizer::identifier())
else {
return Default::default();
};
let Some(applicable_entities) = ctx
.applicable_entities_per_visualizer
.get(&TVisualizer::identifier())
else {
return Default::default();
};

let visualizer = TVisualizer::default();
Copy link
Member

Choose a reason for hiding this comment

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

The need to instantiate this feels weird to me but I'm guessing the self in the signature stems from some context where we need to dynamically dispatch across a set of visualizers instead of being templated on <TVisualizer>?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah all these self-passings are about object safety really. We'd like this to be static but Rust makes this messy

let recommended_space_views = applicable_entities
.intersection(indicator_matching_entities)
.filter_map(|entity| {
let context = space_view.visualizable_filter_context(entity, ctx.entity_db);
if visualizer
.filter_visualizable_entities(
ApplicableEntities(std::iter::once(entity.clone()).collect()),
context.as_ref(),
)
.is_empty()
{
None
} else {
Some(RecommendedSpaceView {
root: entity.clone(),
query_filter: EntityPathFilter::single_entity_filter(entity),
})
}
})
.collect();

re_viewer_context::SpaceViewSpawnHeuristics {
recommended_space_views,
}
}
2 changes: 2 additions & 0 deletions crates/re_space_view/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ pub mod blueprint;
pub mod controls;
mod data_query;
mod data_query_blueprint;
mod heuristics;
mod screenshot;
mod unreachable_transform_reason;

pub use data_query::{DataQuery, EntityOverrideContext, PropertyResolver};
pub use data_query_blueprint::DataQueryBlueprint;
pub use heuristics::recommend_space_views_for_all_default_visualized_entities_for;
pub use screenshot::ScreenshotMode;
pub use unreachable_transform_reason::UnreachableTransformReason;

Expand Down
12 changes: 11 additions & 1 deletion crates/re_space_view_bar_chart/src/space_view_class.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use egui::util::hash;
use re_entity_db::{EditableAutoValue, EntityProperties, LegendCorner};
use re_log_types::EntityPath;
use re_space_view::controls;
use re_space_view::{controls, recommend_space_views_for_all_default_visualized_entities_for};
use re_types::datatypes::TensorBuffer;
use re_viewer_context::{
auto_color, SpaceViewClass, SpaceViewClassRegistryError, SpaceViewId,
Expand Down Expand Up @@ -59,6 +59,16 @@ impl SpaceViewClass for BarChartSpaceView {
None
}

fn spawn_heuristics(
&self,
ctx: &ViewerContext<'_>,
) -> re_viewer_context::SpaceViewSpawnHeuristics {
re_tracing::profile_function!();
recommend_space_views_for_all_default_visualized_entities_for::<BarChartVisualizerSystem>(
ctx, self,
)
}

fn layout_priority(&self) -> re_viewer_context::SpaceViewClassLayoutPriority {
re_viewer_context::SpaceViewClassLayoutPriority::Low
}
Expand Down
12 changes: 2 additions & 10 deletions crates/re_space_view_bar_chart/src/visualizer_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ use re_data_store::LatestAtQuery;
use re_entity_db::EntityPath;
use re_space_view::diff_component_filter;
use re_types::{
archetypes::{BarChart, Tensor},
components::Color,
datatypes::TensorData,
Archetype, ComponentNameSet,
archetypes::BarChart, components::Color, datatypes::TensorData, Archetype, ComponentNameSet,
};
use re_viewer_context::{
IdentifiedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, ViewQuery,
Expand Down Expand Up @@ -45,12 +42,7 @@ impl VisualizerSystem for BarChartVisualizerSystem {
}

fn indicator_components(&self) -> ComponentNameSet {
// TODO(#3342): For now, we relax the indicator component heuristics on bar charts so that
// logging a 1D tensor also results in a bar chart view, rather than a broken viewer (see #3709).
// Ideally though, this should be implemented using an heuristic fallback mechanism.
[BarChart::indicator().name(), Tensor::indicator().name()]
.into_iter()
.collect()
std::iter::once(BarChart::indicator().name()).collect()
}

fn applicability_filter(&self) -> Option<Box<dyn VisualizerAdditionalApplicabilityFilter>> {
Expand Down
14 changes: 6 additions & 8 deletions crates/re_space_view_dataframe/src/space_view_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ use re_entity_db::{EntityProperties, InstancePath};
use re_log_types::{EntityPath, Timeline};
use re_query::get_component_with_instances;
use re_viewer_context::{
AutoSpawnHeuristic, PerSystemEntities, SpaceViewClass, SpaceViewClassRegistryError,
SpaceViewId, SpaceViewSystemExecutionError, SystemExecutionOutput, UiVerbosity, ViewQuery,
ViewerContext,
SpaceViewClass, SpaceViewClassRegistryError, SpaceViewId, SpaceViewSystemExecutionError,
SystemExecutionOutput, UiVerbosity, ViewQuery, ViewerContext,
};

use crate::visualizer_system::EmptySystem;
Expand Down Expand Up @@ -53,13 +52,12 @@ impl SpaceViewClass for DataframeSpaceView {
re_viewer_context::SpaceViewClassLayoutPriority::Low
}

fn auto_spawn_heuristic(
fn spawn_heuristics(
&self,
_ctx: &ViewerContext<'_>,
_space_origin: &EntityPath,
_ent_paths: &PerSystemEntities,
) -> re_viewer_context::AutoSpawnHeuristic {
AutoSpawnHeuristic::NeverSpawn
) -> re_viewer_context::SpaceViewSpawnHeuristics {
// Doesn't spawn anything by default.
Default::default()
}

fn selection_ui(
Expand Down
Loading
Loading