Skip to content

Commit

Permalink
Fix parents of queried paths getting visualized, fix 2D objects not s…
Browse files Browse the repository at this point in the history
…howing at all in 3D if their camera parent is not included (#5424)

### What

> Fix parents of queried paths getting visualized

The is about`tree_prefix_only` (former `direct_included`, inverted now)
not causing visualizer gathering to stop.
We now also grey out everything that doesn't have a visualizer in the
blueprint hierarchy. Debatable whether that should be just tree prefixes
(i.e. could instead _not_ grey out if a result is the query but doesn't
have a visualizer?). I found this behavior nicer since "not having
visualizer" is equivalent to being invisible (which also causes greying
out)

> fix 2D objects not showing at all in 3D if their camera parent is not
included

long story short: `pinhole_image_plane_distance` would default to 0.0 if
the camera is not present. Bandaid is to make it default to 1.0 which is
better anyways. Hoping this will be solved better once
`EntityProperties` is gone.



![image](https://github.com/rerun-io/rerun/assets/1220815/eb47b133-69a1-4c7c-b61f-81f92404585c)



### 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/5424/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5424/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/5424/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/5424)
- [Docs
preview](https://rerun.io/preview/856dbd5fafcc8910b5e416590345aef504953a7f/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/856dbd5fafcc8910b5e416590345aef504953a7f/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 authored Mar 8, 2024
1 parent 80d2af2 commit 9f65a12
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 33 deletions.
2 changes: 1 addition & 1 deletion crates/re_entity_db/src/entity_properties.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl Default for EntityProperties {
visible_history: re_query::ExtraQueryHistory::default(),
interactive: true,
color_mapper: EditableAutoValue::default(),
pinhole_image_plane_distance: EditableAutoValue::default(),
pinhole_image_plane_distance: EditableAutoValue::Auto(1.0),
backproject_depth: EditableAutoValue::Auto(true),
depth_from_world_scale: EditableAutoValue::Auto(1.0),
backproject_radius_scale: EditableAutoValue::Auto(1.0),
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view/src/space_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ impl SpaceViewBlueprint {
DataResult {
entity_path: entity_path.clone(),
visualizers: Default::default(),
direct_included: true,
tree_prefix_only: false,
property_overrides: Some(PropertyOverrides {
accumulated_properties,
individual_properties,
Expand Down
17 changes: 11 additions & 6 deletions crates/re_space_view/src/space_view_contents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,16 +283,21 @@ impl<'a> QueryExpressionEvaluator<'a> {

let entity_path = &tree.path;

let tree_prefix_only = !self.entity_path_filter.is_included(entity_path);

// TODO(#5067): For now, we always start by setting visualizers to the full list of available visualizers.
// This is currently important for evaluating auto-properties during the space-view `on_frame_start`, which
// is called before the property-overrider has a chance to update this list.
// This list will be updated below during `update_overrides_recursive` by calling `choose_default_visualizers`
// on the space view.
let visualizers: SmallVec<[_; 4]> = self
.visualizable_entities_for_visualizer_systems
.iter()
.filter_map(|(visualizer, ents)| ents.contains(entity_path).then_some(*visualizer))
.collect();
let visualizers: SmallVec<[_; 4]> = if tree_prefix_only {
Default::default()
} else {
self.visualizable_entities_for_visualizer_systems
.iter()
.filter_map(|(visualizer, ents)| ents.contains(entity_path).then_some(*visualizer))
.collect()
};

let children: SmallVec<[_; 4]> = tree
.children
Expand All @@ -311,7 +316,7 @@ impl<'a> QueryExpressionEvaluator<'a> {
data_result: DataResult {
entity_path: entity_path.clone(),
visualizers,
direct_included: self.entity_path_filter.is_included(entity_path),
tree_prefix_only,
property_overrides: None,
},
children,
Expand Down
14 changes: 1 addition & 13 deletions crates/re_viewer_context/src/query_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,7 @@ impl DataQueryResult {
pub fn contains_entity(&self, path: &EntityPath) -> bool {
self.tree
.lookup_result_by_path(path)
.map_or(false, |result| result.direct_included)
}

#[inline]
pub fn contains_group(&self, path: &EntityPath) -> bool {
self.tree
.lookup_result_by_path(path)
.map_or(false, |result| result.direct_included)
}

#[inline]
pub fn contains_any(&self, path: &EntityPath) -> bool {
self.contains_entity(path) || self.contains_group(path)
.map_or(false, |result| !result.tree_prefix_only)
}
}

Expand Down
8 changes: 5 additions & 3 deletions crates/re_viewer_context/src/space_view/view_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,11 @@ pub struct DataResult {
/// Which `ViewSystems`s to pass the `DataResult` to.
pub visualizers: SmallVisualizerSet,

// This result was actually in the query results, not just a path that
// exists due to a common prefix.
pub direct_included: bool,
/// If true, this path is not actually included in the query results and is just here
/// because of a common prefix.
///
/// If this is true, `visualizers` must be empty.
pub tree_prefix_only: bool,

/// The accumulated property overrides for this `DataResult`.
pub property_overrides: Option<PropertyOverrides>,
Expand Down
2 changes: 1 addition & 1 deletion crates/re_viewport/src/space_view_entity_picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ fn create_entity_add_info(
let can_add: CanAddToSpaceView =
if visualizable_entities.iter().any(|(_, entities)| entities.contains(entity_path)) {
CanAddToSpaceView::Compatible {
already_added: query_result.contains_any(entity_path),
already_added: query_result.contains_entity(entity_path),
}
} else {
// TODO(#4826): This shouldn't necessarily prevent us from adding it.
Expand Down
15 changes: 7 additions & 8 deletions crates/re_viewport/src/viewport_blueprint_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,16 +307,17 @@ impl Viewport<'_, '_> {
// The later is important since `+ image/camera/**` necessarily has `image` and `image/camera` in the data result tree.
let mut projections = Vec::new();
result_tree.visit(&mut |node| {
if !node
if node
.data_result
.entity_path
.starts_with(&space_view.space_origin)
&& node.data_result.direct_included
{
projections.push(node);
false
false // If its under the origin, we're not interested, stop recursing.
} else if node.data_result.tree_prefix_only {
true // Keep recursing until we find a projection.
} else {
true
projections.push(node);
false // We found a projection, stop recursing as everything below is now included in the projections.
}
});
if !projections.is_empty() {
Expand Down Expand Up @@ -430,9 +431,7 @@ impl Viewport<'_, '_> {

let subdued = !space_view_visible
|| !visible
|| data_result_node.map_or(true, |n| {
n.data_result.visualizers.is_empty() && n.children.is_empty()
});
|| data_result_node.map_or(true, |n| n.data_result.visualizers.is_empty());

let list_item = ListItem::new(ctx.re_ui, item_label)
.selected(is_selected)
Expand Down

0 comments on commit 9f65a12

Please sign in to comment.