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

Improve heuristics around 2D vs 3D space-view creation #3822

Merged
merged 7 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions crates/re_space_view_bar_chart/src/view_part_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use re_types::{
Archetype, ComponentNameSet,
};
use re_viewer_context::{
default_heuristic_filter, NamedViewSystem, SpaceViewSystemExecutionError,
ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
default_heuristic_filter, HeuristicFilterContext, NamedViewSystem,
SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
};

/// A bar chart system, with everything needed to render it.
Expand Down Expand Up @@ -45,6 +45,7 @@ impl ViewPartSystem for BarChartViewPartSystem {
&self,
store: &re_arrow_store::DataStore,
ent_path: &EntityPath,
_ctx: HeuristicFilterContext,
query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
Expand Down
5 changes: 0 additions & 5 deletions crates/re_space_view_spatial/src/heuristics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ pub fn auto_spawn_heuristic(
}
}

if view_kind == SpatialSpaceViewKind::TwoD {
// Prefer 2D views over 3D views.
score += 0.1;
}

AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(score)
}

Expand Down
25 changes: 23 additions & 2 deletions crates/re_space_view_spatial/src/parts/boxes2d.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use re_arrow_store::LatestAtQuery;
use re_data_store::{EntityPath, InstancePathHash};
use re_query::{ArchetypeView, QueryError};
use re_types::{
Expand All @@ -6,8 +7,8 @@ use re_types::{
Archetype, ComponentNameSet,
};
use re_viewer_context::{
NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
default_heuristic_filter, HeuristicFilterContext, NamedViewSystem, ResolvedAnnotationInfos,
SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
};

use crate::{
Expand Down Expand Up @@ -177,6 +178,26 @@ impl ViewPartSystem for Boxes2DPart {
std::iter::once(Boxes2D::indicator().name()).collect()
}

fn heuristic_filter(
&self,
_store: &re_arrow_store::DataStore,
_ent_path: &EntityPath,
ctx: HeuristicFilterContext,
_query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
if !default_heuristic_filter(entity_components, &self.indicator_components()) {
return false;
}

// If this is a 3D view and there's no parent pinhole, do not include this part.
if ctx.class == "3D" && !ctx.has_ancestor_pinhole {
return false;
}
Comment on lines +193 to +196
Copy link
Member

Choose a reason for hiding this comment

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

Again - why?


true
}

fn execute(
&mut self,
ctx: &mut ViewerContext<'_>,
Expand Down
8 changes: 7 additions & 1 deletion crates/re_space_view_spatial/src/parts/images.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use re_types::{
Archetype as _, ComponentNameSet,
};
use re_viewer_context::{
default_heuristic_filter, gpu_bridge, DefaultColor, SpaceViewClass,
default_heuristic_filter, gpu_bridge, DefaultColor, HeuristicFilterContext, SpaceViewClass,
SpaceViewSystemExecutionError, TensorDecodeCache, TensorStatsCache, ViewPartSystem, ViewQuery,
ViewerContext,
};
Expand Down Expand Up @@ -681,13 +681,19 @@ impl ViewPartSystem for ImagesPart {
&self,
store: &re_arrow_store::DataStore,
ent_path: &EntityPath,
ctx: HeuristicFilterContext,
query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
if !default_heuristic_filter(entity_components, &self.indicator_components()) {
return false;
}

// If this is a 3D view and there's no parent pinhole, do not include this part.
if ctx.class == "3D" && !ctx.has_ancestor_pinhole {
return false;
}
Comment on lines +692 to +695
Copy link
Member

Choose a reason for hiding this comment

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

Why though?

Copy link
Member

Choose a reason for hiding this comment

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

I can already read the Rust code and see what it does, I want to know why it does it!

Copy link
Member Author

Choose a reason for hiding this comment

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


// NOTE: We want to make sure we query at the right time, otherwise we might take into
// account a `Clear()` that actually only applies into the future, and then
// `is_shaped_like_an_image` will righfully fail because of the empty tensor.
Expand Down
25 changes: 23 additions & 2 deletions crates/re_space_view_spatial/src/parts/lines2d.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use re_arrow_store::LatestAtQuery;
use re_data_store::{EntityPath, InstancePathHash};
use re_query::{ArchetypeView, QueryError};
use re_types::{
Expand All @@ -6,8 +7,8 @@ use re_types::{
Archetype as _, ComponentNameSet,
};
use re_viewer_context::{
NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
default_heuristic_filter, HeuristicFilterContext, NamedViewSystem, ResolvedAnnotationInfos,
SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
};

use crate::{
Expand Down Expand Up @@ -172,6 +173,26 @@ impl ViewPartSystem for Lines2DPart {
std::iter::once(LineStrips2D::indicator().name()).collect()
}

fn heuristic_filter(
&self,
_store: &re_arrow_store::DataStore,
_ent_path: &EntityPath,
ctx: HeuristicFilterContext,
_query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
if !default_heuristic_filter(entity_components, &self.indicator_components()) {
return false;
}

// If this is a 3D view and there's no parent pinhole, do not include this part.
if ctx.class == "3D" && !ctx.has_ancestor_pinhole {
return false;
}

true
}

fn execute(
&mut self,
ctx: &mut ViewerContext<'_>,
Expand Down
25 changes: 23 additions & 2 deletions crates/re_space_view_spatial/src/parts/points2d.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use re_arrow_store::LatestAtQuery;
use re_data_store::{EntityPath, InstancePathHash};
use re_query::{ArchetypeView, QueryError};
use re_types::{
Expand All @@ -6,8 +7,8 @@ use re_types::{
Archetype, ComponentNameSet,
};
use re_viewer_context::{
NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, ViewContextCollection,
ViewPartSystem, ViewQuery, ViewerContext,
default_heuristic_filter, HeuristicFilterContext, NamedViewSystem, ResolvedAnnotationInfos,
SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
};

use crate::{
Expand Down Expand Up @@ -199,6 +200,26 @@ impl ViewPartSystem for Points2DPart {
std::iter::once(Points2D::indicator().name()).collect()
}

fn heuristic_filter(
&self,
_store: &re_arrow_store::DataStore,
_ent_path: &EntityPath,
ctx: HeuristicFilterContext,
_query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
if !default_heuristic_filter(entity_components, &self.indicator_components()) {
return false;
}

// If this is a 3D view and there's no parent pinhole, do not include this part.
if ctx.class == "3D" && !ctx.has_ancestor_pinhole {
return false;
}
Comment on lines +215 to +218
Copy link
Member

Choose a reason for hiding this comment

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

I've read this many times now


true
}

fn execute(
&mut self,
ctx: &mut ViewerContext<'_>,
Expand Down
49 changes: 45 additions & 4 deletions crates/re_space_view_spatial/src/space_view_2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use re_viewer_context::{
use crate::{
contexts::{register_spatial_contexts, PrimitiveCounter},
heuristics::{auto_spawn_heuristic, update_object_property_heuristics},
parts::{calculate_bounding_box, register_2d_spatial_parts},
parts::{calculate_bounding_box, register_2d_spatial_parts, SpatialViewPartData},
ui::SpatialSpaceViewState,
view_kind::SpatialSpaceViewKind,
};
Expand Down Expand Up @@ -69,10 +69,51 @@ impl SpaceViewClass for SpatialSpaceView2D {
fn auto_spawn_heuristic(
&self,
ctx: &ViewerContext<'_>,
_space_origin: &EntityPath,
ent_paths: &PerSystemEntities,
space_origin: &EntityPath,
per_system_entities: &PerSystemEntities,
) -> AutoSpawnHeuristic {
auto_spawn_heuristic(&self.name(), ctx, ent_paths, SpatialSpaceViewKind::TwoD)
let mut score = auto_spawn_heuristic(
&self.name(),
ctx,
per_system_entities,
SpatialSpaceViewKind::TwoD,
);

// If this is the root space view, and it contains a part that would
// prefer to be 3D, don't spawn the 2D view. This is because it's never
// possible to correctly project 3d objects to a root 2d view since the
// the pinhole would go past the root.
Comment on lines +84 to +85
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow this. "go past the root"?

if space_origin.is_root() {
let parts = ctx
.space_view_class_registry
.get_system_registry_or_log_error(&self.name())
.new_part_collection();

for part in per_system_entities.keys() {
if let Ok(part) = parts.get_by_name(*part) {
if let Some(part_data) = part
.data()
.and_then(|d| d.downcast_ref::<SpatialViewPartData>())
{
if part_data.preferred_view_kind == Some(SpatialSpaceViewKind::ThreeD) {
return AutoSpawnHeuristic::NeverSpawn;
}
}
}
}
}

if let AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(score) = &mut score {
// If a 2D view contains nothing inherently 2D in nature, don't
// spawn it, though in all other cases default to 2D over 3D as a tie-breaker.
if *score == 0.0 {
return AutoSpawnHeuristic::NeverSpawn;
} else {
*score += 0.1;
}
}

score
}

fn selection_ui(
Expand Down
6 changes: 4 additions & 2 deletions crates/re_space_view_tensor/src/view_part_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ use re_types::{
ComponentNameSet,
};
use re_viewer_context::{
default_heuristic_filter, NamedViewSystem, SpaceViewSystemExecutionError, TensorDecodeCache,
ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext,
default_heuristic_filter, HeuristicFilterContext, NamedViewSystem,
SpaceViewSystemExecutionError, TensorDecodeCache, ViewContextCollection, ViewPartSystem,
ViewQuery, ViewerContext,
};

#[derive(Default)]
Expand Down Expand Up @@ -37,6 +38,7 @@ impl ViewPartSystem for TensorSystem {
&self,
store: &re_arrow_store::DataStore,
ent_path: &EntityPath,
_ctx: HeuristicFilterContext,
query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
Expand Down
12 changes: 6 additions & 6 deletions crates/re_viewer_context/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ pub use selection_state::{
HoverHighlight, HoveredSpace, InteractionHighlight, SelectionHighlight, SelectionState,
};
pub use space_view::{
default_heuristic_filter, AutoSpawnHeuristic, DynSpaceViewClass, NamedViewSystem,
PerSystemEntities, SpaceViewClass, SpaceViewClassLayoutPriority, SpaceViewClassName,
SpaceViewClassRegistry, SpaceViewClassRegistryError, SpaceViewEntityHighlight,
SpaceViewHighlights, SpaceViewOutlineMasks, SpaceViewState, SpaceViewSystemExecutionError,
SpaceViewSystemRegistry, ViewContextCollection, ViewContextSystem, ViewPartCollection,
ViewPartSystem, ViewQuery, ViewSystemName,
default_heuristic_filter, AutoSpawnHeuristic, DynSpaceViewClass, HeuristicFilterContext,
NamedViewSystem, PerSystemEntities, SpaceViewClass, SpaceViewClassLayoutPriority,
SpaceViewClassName, SpaceViewClassRegistry, SpaceViewClassRegistryError,
SpaceViewEntityHighlight, SpaceViewHighlights, SpaceViewOutlineMasks, SpaceViewState,
SpaceViewSystemExecutionError, SpaceViewSystemRegistry, ViewContextCollection,
ViewContextSystem, ViewPartCollection, ViewPartSystem, ViewQuery, ViewSystemName,
};
pub use store_context::StoreContext;
pub use tensor::{TensorDecodeCache, TensorStats, TensorStatsCache};
Expand Down
4 changes: 3 additions & 1 deletion crates/re_viewer_context/src/space_view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ pub use space_view_class_registry::{
SpaceViewClassRegistry, SpaceViewClassRegistryError, SpaceViewSystemRegistry,
};
pub use view_context_system::{ViewContextCollection, ViewContextSystem};
pub use view_part_system::{default_heuristic_filter, ViewPartCollection, ViewPartSystem};
pub use view_part_system::{
default_heuristic_filter, HeuristicFilterContext, ViewPartCollection, ViewPartSystem,
};
pub use view_query::ViewQuery;

// ---------------------------------------------------------------------------
Expand Down
35 changes: 32 additions & 3 deletions crates/re_viewer_context/src/space_view/view_part_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,38 @@ use re_log_types::EntityPath;
use re_types::ComponentNameSet;

use crate::{
NamedViewSystem, SpaceViewSystemExecutionError, ViewContextCollection, ViewQuery,
ViewSystemName, ViewerContext,
NamedViewSystem, SpaceViewClassName, SpaceViewSystemExecutionError, ViewContextCollection,
ViewQuery, ViewSystemName, ViewerContext,
};

/// This is additional context made available to the `heuristic_filter`.
/// It includes tree-context information such as whether certain components
/// exist in the parent hierarchy which are better computed once than having
/// each entity do their own tree-walk.
#[derive(Clone, Copy, Debug)]
pub struct HeuristicFilterContext {
pub class: SpaceViewClassName,
pub has_ancestor_pinhole: bool,
}

impl Default for HeuristicFilterContext {
fn default() -> HeuristicFilterContext {
Self {
class: SpaceViewClassName::invalid(),
has_ancestor_pinhole: false,
}
}
}

impl HeuristicFilterContext {
pub fn with_class(&self, class: SpaceViewClassName) -> Self {
Self {
class,
has_ancestor_pinhole: self.has_ancestor_pinhole,
}
}
}

/// Element of a scene derived from a single archetype query.
///
/// Is populated after scene contexts and has access to them.
Expand Down Expand Up @@ -46,6 +74,7 @@ pub trait ViewPartSystem {
&self,
_store: &re_arrow_store::DataStore,
_ent_path: &EntityPath,
_ctx: HeuristicFilterContext,
_query: &LatestAtQuery,
entity_components: &ComponentNameSet,
) -> bool {
Expand Down Expand Up @@ -80,7 +109,7 @@ pub trait ViewPartSystem {

/// The default implementation for [`ViewPartSystem::heuristic_filter`].
///
/// Returns true if eiher `indicator_components` is empty or `entity_components` contains at least one
/// Returns true if either `indicator_components` is empty or `entity_components` contains at least one
/// of these indicator components.
///
/// Exported as a standalone function to simplify the implementation of custom filters.
Expand Down
9 changes: 8 additions & 1 deletion crates/re_viewport/src/space_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use re_viewer_context::{
use crate::{
space_info::SpaceInfoCollection,
space_view_heuristics::{
is_entity_processed_by_class, reachable_entities_from_root, EntitiesPerSystem,
compute_heuristic_context_for_entities, is_entity_processed_by_class,
reachable_entities_from_root, EntitiesPerSystem,
},
};

Expand Down Expand Up @@ -255,12 +256,18 @@ impl SpaceViewBlueprint {
) {
re_tracing::profile_function!();

let heuristic_context = compute_heuristic_context_for_entities(ctx);

let mut entities = Vec::new();
tree.visit_children_recursively(&mut |entity_path: &EntityPath| {
if is_entity_processed_by_class(
ctx,
&self.class_name,
entity_path,
heuristic_context
.get(entity_path)
.copied()
.unwrap_or_default(),
&ctx.current_query(),
) && !self.contents.contains_entity(entity_path)
&& spaces_info
Expand Down
Loading
Loading