Skip to content

Commit

Permalink
Cleanup of todos referring to closed tickets (#6323)
Browse files Browse the repository at this point in the history
### What

Make `pixi run python ./scripts/zombie_todos.py` happy.

Removed some dead code in the process. Two tickets were opened to
account for stuff that's actually not quite done:
* #6321
* #6320

Commit by commit: I put everything noteworth into its own commit and put
the general less deep cleanup last (still has various judgement calls)

### 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 examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6323?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6323?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)!
* [x] Succeed a main pr check since the pyproject.toml change makes me
nervous

- [PR Build Summary](https://build.rerun.io/pr/6323)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
  • Loading branch information
Wumpf authored May 14, 2024
1 parent 74998a4 commit 3805781
Show file tree
Hide file tree
Showing 29 changed files with 59 additions and 134 deletions.
1 change: 0 additions & 1 deletion crates/re_data_loader/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ impl DataLoaderSettings {
/// [Text files]: crate::SUPPORTED_TEXT_EXTENSIONS
//
// TODO(#4525): `DataLoader`s should support arbitrary URIs
// TODO(#4526): `DataLoader`s should be exposed to the SDKs
// TODO(#4527): Web Viewer `?url` parameter should accept anything our `DataLoader`s support
pub trait DataLoader: Send + Sync {
/// Name of the [`DataLoader`].
Expand Down
3 changes: 2 additions & 1 deletion crates/re_data_ui/src/app_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ impl crate::DataUi for ApplicationId {
.sorted_by_key(|entity_db| entity_db.store_info().map(|info| info.started))
.collect();

//TODO(#6245): we should _not_ use interactive UI in code used for hover tooltip!
// Using the same content ui also for tooltips even if it can't be interacted with.
// (still displays the content we want)
if !recordings.is_empty() {
let content_ui = |ui: &mut egui::Ui| {
ui.spacing_mut().item_spacing.y = 0.0;
Expand Down
3 changes: 2 additions & 1 deletion crates/re_data_ui/src/data_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ impl crate::DataUi for re_smart_channel::SmartChannelSource {
recordings.sort_by_key(|entity_db| entity_db.store_info().map(|info| info.started));
blueprints.sort_by_key(|entity_db| entity_db.store_info().map(|info| info.started));

//TODO(#6245): we should _not_ use interactive UI in code used for hover tooltip!
// Using the same content ui also for tooltips even if it can't be interacted with.
// (still displays the content we want)
let content_ui = |ui: &mut egui::Ui| {
if !recordings.is_empty() {
ui.add_space(8.0);
Expand Down
2 changes: 1 addition & 1 deletion crates/re_entity_db/src/time_histogram_per_timeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ impl TimeHistogramPerTimeline {
.num_static_messages
.checked_sub(n as u64)
.unwrap_or_else(|| {
// TODO(#4355): hitting this on plots demo
// We used to hit this on plots demo, see https://github.com/rerun-io/rerun/issues/4355.
re_log::debug!(
current = self.num_static_messages,
removed = n,
Expand Down
2 changes: 0 additions & 2 deletions crates/re_log_types/src/data_row.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,6 @@ re_types_core::delegate_arrow_tuid!(RowId as "rerun.controls.RowId");
/// ).unwrap();
/// eprintln!("{row}");
/// ```
//
// TODO(#5303): the Layout part will be outdated in the new key-less model
#[derive(Debug, Clone)]
pub struct DataRow {
/// Auto-generated `TUID`, uniquely identifying this event and keeping track of the client's
Expand Down
2 changes: 0 additions & 2 deletions crates/re_log_types/src/data_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,6 @@ re_types_core::delegate_arrow_tuid!(TableId as "rerun.controls.TableId");
/// #
/// # assert_eq!(table_in, table_out);
/// ```
//
// TODO(#5303): the Layout part will be outdated in the new key-less model
#[derive(Debug, Clone, PartialEq)]
pub struct DataTable {
/// Auto-generated `TUID`, uniquely identifying this batch of data and keeping track of the
Expand Down
3 changes: 1 addition & 2 deletions crates/re_log_types/src/time_point/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ impl TimeType {
let time_int = time_int.into();
match time_int {
TimeInt::STATIC => "<static>".into(),
// TODO(#5264): remove time panel hack once we migrate to the new static UI
TimeInt::MIN | TimeInt::MIN_TIME_PANEL => "-∞".into(),
TimeInt::MIN => "-∞".into(),
TimeInt::MAX => "+∞".into(),
_ => match self {
Self::Time => Time::from(time_int).format(time_zone_for_timestamps),
Expand Down
12 changes: 0 additions & 12 deletions crates/re_log_types/src/time_point/time_int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,6 @@ impl std::fmt::Debug for TimeInt {
}

impl TimeInt {
/// Hack.
///
/// Special value used to represent timeless data in the time panel.
///
/// The reason we don't use i64::MIN is because in the time panel we need
/// to be able to pan to before the [`TimeInt::MIN`], and so we need
/// a bit of leeway.
//
// TODO(#5264): remove this once we migrate to the new static UI
#[doc(hidden)]
pub const MIN_TIME_PANEL: Self = Self(NonMinI64::new(i64::MIN / 2));

/// Special value used to represent static data.
///
/// It is illegal to create a [`TimeInt`] with that value in a temporal context.
Expand Down
4 changes: 2 additions & 2 deletions crates/re_query/src/latest_at/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ impl LatestAtComponentResults {
}

PromiseResult::Ready(data) => {
// TODO(#5303): Figure out if/how we'd like to integrate clamping semantics into the
// TODO(#5259): Figure out if/how we'd like to integrate clamping semantics into the
// selection panel.
//
// For now, we simply always clamp, which is the closest to the legacy behavior that the UI
Expand Down Expand Up @@ -216,7 +216,7 @@ impl LatestAtComponentResults {
PromiseResult::Ready(cell) => {
let len = cell.num_instances() as usize;

// TODO(#5303): Figure out if/how we'd like to integrate clamping semantics into the
// TODO(#5259): Figure out if/how we'd like to integrate clamping semantics into the
// selection panel.
//
// For now, we simply always clamp, which is the closest to the legacy behavior that the UI
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/space_view_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ impl SpaceViewClass for SpatialSpaceView3D {
// Note that if the root has `ViewCoordinates`, this will stop the root splitting heuristic
// from splitting the root space into several subspaces.
//
// TODO(andreas)/TODO(#4926):
// TODO(andreas):
// It's tempting to add a visualizer for view coordinates so that it's already picked up via `entities_with_indicator_for_visualizer_kind`.
// Is there a nicer way for this or do we want a visualizer for view coordinates anyways?
// There's also a strong argument to be made that ViewCoordinates implies a 3D space, thus changing the SpacialTopology accordingly!
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/visualizers/assets3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl Asset3DVisualizer {
let picking_instance_hash = re_entity_db::InstancePathHash::entity_all(entity_path);
let outline_mask_ids = ent_context.highlight.index_outline_mask(Instance::ALL);

// TODO(#3232): this is subtly wrong, the key should actually be a hash of everything that got
// TODO(#5974): this is subtly wrong, the key should actually be a hash of everything that got
// cached, which includes the media type…
let mesh = ctx.cache.entry(|c: &mut MeshCache| {
c.entry(
Expand Down
4 changes: 3 additions & 1 deletion crates/re_space_view_time_series/src/space_view_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ It can greatly improve performance (and readability) in such situations as it pr

// Spawn time series data at the root if there's time series data either
// directly at the root or one of its children.
// TODO(#4926): This seems to be unnecessarily complicated.
//
// This is the last hold out of "child of root" spawning, which we removed otherwise
// (see https://github.com/rerun-io/rerun/issues/4926)
let subtree_of_root_entity = &ctx.recording().tree().children;
if indicated_entities.contains(&EntityPath::root())
|| subtree_of_root_entity
Expand Down
55 changes: 6 additions & 49 deletions crates/re_time_panel/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,6 @@ impl TimePanel {
full_y_range,
);
time_selection_ui::loop_selection_ui(
entity_db,
time_ctrl,
&self.time_ranges_ui,
ui,
Expand Down Expand Up @@ -860,7 +859,7 @@ impl TimePanel {
ui,
);

current_time_ui(ctx, entity_db, ui, time_ctrl);
current_time_ui(ctx, ui, time_ctrl);

ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| {
help_button(ui);
Expand All @@ -878,7 +877,7 @@ impl TimePanel {
.timeline_selector_ui(time_ctrl, times_per_timeline, ui);
self.time_control_ui.playback_speed_ui(time_ctrl, ui);
self.time_control_ui.fps_ui(time_ctrl, ui);
current_time_ui(ctx, entity_db, ui, time_ctrl);
current_time_ui(ctx, ui, time_ctrl);

ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| {
help_button(ui);
Expand Down Expand Up @@ -973,7 +972,7 @@ fn collapsed_time_marker_and_time(
}
}

current_time_ui(ctx, entity_db, ui, time_ctrl);
current_time_ui(ctx, ui, time_ctrl);
}

fn paint_range_highlight(
Expand Down Expand Up @@ -1012,52 +1011,10 @@ fn help_button(ui: &mut egui::Ui) {
);
}

/// A user can drag the time slider to between the timeless data and the first real data.
///
/// The time interpolated there is really weird, as it goes from [`TimeInt::MIN_TIME_PANEL`]
/// (which is extremely long time ago) to whatever tim the user logged.
/// So we do not want to display these times to the user.
///
/// This functions returns `true` iff the given time is safe to show.
//
// TODO(#5264): remove time panel hack once we migrate to the new static UI
fn is_time_safe_to_show(
entity_db: &re_entity_db::EntityDb,
timeline: &re_data_store::Timeline,
time: TimeReal,
) -> bool {
if entity_db.num_static_messages() == 0 {
return true; // no timeless messages, no problem
}

if let Some(times) = entity_db.tree().subtree.time_histogram.get(timeline) {
if let Some(first_time) = times.min_key() {
let margin = match timeline.typ() {
re_data_store::TimeType::Time => TimeInt::from_seconds(10_000.try_into().unwrap()),
re_data_store::TimeType::Sequence => {
TimeInt::from_sequence(1_000.try_into().unwrap())
}
};

return TimeInt::new_temporal(first_time) <= time + margin;
}
}

TimeInt::MIN_TIME_PANEL < time
}

fn current_time_ui(
ctx: &ViewerContext<'_>,
entity_db: &re_entity_db::EntityDb,
ui: &mut egui::Ui,
time_ctrl: &TimeControl,
) {
fn current_time_ui(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, time_ctrl: &TimeControl) {
if let Some(time_int) = time_ctrl.time_int() {
let timeline = time_ctrl.timeline();
if is_time_safe_to_show(entity_db, timeline, time_int.into()) {
let time_type = time_ctrl.time_type();
ui.monospace(time_type.format(time_int, ctx.app_options.time_zone));
}
let time_type = time_ctrl.time_type();
ui.monospace(time_type.format(time_int, ctx.app_options.time_zone));
}
}

Expand Down
16 changes: 2 additions & 14 deletions crates/re_time_panel/src/time_selection_ui.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,17 @@
use egui::{CursorIcon, Id, NumExt as _, Rect};

use re_entity_db::EntityDb;
use re_log_types::{Duration, ResolvedTimeRangeF, TimeInt, TimeReal, TimeType};
use re_viewer_context::{Looping, TimeControl};

use super::{is_time_safe_to_show, time_ranges_ui::TimeRangesUi};
use super::time_ranges_ui::TimeRangesUi;

pub fn loop_selection_ui(
entity_db: &EntityDb,
time_ctrl: &mut TimeControl,
time_ranges_ui: &TimeRangesUi,
ui: &egui::Ui,
time_area_painter: &egui::Painter,
timeline_rect: &Rect,
) {
let timeline = *time_ctrl.timeline();

if time_ctrl.loop_selection().is_none() && time_ctrl.looping() == Looping::Selection {
// Helpfully select a time slice
if let Some(selection) = initial_time_selection(time_ranges_ui, time_ctrl.time_type()) {
Expand Down Expand Up @@ -74,11 +70,7 @@ pub fn loop_selection_ui(
time_area_painter.rect_filled(rect, rounding, selection_color);
}

if is_active
&& !selected_range.is_empty()
&& is_time_safe_to_show(entity_db, &timeline, selected_range.min)
&& is_time_safe_to_show(entity_db, &timeline, selected_range.max)
{
if is_active && !selected_range.is_empty() {
paint_range_text(time_ctrl, selected_range, ui, time_area_painter, rect);
}

Expand Down Expand Up @@ -302,10 +294,6 @@ fn paint_range_text(
) {
use egui::{Pos2, Stroke};

if selected_range.min <= TimeInt::MIN_TIME_PANEL {
return; // huge time selection, don't show a confusing times
}

let text_color = ui.visuals().strong_text_color();

let arrow_color = text_color.gamma_multiply(0.75);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ include "rerun/datatypes.fbs";

namespace rerun.components;

// TODO(#3384)
// TODO(#6320)
/*
enum ViewDir: byte {
Up = 1,
Expand Down
6 changes: 2 additions & 4 deletions crates/re_types_builder/src/codegen/rust/deserializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,8 @@ use crate::{
/// The deserializers are designed for maximum performance, assuming the incoming data is correct.
/// If the data is not correct, the deserializers will return an error, but never panic or crash.
///
/// In the future we should add some basic arrow datatype validation during data ingestion,
/// so that changing the datatype between versions of the SDK will produce some helpful warnings
/// instead of just silent bugs.
/// TODO(#5291): add basic arrow datatype validation during data ingestion
/// TODO(#5305): Currently we're doing a lot of checking for exact matches.
/// We should instead assume data is correct and handle errors gracefully.
///
/// ## Understanding datatypes
///
Expand Down
36 changes: 23 additions & 13 deletions crates/re_types_builder/src/objects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ impl Objects {
continue;
}

let resolved_obj = Object::from_raw_object(include_dir_path, &enums, &objs, &obj);
let resolved_obj =
Object::from_raw_object(reporter, include_dir_path, &enums, &objs, &obj);
resolved_objs.insert(resolved_obj.fqname.clone(), resolved_obj);
}

Expand Down Expand Up @@ -331,6 +332,7 @@ impl Object {
/// Resolves a raw [`crate::Object`] into a higher-level representation that can be easily
/// interpreted and manipulated.
pub fn from_raw_object(
reporter: &Reporter,
include_dir_path: impl AsRef<Utf8Path>,
enums: &[FbsEnum<'_>],
objs: &[FbsObject<'_>],
Expand Down Expand Up @@ -369,7 +371,14 @@ impl Object {
.filter(|field| field.type_().base_type() != FbsBaseType::UType)
.filter(|field| field.type_().element() != FbsBaseType::UType)
.map(|field| {
ObjectField::from_raw_object_field(include_dir_path, enums, objs, obj, &field)
ObjectField::from_raw_object_field(
reporter,
include_dir_path,
enums,
objs,
obj,
&field,
)
})
.collect();

Expand Down Expand Up @@ -647,12 +656,6 @@ pub struct ObjectField {
/// Whether the field is nullable.
pub is_nullable: bool,

/// Whether the field is deprecated.
//
// TODO(#2366): do something with this
// TODO(#2367): implement custom attr to specify deprecation reason
pub is_deprecated: bool,

/// The Arrow datatype of this `ObjectField`.
///
/// This is lazily computed when the parent object gets registered into the Arrow registry and
Expand All @@ -662,6 +665,7 @@ pub struct ObjectField {

impl ObjectField {
pub fn from_raw_object_field(
reporter: &Reporter,
include_dir_path: impl AsRef<Utf8Path>,
enums: &[FbsEnum<'_>],
objs: &[FbsObject<'_>],
Expand Down Expand Up @@ -690,7 +694,17 @@ impl ObjectField {
let order = attrs.get::<u32>(&fqname, crate::ATTR_ORDER);

let is_nullable = attrs.has(crate::ATTR_NULLABLE) || typ == Type::Unit; // null type is always nullable
let is_deprecated = field.deprecated();

if field.deprecated() {
reporter.warn(
&virtpath,
&fqname,
format!(
"Use {} attribute for deprecation instead",
crate::ATTR_RERUN_DEPRECATED
),
);
}

Self {
virtpath,
Expand All @@ -703,7 +717,6 @@ impl ObjectField {
attrs,
order,
is_nullable,
is_deprecated,
datatype: None,
}
}
Expand Down Expand Up @@ -745,8 +758,6 @@ impl ObjectField {
attrs.has(crate::ATTR_NULLABLE) || typ == Type::Unit // null type is always nullable
};

let is_deprecated = false;

if attrs.has(crate::ATTR_ORDER) {
reporter.warn(
&virtpath,
Expand All @@ -766,7 +777,6 @@ impl ObjectField {
attrs,
order: 0, // no needed for enums
is_nullable,
is_deprecated,
datatype: None,
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/re_ui/src/list_item2/list_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub struct ShowCollapsingResponse<R> {
///
/// The following features are supported:
/// - Flat or collapsible hierarchical lists.
/// - Full-span background highlighting via [`super::list_item_scope`]. TODO(#6156): fix reference
/// - Full-span background highlighting via [`crate::full_span::full_span_scope`].
/// - Interactive or not.
/// - Support for drag and drop with [`crate::drag_and_drop`].
///
Expand Down
Loading

0 comments on commit 3805781

Please sign in to comment.