Skip to content

Commit

Permalink
Handle serde-field that fails to deserialize (rerun-io#3130)
Browse files Browse the repository at this point in the history
### What
arrow2-convert forces us into needing to provide data even if serde fail
to deserialize the field. Fall back to default in this case, but specify
invalid.

Additionally, this checks to see if a blueprint is totally invalid and
if so resets it to the default state. Because this can reset the
blueprint immediately upon loading, it was important that this properly
set auto-space-views. Needed to pull in a slightly different version of
rerun-io#3077 that still works with the
now-required usage of Default.

Resolves: rerun-io#3127

### 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 [demo.rerun.io](https://demo.rerun.io/pr/3129) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3129)
- [Docs
preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/8831ec03d85499333648bb15442a39760a2ad41d/examples)
<!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)

---------

Co-authored-by: Andreas Reich <[email protected]>
  • Loading branch information
jleibs and Wumpf authored Aug 28, 2023
1 parent 315f87e commit 9fa097b
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 11 deletions.
11 changes: 7 additions & 4 deletions crates/re_log_types/src/serde_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use arrow2_convert::{deserialize::ArrowDeserialize, field::ArrowField, serialize
/// use arrow2_convert::{ArrowDeserialize, ArrowField, ArrowSerialize, field::ArrowField};
/// use arrow2::datatypes::{DataType, Field};
///
/// #[derive(Clone, Debug, PartialEq, serde::Deserialize, serde::Serialize)]
/// #[derive(Clone, Debug, Default, PartialEq, serde::Deserialize, serde::Serialize)]
/// struct SomeStruct {
/// foo: String,
/// bar: u32,
Expand Down Expand Up @@ -63,14 +63,17 @@ where

impl<T> ArrowDeserialize for SerdeField<T>
where
T: serde::ser::Serialize + serde::de::DeserializeOwned,
T: serde::ser::Serialize + serde::de::DeserializeOwned + Default,
{
type ArrayType = BinaryArray<i32>;

#[inline]
fn arrow_deserialize(v: <&Self::ArrayType as IntoIterator>::Item) -> Option<T> {
re_tracing::profile_function!();
v.and_then(|v| rmp_serde::from_slice::<T>(v).ok())
Some(
v.and_then(|v| rmp_serde::from_slice::<T>(v).ok())
.unwrap_or_default(),
)
}
}

Expand All @@ -79,7 +82,7 @@ mod tests {
use super::SerdeField;
use arrow2_convert::{ArrowDeserialize, ArrowField, ArrowSerialize};

#[derive(Clone, Debug, PartialEq, serde::Deserialize, serde::Serialize)]
#[derive(Clone, Debug, PartialEq, Default, serde::Deserialize, serde::Serialize)]
struct SomeStruct {
foo: String,
bar: u32,
Expand Down
7 changes: 7 additions & 0 deletions crates/re_viewer/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ impl AppState {
// This may update their heuristics, so that all panels that are shown in this frame,
// have the latest information.
let spaces_info = SpaceInfoCollection::new(&ctx.store_db.entity_db);

// If the blueprint is invalid, reset it.
if viewport.blueprint.is_invalid() {
re_log::warn!("Incompatible blueprint detected. Resetting to default.");
viewport.blueprint.reset(&mut ctx, &spaces_info);
}

viewport.on_frame_start(&mut ctx, &spaces_info);

time_panel.show_panel(&mut ctx, ui, app_blueprint.time_panel_expanded);
Expand Down
11 changes: 11 additions & 0 deletions crates/re_viewer_context/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub use command_sender::{
};
pub use component_ui_registry::{ComponentUiRegistry, UiVerbosity};
pub use item::{resolve_mono_instance_path, resolve_mono_instance_path_item, Item, ItemCollection};
use re_log_types::EntityPath;
pub use selection_history::SelectionHistory;
pub use selection_state::{
HoverHighlight, HoveredSpace, InteractionHighlight, SelectionHighlight, SelectionState,
Expand Down Expand Up @@ -69,10 +70,20 @@ pub mod external {
pub struct SpaceViewId(uuid::Uuid);

impl SpaceViewId {
pub fn invalid() -> Self {
Self(uuid::Uuid::nil())
}

pub fn random() -> Self {
Self(uuid::Uuid::new_v4())
}

pub fn from_entity_path(path: &EntityPath) -> Self {
path.last()
.and_then(|last| uuid::Uuid::parse_str(last.to_string().as_str()).ok())
.map_or(Self::invalid(), Self)
}

pub fn hashed_from_str(s: &str) -> Self {
use std::hash::{Hash as _, Hasher as _};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ re_string_interner::declare_new_type!(
pub struct SpaceViewClassName;
);

impl SpaceViewClassName {
pub fn invalid() -> Self {
Self::from("invalid")
}
}

#[derive(Clone, Copy, Debug, Default, PartialEq, PartialOrd, Ord, Eq)]
pub enum SpaceViewClassLayoutPriority {
/// This space view can share space with others
Expand Down
14 changes: 14 additions & 0 deletions crates/re_viewport/src/space_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ pub struct SpaceViewBlueprint {
pub entities_determined_by_user: bool,
}

// Default needed for deserialization when adding/changing fields.
impl Default for SpaceViewBlueprint {
fn default() -> Self {
Self {
id: SpaceViewId::invalid(),
display_name: "invalid".to_owned(),
class_name: SpaceViewClassName::invalid(),
space_origin: EntityPath::root(),
data_blueprint: Default::default(),
entities_determined_by_user: Default::default(),
}
}
}

/// Determine whether this `SpaceViewBlueprint` has user-edits relative to another `SpaceViewBlueprint`
impl SpaceViewBlueprint {
pub fn has_edits(&self, other: &Self) -> bool {
Expand Down
4 changes: 3 additions & 1 deletion crates/re_viewport/src/viewport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,10 @@ impl<'a, 'b> Viewport<'a, 'b> {

let blueprint = load_viewport_blueprint(blueprint_db);

let snapshot = blueprint.clone();

Self {
snapshot: blueprint.clone(),
snapshot,
blueprint,
state,
}
Expand Down
52 changes: 46 additions & 6 deletions crates/re_viewport/src/viewport_blueprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use re_data_store::{EntityPath, StoreDb};
use re_log_types::{DataCell, DataRow, RowId, TimePoint};
use re_types::Loggable as _;
use re_viewer_context::{
CommandSender, Item, SpaceViewId, SystemCommand, SystemCommandSender, ViewerContext,
CommandSender, Item, SpaceViewClassName, SpaceViewId, SystemCommand, SystemCommandSender,
ViewerContext,
};

use crate::{
Expand Down Expand Up @@ -46,8 +47,27 @@ pub struct ViewportBlueprint<'a> {
}

impl<'a> ViewportBlueprint<'a> {
/// Determine whether all views in a blueprint are invalid.
///
/// This most commonly happens due to a change in struct definition that
/// breaks the definition of a serde-field, which means all views will
/// become invalid.
///
/// Note: the invalid check is used to potentially reset the blueprint, so we
/// take the conservative stance that if any view is still usable we will still
/// treat the blueprint as valid and show it.
pub fn is_invalid(&self) -> bool {
!self.space_views.is_empty()
&& self
.space_views
.values()
.all(|sv| sv.class_name() == &SpaceViewClassName::invalid())
}

/// Reset the blueprint to a default state using some heuristics.
pub fn reset(&mut self, ctx: &mut ViewerContext<'_>, spaces_info: &SpaceInfoCollection) {
// TODO(jleibs): When using blueprint API, "reset" should go back to the initially transmitted
// blueprint, not the default blueprint.
re_tracing::profile_function!();

let ViewportBlueprint {
Expand All @@ -59,11 +79,16 @@ impl<'a> ViewportBlueprint<'a> {
auto_space_views,
} = self;

// Note, it's important that these values match the behavior in `load_viewport_blueprint` below.
*space_views = Default::default();
*tree = Default::default();
*maximized = Default::default();
*has_been_user_edited = Default::default();
*auto_space_views = Default::default();
*maximized = None;
*has_been_user_edited = false;
// Only enable auto-space-views if this is the app-default blueprint
*auto_space_views = self
.blueprint_db
.store_info()
.map_or(false, |ri| ri.is_app_default_blueprint());

for space_view in default_created_space_views(ctx, spaces_info) {
self.add_space_view(space_view);
Expand Down Expand Up @@ -272,10 +297,25 @@ pub fn load_space_view_blueprint(
blueprint_db: &re_data_store::StoreDb,
) -> Option<SpaceViewBlueprint> {
re_tracing::profile_function!();
blueprint_db
let mut space_view = blueprint_db
.store()
.query_timeless_component::<SpaceViewComponent>(path)
.map(|c| c.space_view)
.map(|c| c.space_view);

// Blueprint data migrations can leave us unable to parse the expected id from the source-data
// We always want the id to match the one derived from the EntityPath since this id is how
// we would end up removing it from the blueprint.
let expected_id = SpaceViewId::from_entity_path(path);
if let Some(space_view) = &mut space_view {
if space_view.id != SpaceViewId::invalid() && space_view.id != expected_id {
re_log::warn_once!(
"SpaceViewBlueprint id is inconsistent with path: {:?}",
space_view.id
);
}
space_view.id = expected_id;
}
space_view
}

pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> ViewportBlueprint<'_> {
Expand Down

0 comments on commit 9fa097b

Please sign in to comment.