From c7252ba1d45200c98c872a6bab4032a89906c971 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 28 Apr 2023 10:21:45 +0200 Subject: [PATCH] Suggest users open an issue on crash, and other fixes (#1993) * Print link to github issues on crash * Add `rerun --version` output to bug report issue template * Hide `venv` folder from vscode * Add instruction for how to do non-uniform plot zoom * Warn about how the native_viewer feature takes a lot of compilation time * fix typo * Remove some unwraps * Experiment with enabling the clippy::unwrap_used lint, but no --- .github/ISSUE_TEMPLATE/bug_report.md | 3 ++ .vscode/settings.json | 4 +- Cranky.toml | 4 ++ clippy.toml | 1 + crates/re_build_build_info/src/lib.rs | 2 + crates/re_build_web_viewer/src/lib.rs | 2 + crates/re_renderer/build.rs | 2 + crates/re_tensor_ops/tests/tensor_tests.rs | 2 + crates/re_ui/src/design_tokens.rs | 2 + crates/re_viewer/src/misc/transform_cache.rs | 5 ++- crates/re_viewer/src/ui/view_spatial/ui_3d.rs | 2 +- .../src/ui/view_time_series/scene.rs | 14 ++++--- .../re_viewer/src/ui/view_time_series/ui.rs | 2 + crates/re_web_viewer_server/build.rs | 2 + crates/rerun/Cargo.toml | 1 + crates/rerun/src/clap.rs | 6 +-- crates/rerun/src/crash_handler.rs | 42 ++++++++++--------- 17 files changed, 63 insertions(+), 33 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index f1dc310cf5d1..af0101704638 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -33,5 +33,8 @@ Steps to reproduce the behavior: **Desktop (please complete the following information):** - OS: +**Rerun version** + + **Additional context** diff --git a/.vscode/settings.json b/.vscode/settings.json index 17f3e3d71095..6a1d745b2645 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -8,10 +8,10 @@ "files.insertFinalNewline": true, "files.trimTrailingWhitespace": true, "files.exclude": { - "env/**": true, - "target/**": true, "target_ra/**": true, "target_wasm/**": true, + "target/**": true, + "venv/**": true, }, "files.autoGuessEncoding": true, "python.formatting.provider": "black", diff --git a/Cranky.toml b/Cranky.toml index 0586b154b6a8..44f7b41f03dd 100644 --- a/Cranky.toml +++ b/Cranky.toml @@ -138,4 +138,8 @@ allow = [ "clippy::missing_errors_doc", "trivial_casts", "unused_qualifications", + + # This would be nice to enable, but we have way too many unwraps in our arrow store 😭 + # Enabling this lint in 2023-04-27 yielded 556 hits. + "clippy::unwrap_used", ] diff --git a/clippy.toml b/clippy.toml index bd8f68dcbf99..dc88f60d9cd0 100644 --- a/clippy.toml +++ b/clippy.toml @@ -2,6 +2,7 @@ msrv = "1.67" +allow-unwrap-in-tests = true # https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros disallowed-macros = [ diff --git a/crates/re_build_build_info/src/lib.rs b/crates/re_build_build_info/src/lib.rs index 3341944f322b..f83309d28365 100644 --- a/crates/re_build_build_info/src/lib.rs +++ b/crates/re_build_build_info/src/lib.rs @@ -1,3 +1,5 @@ +#![allow(clippy::unwrap_used)] + //! This crate is to be used from `build.rs` build scripts. //! //! Use this crate together with the `re_build_info` crate. diff --git a/crates/re_build_web_viewer/src/lib.rs b/crates/re_build_web_viewer/src/lib.rs index c5f1c179f10a..20d8e85d2ae8 100644 --- a/crates/re_build_web_viewer/src/lib.rs +++ b/crates/re_build_web_viewer/src/lib.rs @@ -1,3 +1,5 @@ +#![allow(clippy::unwrap_used)] + //! Build the Rerun web-viewer .wasm and generate the .js bindings for it. use cargo_metadata::camino::Utf8PathBuf; diff --git a/crates/re_renderer/build.rs b/crates/re_renderer/build.rs index f2156c0c332a..c39c73c05f76 100644 --- a/crates/re_renderer/build.rs +++ b/crates/re_renderer/build.rs @@ -12,6 +12,8 @@ // TODO(cmc): this should only run for release builds +#![allow(clippy::unwrap_used)] + use std::path::Path; use walkdir::{DirEntry, WalkDir}; diff --git a/crates/re_tensor_ops/tests/tensor_tests.rs b/crates/re_tensor_ops/tests/tensor_tests.rs index c04cd4ebad98..f85e45d6cc65 100644 --- a/crates/re_tensor_ops/tests/tensor_tests.rs +++ b/crates/re_tensor_ops/tests/tensor_tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::unwrap_used)] + use re_log_types::component_types::{ Tensor, TensorCastError, TensorData, TensorDataMeaning, TensorDimension, TensorId, }; diff --git a/crates/re_ui/src/design_tokens.rs b/crates/re_ui/src/design_tokens.rs index 4a3642695e87..49d616988610 100644 --- a/crates/re_ui/src/design_tokens.rs +++ b/crates/re_ui/src/design_tokens.rs @@ -1,3 +1,5 @@ +#![allow(clippy::unwrap_used)] // fixed json file + use egui::Color32; /// The look and feel of the UI. diff --git a/crates/re_viewer/src/misc/transform_cache.rs b/crates/re_viewer/src/misc/transform_cache.rs index 59290e1e94aa..a909f797b2a4 100644 --- a/crates/re_viewer/src/misc/transform_cache.rs +++ b/crates/re_viewer/src/misc/transform_cache.rs @@ -282,7 +282,8 @@ fn inverse_transform_at( *encountered_pinhole = true; // TODO(andreas): If we don't have a resolution we don't know the FOV ergo we don't know how to project. Unclear what to do. - if let Some(resolution) = pinhole.resolution() { + if let (Some(resolution), Some(fov_y)) = (pinhole.resolution(), pinhole.fov_y()) + { let translation = pinhole.principal_point().extend(-100.0); // Large Y offset so this is in front of all 2d that came so far. TODO(andreas): Find better solution Ok(Some( glam::Mat4::from_scale_rotation_translation( @@ -291,7 +292,7 @@ fn inverse_transform_at( glam::Quat::IDENTITY, translation, ) * glam::Mat4::perspective_infinite_lh( - pinhole.fov_y().unwrap(), + fov_y, pinhole.aspect_ratio().unwrap_or(1.0), 0.0, ), diff --git a/crates/re_viewer/src/ui/view_spatial/ui_3d.rs b/crates/re_viewer/src/ui/view_spatial/ui_3d.rs index 25fa82bceaab..8797c73e46ae 100644 --- a/crates/re_viewer/src/ui/view_spatial/ui_3d.rs +++ b/crates/re_viewer/src/ui/view_spatial/ui_3d.rs @@ -327,7 +327,7 @@ pub fn view_3d( view_from_world: eye.world_from_view.inverse(), projection_from_view: Projection::Perspective { - vertical_fov: eye.fov_y.unwrap(), + vertical_fov: eye.fov_y.unwrap_or(Eye::DEFAULT_FOV_Y), near_plane_distance: eye.near(), }, diff --git a/crates/re_viewer/src/ui/view_time_series/scene.rs b/crates/re_viewer/src/ui/view_time_series/scene.rs index c063c5d5ab55..208b3195a94b 100644 --- a/crates/re_viewer/src/ui/view_time_series/scene.rs +++ b/crates/re_viewer/src/ui/view_time_series/scene.rs @@ -75,6 +75,7 @@ impl SceneTimeSeries { self.load_scalars(ctx, query); } + #[inline(never)] // Better callstacks on crashes fn load_scalars(&mut self, ctx: &mut ViewerContext<'_>, query: &SceneQuery<'_>) { crate::profile_function!(); @@ -106,6 +107,8 @@ impl SceneTimeSeries { ); for (time, ent_view) in ent_views { + let Some(time) = time else { continue; }; // scalars cannot be timeless + match ent_view.visit5( |_instance, scalar: component_types::Scalar, @@ -121,7 +124,7 @@ impl SceneTimeSeries { const DEFAULT_RADIUS: f32 = 0.75; points.push(PlotPoint { - time: time.unwrap().as_i64(), // scalars cannot be timeless + time: time.as_i64(), value: scalar.into(), attrs: PlotPointAttrs { label, @@ -148,10 +151,10 @@ impl SceneTimeSeries { // If all points within a line share the label (and it isn't `None`), then we use it // as the whole line label for the plot legend. // Otherwise, we just use the entity path as-is. - let same_label = |points: &[PlotPoint]| { - let label = points[0].attrs.label.as_ref(); - (label.is_some() && points.iter().all(|p| p.attrs.label.as_ref() == label)) - .then(|| label.cloned().unwrap()) + let same_label = |points: &[PlotPoint]| -> Option { + let label = points[0].attrs.label.as_ref()?; + (points.iter().all(|p| p.attrs.label.as_ref() == Some(label))) + .then(|| label.clone()) }; let line_label = same_label(&points).unwrap_or_else(|| entity_path.to_string()); @@ -163,6 +166,7 @@ impl SceneTimeSeries { // segments. // A line segment is a continuous run of points with identical attributes: each time // we notice a change in attributes, we need a new line segment. + #[inline(never)] // Better callstacks on crashes fn add_line_segments(&mut self, line_label: &str, points: Vec) { crate::profile_function!(); diff --git a/crates/re_viewer/src/ui/view_time_series/ui.rs b/crates/re_viewer/src/ui/view_time_series/ui.rs index 68553847af5b..1648fd9f0205 100644 --- a/crates/re_viewer/src/ui/view_time_series/ui.rs +++ b/crates/re_viewer/src/ui/view_time_series/ui.rs @@ -17,6 +17,8 @@ use super::SceneTimeSeries; pub(crate) const HELP_TEXT: &str = "Pan by dragging, or scroll (+ shift = horizontal).\n\ Box zooming: Right click to zoom in and zoom out using a selection.\n\ Zoom with ctrl / ⌘ + pointer wheel, or with pinch gesture.\n\ + You can also zoom by dragging a rectangle with the right mouse button.\n\ + \n\ Reset view with double-click.\n\ Right click to move the time cursor to the current position."; diff --git a/crates/re_web_viewer_server/build.rs b/crates/re_web_viewer_server/build.rs index 79b8fe048136..3837410ac6a2 100644 --- a/crates/re_web_viewer_server/build.rs +++ b/crates/re_web_viewer_server/build.rs @@ -1,3 +1,5 @@ +#![allow(clippy::unwrap_used)] + use std::collections::{HashMap, HashSet}; use cargo_metadata::{CargoOpt, Metadata, MetadataCommand, Package, PackageId}; diff --git a/crates/rerun/Cargo.toml b/crates/rerun/Cargo.toml index 15e31264d47f..2d5a45889fd3 100644 --- a/crates/rerun/Cargo.toml +++ b/crates/rerun/Cargo.toml @@ -40,6 +40,7 @@ global_session = ["re_sdk?/global_session"] image = ["re_log_types/image"] ## Support spawning a native viewer. +## This adds a lot of extra dependencies, so only enable this feature if you need it! native_viewer = ["dep:re_viewer"] ## Support for running a HTTP server that listens to incoming log messages from a Rerun SDK. diff --git a/crates/rerun/src/clap.rs b/crates/rerun/src/clap.rs index fddb1de26aa6..4cd0514d9ce0 100644 --- a/crates/rerun/src/clap.rs +++ b/crates/rerun/src/clap.rs @@ -80,14 +80,12 @@ impl RerunArgs { run: impl FnOnce(Session) + Send + 'static, ) -> anyhow::Result<()> { // Ensure we have a running tokio runtime. - #[allow(unused_assignments)] let mut tokio_runtime = None; let tokio_runtime_handle = if let Ok(handle) = tokio::runtime::Handle::try_current() { handle } else { - tokio_runtime = - Some(tokio::runtime::Runtime::new().expect("Failed to create tokio runtime")); - tokio_runtime.as_ref().unwrap().handle().clone() + let rt = tokio::runtime::Runtime::new().expect("Failed to create tokio runtime"); + tokio_runtime.get_or_insert(rt).handle().clone() }; let _tokio_runtime_guard = tokio_runtime_handle.enter(); diff --git a/crates/rerun/src/crash_handler.rs b/crates/rerun/src/crash_handler.rs index ba1174daac72..157ac6195d83 100644 --- a/crates/rerun/src/crash_handler.rs +++ b/crates/rerun/src/crash_handler.rs @@ -59,7 +59,8 @@ fn install_panic_hook(_build_info: BuildInfo) { eprintln!( "\n\ - Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting" + Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting\n\ + Report bugs: https://github.com/rerun-io/rerun/issues" ); #[cfg(feature = "analytics")] @@ -135,6 +136,17 @@ fn install_signal_handler(build_info: BuildInfo) { } unsafe extern "C" fn signal_handler(signal_number: libc::c_int) { + fn print_problem_and_links(signal_name: &str) { + write_to_stderr("Rerun caught a signal: "); + write_to_stderr(signal_name); + write_to_stderr("\n"); + write_to_stderr( + "Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting\n", + ); + write_to_stderr("Report bugs: https://github.com/rerun-io/rerun/issues\n"); + write_to_stderr("\n"); + } + let signal_name = match signal_number { libc::SIGABRT => "SIGABRT", libc::SIGBUS => "SIGBUS", @@ -150,15 +162,10 @@ fn install_signal_handler(build_info: BuildInfo) { // but writing to stderr is one of them. // So we first print out what happened to stderr so we're sure that gets out, // then we do the unsafe things, like logging the stack trace. - // We take care not to allocate any memory along the way. - - write_to_stderr("\n\n"); - write_to_stderr("Rerun caught a signal: "); - write_to_stderr(signal_name); - write_to_stderr("\n\n"); - write_to_stderr( - "Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting\n\n", - ); + // We take care not to allocate any memory before we generate the call stack. + + write_to_stderr("\n"); + print_problem_and_links(signal_name); // Ok, we printed the most important things. // Let's do less important things that require memory allocations. @@ -167,7 +174,13 @@ fn install_signal_handler(build_info: BuildInfo) { let callstack = callstack(); write_to_stderr(&callstack); + write_to_stderr("\n"); + // Let's print the important stuff _again_ so it is visible at the bottom of the users terminal: + write_to_stderr("\n"); + print_problem_and_links(signal_name); + + // Send analytics - this also sleeps a while to give the analytics time to send the event. #[cfg(feature = "analytics")] { let build_info = BUILD_INFO @@ -176,15 +189,6 @@ fn install_signal_handler(build_info: BuildInfo) { send_signal_analytics(build_info, signal_name, callstack); } - // Let's print the important stuff _again_ so it is visible at the bottom of the users terminal: - write_to_stderr("\n"); - write_to_stderr("Rerun caught a signal: "); - write_to_stderr(signal_name); - write_to_stderr("\n"); - write_to_stderr( - "Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting\n\n", - ); - // We are done! // Call the default signal handler (which usually terminates the app): // SAFETY: we're calling a signal handler