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

Replace complex uses of query_entity_with_primary with query_latest_single #2137

Merged
merged 15 commits into from
May 16, 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
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.

7 changes: 3 additions & 4 deletions crates/re_data_store/src/entity_properties.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use re_arrow_store::LatestAtQuery;
use re_log_types::{DeserializableComponent, EntityPath};

use crate::log_db::EntityDb;

#[cfg(feature = "serde")]
use crate::EditableAutoValue;

Expand Down Expand Up @@ -195,8 +193,10 @@ impl Default for ColorMapper {
///
/// This assumes that the row we get from the store only contains a single instance for this
/// component; it will log a warning otherwise.
///
/// This should only be used for "mono-components" such as `Transform` and `Tensor`.
pub fn query_latest_single<C: DeserializableComponent>(
entity_db: &EntityDb,
data_store: &re_arrow_store::DataStore,
entity_path: &EntityPath,
query: &LatestAtQuery,
) -> Option<C>
Expand All @@ -208,7 +208,6 @@ where
// Although it would be nice to use the `re_query` helpers for this, we would need to move
// this out of re_data_store to avoid a circular dep. Since we don't need to do a join for
// single components this is easy enough.
let data_store = &entity_db.data_store;

let (_, cells) = data_store.latest_at(query, entity_path, C::name(), &[C::name()])?;
let cell = cells.get(0)?.as_ref()?;
Expand Down
3 changes: 2 additions & 1 deletion crates/re_data_ui/src/annotation_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ fn annotation_info(
query: &re_arrow_store::LatestAtQuery,
keypoint_id: &re_log_types::component_types::KeypointId,
) -> Option<re_log_types::context::AnnotationInfo> {
let class_id = re_data_store::query_latest_single(&ctx.log_db.entity_db, entity_path, query)?;
let class_id =
re_data_store::query_latest_single(&ctx.log_db.entity_db.data_store, entity_path, query)?;
let annotations = crate::annotations(ctx, query, entity_path);
let class = annotations
.class_description(Some(class_id))
Expand Down
3 changes: 3 additions & 0 deletions crates/re_error/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,8 @@ version.workspace = true
all-features = true


[dependencies]
re_log.workspace = true

[dev-dependencies]
anyhow.workspace = true
17 changes: 17 additions & 0 deletions crates/re_error/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,20 @@ fn test_format() {
// Now we do:
assert_eq!(format(&err), "outer_context -> inner_context -> root_cause");
}

pub trait ResultExt<T> {
fn warn_on_err_once(self, msg: impl std::fmt::Display) -> Option<T>;
}

impl<T, E: std::fmt::Display> ResultExt<T> for Result<T, E> {
/// Log a warning if there is an `Err`, but only log the exact same message once.
fn warn_on_err_once(self, msg: impl std::fmt::Display) -> Option<T> {
match self {
Ok(value) => Some(value),
Err(err) => {
re_log::warn_once!("{msg}: {err}");
None
}
}
}
}
3 changes: 3 additions & 0 deletions crates/re_log_types/src/component_types/tensor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,9 @@ pub enum TensorDataMeaning {
///
/// All clones are shallow.
///
/// The `Tensor` component is special, as you can only have one instance of it per entity.
/// This is because each element in a tensor is considered to be a separate instance.
///
/// ## Examples
///
/// ```
Expand Down
8 changes: 8 additions & 0 deletions crates/re_log_types/src/component_types/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,14 @@ impl Pinhole {

/// A transform between two spaces.
///
/// The [`Transform`] component is special, as all instances of
/// an Entity always share the same transform.
/// In other words, a [`Transform`] component acts like a splat
/// (all instances of an entity share the same transform).
/// This is because each entity must have a unique transform chain,
/// e.g. the entity `foo/bar/baz` is has the transform that is the product of
/// `foo.transform * foo/bar.transform * foo/bar/baz.transform`.
///
/// ```
/// use re_log_types::component_types::{Transform, Rigid3, Pinhole};
/// use arrow2_convert::field::ArrowField;
Expand Down
4 changes: 3 additions & 1 deletion crates/re_log_types/src/path/entity_path_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ where

impl std::fmt::Debug for EntityPathImpl {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "EntityPath({self})")
// Show the nicely formatted entity path, but surround with quotes and escape
// troublesome characters such as back-slashes and newlines.
write!(f, "{:?}", self.to_string())
}
}

Expand Down
3 changes: 3 additions & 0 deletions crates/re_query/src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ pub fn get_component_with_instances(
/// available, they are implicitly treated as an integer sequence of the correct
/// length.
///
/// If you expect only one instance (e.g. for mono-components like `Transform` `Tensor`]
/// and have no additional components you can use [`re_data_store::query_latest_single`] instead.
///
/// ```
/// # use re_arrow_store::LatestAtQuery;
/// # use re_log_types::{Timeline, component_types::{Point2D, ColorRGBA}, Component};
Expand Down
4 changes: 3 additions & 1 deletion crates/re_viewer/src/misc/queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ pub fn closest_pinhole_transform(
) -> Option<EntityPath> {
crate::profile_function!();

let data_store = &ctx.log_db.entity_db.data_store;

let mut pinhole_ent_path = None;
let mut cur_path = Some(entity_path.clone());
while let Some(path) = cur_path {
if let Some(Transform::Pinhole(_)) =
query_latest_single::<Transform>(&ctx.log_db.entity_db, &path, query)
query_latest_single::<Transform>(data_store, &path, query)
{
pinhole_ent_path = Some(path);
break;
Expand Down
31 changes: 6 additions & 25 deletions crates/re_viewer/src/misc/space_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ use nohash_hasher::IntSet;

use re_arrow_store::{LatestAtQuery, TimeInt, Timeline};
use re_data_store::{log_db::EntityDb, query_latest_single, EntityPath, EntityTree};
use re_log_types::{Transform, ViewCoordinates};
use re_query::query_entity_with_primary;
use re_log_types::Transform;

use super::UnreachableTransform;

Expand Down Expand Up @@ -112,7 +111,8 @@ impl SpaceInfoCollection {
tree: &EntityTree,
query: &LatestAtQuery,
) {
if let Some(transform) = query_latest_single::<Transform>(entity_db, &tree.path, query)
if let Some(transform) =
query_latest_single::<Transform>(&entity_db.data_store, &tree.path, query)
{
// A set transform (likely non-identity) - create a new space.
parent_space
Expand Down Expand Up @@ -157,7 +157,9 @@ impl SpaceInfoCollection {
let mut spaces_info = Self::default();

// Start at the root. The root is always part of the collection!
if query_latest_single::<Transform>(entity_db, &EntityPath::root(), &query).is_some() {
if query_latest_single::<Transform>(&entity_db.data_store, &EntityPath::root(), &query)
.is_some()
{
re_log::warn_once!("The root entity has a 'transform' component! This will have no effect. Did you mean to apply the transform elsewhere?");
}
let mut root_space_info = SpaceInfo::new(EntityPath::root());
Expand Down Expand Up @@ -268,24 +270,3 @@ impl SpaceInfoCollection {
}

// ----------------------------------------------------------------------------

pub fn query_view_coordinates(
entity_db: &EntityDb,
ent_path: &EntityPath,
query: &LatestAtQuery,
) -> Option<re_log_types::ViewCoordinates> {
let data_store = &entity_db.data_store;

let entity_view =
query_entity_with_primary::<ViewCoordinates>(data_store, query, ent_path, &[]).ok()?;

let mut iter = entity_view.iter_primary().ok()?;

let view_coords = iter.next()?;

if iter.next().is_some() {
re_log::warn_once!("Unexpected batch for ViewCoordinates at: {}", ent_path);
}

view_coords
}
16 changes: 8 additions & 8 deletions crates/re_viewer/src/misc/transform_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl TransformCache {
// Child transforms of this space
transforms.gather_descendants_transforms(
current_tree,
entity_db,
&entity_db.data_store,
&query,
entity_prop_map,
glam::Affine3A::IDENTITY,
Expand All @@ -117,7 +117,7 @@ impl TransformCache {
// Generally, the transform _at_ a node isn't relevant to it's children, but only to get to its parent in turn!
match transform_at(
&current_tree.path,
entity_db,
&entity_db.data_store,
&query,
// TODO(#1988): See comment in transform_at. This is a workaround for precision issues
// and the fact that there is no meaningful image plane distance for 3D->2D views.
Expand All @@ -138,7 +138,7 @@ impl TransformCache {
// (skip over everything at and under `current_tree` automatically)
transforms.gather_descendants_transforms(
parent_tree,
entity_db,
&entity_db.data_store,
&query,
entity_prop_map,
reference_from_ancestor,
Expand All @@ -154,7 +154,7 @@ impl TransformCache {
fn gather_descendants_transforms(
&mut self,
tree: &EntityTree,
entity_db: &EntityDb,
data_store: &re_arrow_store::DataStore,
query: &LatestAtQuery,
entity_properties: &EntityPropertyMap,
reference_from_entity: glam::Affine3A,
Expand All @@ -176,7 +176,7 @@ impl TransformCache {
let mut encountered_pinhole = encountered_pinhole;
let reference_from_child = match transform_at(
&child_tree.path,
entity_db,
data_store,
query,
|p| *entity_properties.get(p).pinhole_image_plane_distance.get(),
&mut encountered_pinhole,
Expand All @@ -191,7 +191,7 @@ impl TransformCache {
};
self.gather_descendants_transforms(
child_tree,
entity_db,
data_store,
query,
entity_properties,
reference_from_child,
Expand Down Expand Up @@ -224,12 +224,12 @@ impl TransformCache {

fn transform_at(
entity_path: &EntityPath,
entity_db: &EntityDb,
data_store: &re_arrow_store::DataStore,
query: &LatestAtQuery,
pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32,
encountered_pinhole: &mut bool,
) -> Result<Option<glam::Affine3A>, UnreachableTransform> {
if let Some(transform) = query_latest_single(entity_db, entity_path, query) {
if let Some(transform) = query_latest_single(data_store, entity_path, query) {
match transform {
re_log_types::Transform::Rigid3(rigid) => Ok(Some(rigid.parent_from_child().into())),
// If we're connected via 'unknown' it's not reachable
Expand Down
5 changes: 3 additions & 2 deletions crates/re_viewer/src/ui/selection_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ fn pinhole_props_ui(
) {
let query = ctx.current_query();
if let Some(re_log_types::Transform::Pinhole(_)) =
query_latest_single::<Transform>(&ctx.log_db.entity_db, entity_path, &query)
query_latest_single::<Transform>(&ctx.log_db.entity_db.data_store, entity_path, &query)
{
ui.label("Image plane distance");
let mut distance = *entity_props.pinhole_image_plane_distance.get();
Expand Down Expand Up @@ -467,7 +467,8 @@ fn depth_props_ui(
crate::profile_function!();

let query = ctx.current_query();
let tensor = query_latest_single::<Tensor>(&ctx.log_db.entity_db, entity_path, &query)?;
let tensor =
query_latest_single::<Tensor>(&ctx.log_db.entity_db.data_store, entity_path, &query)?;
if tensor.meaning != TensorDataMeaning::Depth {
return Some(());
}
Expand Down
Loading