Improve the orbit eye to always maintain an up-axis

clippy.toml

@@ -41,6 +41,8 @@ disallowed-macros = [
 # https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
 disallowed-methods = [
   { path = "egui_extras::TableBody::row", reason = "`row` doesn't scale. Use `rows` instead." },
+  { path = "glam::Vec2::normalize", reason = "normalize() can create NaNs. Use try_normalize or normalize_or_zero" },
+  { path = "glam::Vec3::normalize", reason = "normalize() can create NaNs. Use try_normalize or normalize_or_zero" },
   { path = "sha1::Digest::new", reason = "SHA1 is cryptographically broken" },
   { path = "std::env::temp_dir", reason = "Use the tempdir crate instead" },
   { path = "std::panic::catch_unwind", reason = "We compile with `panic = 'abort'`" },

crates/re_space_view_spatial/src/eye.rs

@@ -1,5 +1,4 @@
 use egui::{lerp, NumExt as _, Rect};
-use glam::Affine3A;
 use macaw::{vec3, IsoTransform, Mat4, Quat, Vec3};
 
 use re_space_view::controls::{
@@ -96,7 +95,7 @@ impl Eye {
             let ray_dir = self
                 .world_from_rub_view
                 .transform_vector3(glam::vec3(px, py, -1.0));
-            macaw::Ray3::from_origin_dir(self.pos_in_world(), ray_dir.normalize())
+            macaw::Ray3::from_origin_dir(self.pos_in_world(), ray_dir.normalize_or_zero())
         } else {
             // The ray originates on the camera plane, not from the camera position
             let ray_dir = self.world_from_rub_view.rotation().mul_vec3(glam::Vec3::Z);
@@ -159,23 +158,37 @@ pub struct OrbitEye {
     pub world_from_view_rot: Quat,
     pub fov_y: f32,
 
-    /// Zero = no up (3dof rotation)
-    pub up: Vec3,
+    /// The up-axis of the eye itself, in world-space.
+    ///
+    /// Initially, the up-axis of the eye will be the same as the up-axis of the scene (or +Z if
+    /// the scene has no up axis defined).
+    /// Rolling the camera (e.g. middle-click) will permanently modify the eye's up axis, until the
+    /// next reset.
+    ///
+    /// A value of `Vec3::ZERO` is valid and will result in 3 degrees of freedom, although we never
+    /// use it at the moment.
+    pub eye_up: Vec3,
 
     /// For controlling the eye with WSAD in a smooth way.
     pub velocity: Vec3,
 }
 
 impl OrbitEye {
-    const MAX_PITCH: f32 = 0.999 * 0.25 * std::f32::consts::TAU;
-
-    pub fn new(orbit_center: Vec3, orbit_radius: f32, world_from_view_rot: Quat, up: Vec3) -> Self {
+    /// Avoids zentith/nadir singularity.
+    const MAX_PITCH: f32 = 0.99 * 0.25 * std::f32::consts::TAU;
+
+    pub fn new(
+        orbit_center: Vec3,
+        orbit_radius: f32,
+        world_from_view_rot: Quat,
+        eye_up: Vec3,
+    ) -> Self {
         OrbitEye {
             orbit_center,
             orbit_radius,
             world_from_view_rot,
             fov_y: Eye::DEFAULT_FOV_Y,
-            up,
+            eye_up,
             velocity: Vec3::ZERO,
         }
     }
@@ -206,6 +219,7 @@ impl OrbitEye {
         self.world_from_view_rot = eye.world_from_rub_view.rotation();
         self.fov_y = eye.fov_y.unwrap_or(Eye::DEFAULT_FOV_Y);
         self.velocity = Vec3::ZERO;
+        self.eye_up = eye.world_from_rub_view.rotation() * glam::Vec3::Y;
     }
 
     pub fn lerp(&self, other: &Self, t: f32) -> Self {
@@ -219,54 +233,28 @@ impl OrbitEye {
                 orbit_radius: lerp(self.orbit_radius..=other.orbit_radius, t),
                 world_from_view_rot: self.world_from_view_rot.slerp(other.world_from_view_rot, t),
                 fov_y: egui::lerp(self.fov_y..=other.fov_y, t),
-                up: self.up.lerp(other.up, t).normalize_or_zero(),
+                // A slerp would technically be nicer for eye_up, but it only really
+                // matters if the user starts interacting half-way through the lerp,
+                // and even then it's not a big deal.
+                eye_up: self.eye_up.lerp(other.eye_up, t).normalize_or_zero(),
                 velocity: self.velocity.lerp(other.velocity, t),
             }
         }
     }
 
-    /// Direction we are looking at
+    /// World-direction we are looking at
     fn fwd(&self) -> Vec3 {
-        self.world_from_view_rot * -Vec3::Z
+        self.world_from_view_rot * -Vec3::Z // view-coordinates are RUB
     }
 
     /// Only valid if we have an up vector.
     ///
     /// `[-tau/4, +tau/4]`
     fn pitch(&self) -> Option<f32> {
-        if self.up == Vec3::ZERO {
+        if self.eye_up == Vec3::ZERO {
             None
         } else {
-            Some(self.fwd().dot(self.up).clamp(-1.0, 1.0).asin())
-        }
-    }
-
-    fn set_fwd(&mut self, fwd: Vec3) {
-        if let Some(pitch) = self.pitch() {
-            let pitch = pitch.clamp(-Self::MAX_PITCH, Self::MAX_PITCH);
-
-            let fwd = project_onto(fwd, self.up).normalize(); // Remove pitch
-            let right = fwd.cross(self.up).normalize();
-            let fwd = Quat::from_axis_angle(right, pitch) * fwd; // Tilt up/down
-            let fwd = fwd.normalize(); // Prevent drift
-
-            let world_from_view_rot =
-                Quat::from_affine3(&Affine3A::look_at_rh(Vec3::ZERO, fwd, self.up).inverse());
-
-            if world_from_view_rot.is_finite() {
-                self.world_from_view_rot = world_from_view_rot;
-            }
-        } else {
-            self.world_from_view_rot = Quat::from_rotation_arc(-Vec3::Z, fwd);
-        }
-    }
-
-    #[allow(unused)]
-    pub fn set_up(&mut self, up: Vec3) {
-        self.up = up.normalize_or_zero();
-
-        if self.up != Vec3::ZERO {
-            self.set_fwd(self.fwd()); // this will clamp the rotation
+            Some(self.fwd().dot(self.eye_up).clamp(-1.0, 1.0).asin())
         }
     }
 
@@ -278,12 +266,12 @@ impl OrbitEye {
         let mut did_interact = response.drag_delta().length() > 0.0;
 
         if response.drag_delta().length() > drag_threshold {
-            if response.dragged_by(ROLL_MOUSE)
+            let roll = response.dragged_by(ROLL_MOUSE)
                 || (response.dragged_by(ROLL_MOUSE_ALT)
                     && response
                         .ctx
-                        .input(|i| i.modifiers.contains(ROLL_MOUSE_MODIFIER)))
-            {
+                        .input(|i| i.modifiers.contains(ROLL_MOUSE_MODIFIER)));
+            if roll {
                 if let Some(pointer_pos) = response.ctx.pointer_latest_pos() {
                     self.roll(&response.rect, pointer_pos, response.drag_delta());
                 }
@@ -388,31 +376,26 @@ impl OrbitEye {
         let sensitivity = 0.004; // radians-per-point. TODO(emilk): take fov_y and canvas size into account
         let delta = sensitivity * delta;
 
-        if self.up == Vec3::ZERO {
-            // 3-dof rotation
-            let rot_delta = Quat::from_rotation_y(-delta.x) * Quat::from_rotation_x(-delta.y);
-            self.world_from_view_rot *= rot_delta;
-        } else {
+        if let Some(old_pitch) = self.pitch() {
             // 2-dof rotation
-            let fwd = Quat::from_axis_angle(self.up, -delta.x) * self.fwd();
-            let fwd = fwd.normalize(); // Prevent drift
 
-            let pitch = self.pitch().unwrap() - delta.y;
-            let pitch = pitch.clamp(-Self::MAX_PITCH, Self::MAX_PITCH);
+            // Apply change in heading:
+            self.world_from_view_rot =
+                Quat::from_axis_angle(self.eye_up, -delta.x) * self.world_from_view_rot;
 
-            let fwd = project_onto(fwd, self.up).normalize(); // Remove pitch
-            let right = fwd.cross(self.up).normalize();
-            let fwd = Quat::from_axis_angle(right, pitch) * fwd; // Tilt up/down
-            let fwd = fwd.normalize(); // Prevent drift
+            // We need to clamp pitch to avoid nadir/zenith singularity:
+            let new_pitch = (old_pitch - delta.y).clamp(-Self::MAX_PITCH, Self::MAX_PITCH);
+            let pitch_delta = new_pitch - old_pitch;
 
-            let new_world_from_view_rot =
-                Quat::from_affine3(&Affine3A::look_at_rh(Vec3::ZERO, fwd, self.up).inverse());
+            // Apply change in pitch:
+            self.world_from_view_rot *= Quat::from_rotation_x(pitch_delta);
 
-            if new_world_from_view_rot.is_finite() {
-                self.world_from_view_rot = new_world_from_view_rot;
-            } else {
-                re_log::debug_once!("Failed to rotate camera: got non-finites");
-            }
+            // Avoid numeric drift:
+            self.world_from_view_rot = self.world_from_view_rot.normalize();
+        } else {
+            // no up-axis -> no pitch -> 3-dof rotation
+            let rot_delta = Quat::from_rotation_y(-delta.x) * Quat::from_rotation_x(-delta.y);
+            self.world_from_view_rot *= rot_delta;
         }
     }
 
@@ -422,9 +405,14 @@ impl OrbitEye {
         let rel = pointer_pos - rect.center();
         let delta_angle = delta.rot90().dot(rel) / rel.length_sq();
         let rot_delta = Quat::from_rotation_z(delta_angle);
+
+        // Permanently change our up-axis, at least until the user resets the view:
+        let up_in_view = self.world_from_view_rot.inverse() * self.eye_up;
+
         self.world_from_view_rot *= rot_delta;
 
-        self.up = Vec3::ZERO; // forget about this until user resets the eye
+        // Permanently change our up-axis, at least until the user resets the view:
+        self.eye_up = self.world_from_view_rot * up_in_view;
     }
 
     /// Translate based on a certain number of pixel delta.
@@ -439,8 +427,3 @@ impl OrbitEye {
         self.orbit_center += translate;
     }
 }
-
-/// e.g. up is `[0,0,1]`, we return things like `[x,y,0]`
-fn project_onto(v: Vec3, up: Vec3) -> Vec3 {
-    v - up * v.dot(up)
-}

crates/re_space_view_spatial/src/picking.rs

@@ -247,10 +247,11 @@ fn picking_textured_rects(context: &PickingContext, images: &[ViewerImage]) -> V
 
     for image in images {
         let rect = &image.textured_rect;
-        let rect_plane = macaw::Plane3::from_normal_point(
-            rect.extent_u.cross(rect.extent_v).normalize(),
-            rect.top_left_corner_position,
-        );
+        let Some(normal) = rect.extent_u.cross(rect.extent_v).try_normalize() else {
+            continue; // extent_u and extent_v are parallel. Shouldn't happen.
+        };
+
+        let rect_plane = macaw::Plane3::from_normal_point(normal, rect.top_left_corner_position);
 
         // TODO(andreas): Interaction radius is currently ignored for rects.
         let (intersect, t) =

crates/re_space_view_spatial/src/ui.rs

@@ -7,7 +7,7 @@ use re_entity_db::EntityPath;
 use re_format::format_f32;
 use re_renderer::OutlineConfig;
 use re_space_view::ScreenshotMode;
-use re_types::components::{DepthMeter, InstanceKey, TensorData};
+use re_types::components::{DepthMeter, InstanceKey, TensorData, ViewCoordinates};
 use re_types::tensor_data::TensorDataMeaning;
 use re_viewer_context::{
     HoverHighlight, Item, SelectedSpaceContext, SelectionHighlight, SpaceViewHighlights,
@@ -97,10 +97,10 @@ impl SpatialSpaceViewState {
     ) {
         let re_ui = ctx.re_ui;
 
-        let view_coordinates = ctx
+        let scene_view_coordinates = ctx
             .entity_db
             .store()
-            .query_latest_component(space_origin, &ctx.current_query())
+            .query_latest_component::<ViewCoordinates>(space_origin, &ctx.current_query())
             .map(|c| c.value);
 
         ctx.re_ui.selection_grid(ui, "spatial_settings_ui")
@@ -145,7 +145,7 @@ impl SpatialSpaceViewState {
                         .clicked()
                     {
                         self.bounding_boxes.accumulated = self.bounding_boxes.current;
-                        self.state_3d.reset_camera(&self.bounding_boxes.accumulated, &view_coordinates);
+                        self.state_3d.reset_camera(&self.bounding_boxes.accumulated, scene_view_coordinates);
                     }
                     let mut spin = self.state_3d.spin();
                     if re_ui.checkbox(ui, &mut spin, "Spin")
@@ -160,19 +160,22 @@ impl SpatialSpaceViewState {
                 ctx.re_ui.grid_left_hand_label(ui, "Coordinates")
                     .on_hover_text("The world coordinate system used for this view");
                 ui.vertical(|ui|{
-                    let up_description = if let Some(up) = view_coordinates.and_then(|v| v.up()) {
-                        format!("Up is {up}")
+                    // TODO(#3816): We should also display the current eye's up axis.
+                    let up_description = if let Some(scene_up) = scene_view_coordinates.and_then(|vc| vc.up()) {
+                        format!("Scene up is {scene_up}")
                     } else {
-                        "Up is unspecified".to_owned()
+                        "Scene up is unspecified".to_owned()
                     };
                     ui.label(up_description).on_hover_ui(|ui| {
-                        ui.horizontal(|ui| {
-                            ui.spacing_mut().item_spacing.x = 0.0;
-                            ui.label("Set with ");
-                            ui.code("rerun.log_view_coordinates");
-                            ui.label(".");
-                        });
+                        re_ui::markdown_ui(ui, egui::Id::new("view_coordinates_tooltip"), "Set with `rerun.ViewCoordinates`.");
                     });
+
+                    if let Some(eye) = &self.state_3d.orbit_eye {
+                        if eye.eye_up != glam::Vec3::ZERO {
+                            ui.label(format!("Current camera-eye up-axis is {}", format_vector(eye.eye_up)));
+                        }
+                    }
+
                     re_ui.checkbox(ui, &mut self.state_3d.show_axes, "Show origin axes").on_hover_text("Show X-Y-Z axes");
                     re_ui.checkbox(ui, &mut self.state_3d.show_bbox, "Show bounding box").on_hover_text("Show the current scene bounding box");
                     re_ui.checkbox(ui, &mut self.state_3d.show_accumulated_bbox, "Show accumulated bounding box").on_hover_text("Show bounding box accumulated over all rendered frames");
@@ -769,3 +772,23 @@ fn hit_ui(ui: &mut egui::Ui, hit: &crate::picking::PickingRayHit) {
         ui.label(format!("Hover position: [{x:.5}, {y:.5}, {z:.5}]"));
     }
 }
+
+fn format_vector(v: glam::Vec3) -> String {
+    use glam::Vec3;
+
+    if v == Vec3::X {
+        "+X".to_owned()
+    } else if v == -Vec3::X {
+        "-X".to_owned()
+    } else if v == Vec3::Y {
+        "+Y".to_owned()
+    } else if v == -Vec3::Y {
+        "-Y".to_owned()
+    } else if v == Vec3::Z {
+        "+Z".to_owned()
+    } else if v == -Vec3::Z {
+        "-Z".to_owned()
+    } else {
+        format!("[{:.02}, {:.02}, {:.02}]", v.x, v.y, v.z)
+    }
+}