Stable image order, fixing flickering (#2191)

* deterministic order of depth offset determination

* improved way of grouping images

* memorize pinhole camera to which entities belong during transform tree walk in order to simplify image grouping

* merge sort functions

crates/re_log_types/src/component_types/draw_order.rs

@@ -19,7 +19,7 @@ use crate::Component;
 ///
 /// assert_eq!(DrawOrder::data_type(), DataType::Float32);
 /// ```
-#[derive(Debug, Clone, ArrowField, ArrowSerialize, ArrowDeserialize)]
+#[derive(Debug, Clone, Copy, ArrowField, ArrowSerialize, ArrowDeserialize)]
 #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
 #[arrow_field(transparent)]
 pub struct DrawOrder(pub f32);

crates/re_viewer/src/misc/transform_cache.rs

@@ -2,8 +2,21 @@ use nohash_hasher::IntMap;
 use re_arrow_store::LatestAtQuery;
 use re_data_store::{log_db::EntityDb, EntityPath, EntityPropertyMap, EntityTree};
 use re_log_types::component_types::{DisconnectedSpace, Pinhole, Transform3D};
+use re_log_types::EntityPathHash;
 use re_viewer_context::TimeControl;
 
+#[derive(Clone)]
+struct TransformInfo {
+    /// The transform from the entity to the reference space.
+    pub reference_from_entity: glam::Affine3A,
+
+    /// The pinhole camera ancestor of this entity if any.
+    ///
+    /// None indicates that this entity is under the eye camera with no Pinhole camera in-between.
+    /// Some indicates that the entity is under a pinhole camera at the given entity path that is not at the root of the space view.
+    pub parent_pinhole: Option<EntityPathHash>,
+}
+
 /// Provides transforms from an entity to a chosen reference space for all elements in the scene
 /// for the currently selected time & timeline.
 ///
@@ -17,7 +30,7 @@ pub struct TransformCache {
     reference_path: EntityPath,
 
     /// All reachable entities.
-    reference_from_entity_per_entity: IntMap<EntityPath, glam::Affine3A>,
+    transform_per_entity: IntMap<EntityPath, TransformInfo>,
 
     /// All unreachable descendant paths of `reference_path`.
     unreachable_descendants: Vec<(EntityPath, UnreachableTransform)>,
@@ -74,7 +87,7 @@ impl TransformCache {
 
         let mut transforms = TransformCache {
             reference_path: space_path.clone(),
-            reference_from_entity_per_entity: Default::default(),
+            transform_per_entity: Default::default(),
             unreachable_descendants: Default::default(),
             first_unreachable_parent: None,
         };
@@ -96,11 +109,11 @@ impl TransformCache {
             &query,
             entity_prop_map,
             glam::Affine3A::IDENTITY,
-            false,
+            None, // Ignore potential pinhole camera at the root of the space view, since it regarded as being "above" this root.
         );
 
         // Walk up from the reference to the highest reachable parent.
-        let mut encountered_pinhole = false;
+        let mut encountered_pinhole = None;
         let mut reference_from_ancestor = glam::Affine3A::IDENTITY;
         while let Some(parent_path) = current_tree.path.parent() {
             let Some(parent_tree) = &entity_db.tree.subtree(&parent_path) else {
@@ -157,17 +170,17 @@ impl TransformCache {
         query: &LatestAtQuery,
         entity_properties: &EntityPropertyMap,
         reference_from_entity: glam::Affine3A,
-        encountered_pinhole: bool,
+        encountered_pinhole: Option<EntityPathHash>,
     ) {
-        match self
-            .reference_from_entity_per_entity
-            .entry(tree.path.clone())
-        {
+        match self.transform_per_entity.entry(tree.path.clone()) {
             std::collections::hash_map::Entry::Occupied(_) => {
                 return;
             }
             std::collections::hash_map::Entry::Vacant(e) => {
-                e.insert(reference_from_entity);
+                e.insert(TransformInfo {
+                    reference_from_entity,
+                    parent_pinhole: encountered_pinhole,
+                });
             }
         }
 
@@ -206,10 +219,20 @@ impl TransformCache {
     /// Retrieves the transform of on entity from its local system to the space of the reference.
     ///
     /// Returns None if the path is not reachable.
-    pub fn reference_from_entity(&self, entity_path: &EntityPath) -> Option<macaw::Affine3A> {
-        self.reference_from_entity_per_entity
-            .get(entity_path)
-            .cloned()
+    pub fn reference_from_entity(&self, ent_path: &EntityPath) -> Option<macaw::Affine3A> {
+        self.transform_per_entity
+            .get(ent_path)
+            .map(|i| i.reference_from_entity)
+    }
+
+    /// Retrieves the ancestor (or self) pinhole under which this entity sits.
+    ///
+    /// None indicates either that the entity does not exist in this hierarchy or that this entity is under the eye camera with no Pinhole camera in-between.
+    /// Some indicates that the entity is under a pinhole camera at the given entity path that is not at the root of the space view.
+    pub fn parent_pinhole(&self, ent_path: &EntityPath) -> Option<EntityPathHash> {
+        self.transform_per_entity
+            .get(ent_path)
+            .and_then(|i| i.parent_pinhole)
     }
 
     // This method isn't currently implemented, but we might need it in the future.
@@ -226,14 +249,14 @@ fn transform_at(
     store: &re_arrow_store::DataStore,
     query: &LatestAtQuery,
     pinhole_image_plane_distance: impl Fn(&EntityPath) -> f32,
-    encountered_pinhole: &mut bool,
+    encountered_pinhole: &mut Option<EntityPathHash>,
 ) -> Result<Option<glam::Affine3A>, UnreachableTransform> {
     let pinhole = store.query_latest_component::<Pinhole>(entity_path, query);
     if pinhole.is_some() {
-        if *encountered_pinhole {
+        if encountered_pinhole.is_some() {
             return Err(UnreachableTransform::NestedPinholeCameras);
         } else {
-            *encountered_pinhole = true;
+            *encountered_pinhole = Some(entity_path.hash());
         }
     }
 

crates/re_viewer/src/ui/view_spatial/scene/mod.rs

@@ -44,6 +44,9 @@ pub struct Image {
     /// Textured rectangle for the renderer.
     pub textured_rect: TexturedRect,
 
+    /// Pinhole camera this image is under.
+    pub parent_pinhole: Option<EntityPathHash>,
+
     /// Draw order value used.
     pub draw_order: DrawOrder,
 }

crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs

@@ -1,11 +1,11 @@
+use std::collections::BTreeMap;
+
 use egui::NumExt;
-use glam::Vec3;
-use itertools::Itertools;
 
 use re_data_store::{EntityPath, EntityProperties};
 use re_log_types::{
     component_types::{ColorRGBA, InstanceKey, Pinhole, Tensor, TensorData, TensorDataMeaning},
-    Component, DecodedTensor, DrawOrder,
+    Component, DecodedTensor, DrawOrder, EntityPathHash,
 };
 use re_query::{query_primary_with_history, EntityView, QueryError};
 use re_renderer::{
@@ -89,63 +89,44 @@ fn to_textured_rect(
     }
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+struct ImageGrouping {
+    parent_pinhole: Option<EntityPathHash>,
+    draw_order: DrawOrder,
+}
+
 fn handle_image_layering(scene: &mut SceneSpatial) {
     crate::profile_function!();
 
-    // Handle layered rectangles that are on (roughly) the same plane with the same depth offset and were logged in sequence.
-    // First, group by similar plane.
-    // TODO(andreas): Need planes later for picking as well!
-    let images_grouped_by_plane = {
-        let mut cur_plane = macaw::Plane3::from_normal_dist(Vec3::NAN, std::f32::NAN);
-        let mut rectangle_group: Vec<Image> = Vec::new();
-        scene
-            .primitives
-            .images
-            .drain(..) // We rebuild the list as we might reorder as well!
-            .batching(move |it| {
-                for image in it {
-                    let rect = &image.textured_rect;
-
-                    let prev_plane = cur_plane;
-                    cur_plane = macaw::Plane3::from_normal_point(
-                        rect.extent_u.cross(rect.extent_v).normalize(),
-                        rect.top_left_corner_position,
-                    );
-
-                    fn is_plane_similar(a: macaw::Plane3, b: macaw::Plane3) -> bool {
-                        a.normal.dot(b.normal) > 0.99 && (a.d - b.d).abs() < 0.01
-                    }
-
-                    // Use draw order, not depth offset since depth offset might change when draw order does not.
-                    let has_same_draw_order = rectangle_group
-                        .last()
-                        .map_or(true, |last_image| last_image.draw_order == image.draw_order);
-
-                    // If the planes are similar, add them to the same group, otherwise start a new group.
-                    if has_same_draw_order && is_plane_similar(prev_plane, cur_plane) {
-                        rectangle_group.push(image);
-                    } else {
-                        let previous_group = std::mem::replace(&mut rectangle_group, vec![image]);
-                        return Some(previous_group);
-                    }
-                }
-                if !rectangle_group.is_empty() {
-                    Some(rectangle_group.drain(..).collect())
-                } else {
-                    None
-                }
+    // Rebuild the image list, grouped by "shared plane", identified with camera & draw order.
+    let mut image_groups: BTreeMap<ImageGrouping, Vec<Image>> = BTreeMap::new();
+    for image in scene.primitives.images.drain(..) {
+        image_groups
+            .entry(ImageGrouping {
+                parent_pinhole: image.parent_pinhole,
+                draw_order: image.draw_order,
             })
+            .or_default()
+            .push(image);
     }
-    .collect_vec();
 
-    // Then, for each planar group do resorting and change transparency.
-    for mut grouped_images in images_grouped_by_plane {
+    // Then, for each group do resorting and change transparency.
+    for (_, mut images) in image_groups {
+        // Since we change transparency depending on order and re_renderer doesn't handle transparency
+        // ordering either, we need to ensure that sorting is stable at the very least.
+        // Sorting is done by depth offset, not by draw order which is the same for the entire group.
+        //
         // Class id images should generally come last within the same layer as
         // they typically have large areas being zeroed out (which maps to fully transparent).
-        grouped_images.sort_by_key(|image| image.tensor.meaning == TensorDataMeaning::ClassId);
+        images.sort_by_key(|image| {
+            (
+                image.textured_rect.options.depth_offset,
+                image.tensor.meaning == TensorDataMeaning::ClassId,
+            )
+        });
 
-        let total_num_images = grouped_images.len();
-        for (idx, image) in grouped_images.iter_mut().enumerate() {
+        let total_num_images = images.len();
+        for (idx, image) in images.iter_mut().enumerate() {
             // make top images transparent
             let opacity = if idx == 0 {
                 1.0
@@ -160,7 +141,7 @@ fn handle_image_layering(scene: &mut SceneSpatial) {
                 .multiply(opacity);
         }
 
-        scene.primitives.images.extend(grouped_images);
+        scene.primitives.images.extend(images);
     }
 }
 
@@ -253,6 +234,7 @@ impl ImagesPart {
                     ent_path: ent_path.clone(),
                     tensor,
                     textured_rect,
+                    parent_pinhole: transforms.parent_pinhole(ent_path),
                     draw_order: draw_order.unwrap_or(DrawOrder::DEFAULT_IMAGE),
                 });
             }