Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle serde-field that fails to deserialize #3130

Merged
merged 8 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -128,6 +128,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 @@ -34,6 +34,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 @@ -70,10 +71,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())
Comment on lines +61 to +64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's only invalid if all are invalid? What if 9/10 are invalid?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought process was that if there are some views that are valid, then we have something to show, so I don't want to just throw it away. The user can always manually click the reset button in that case, or just remove the invalid views to create new ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair - let's add that as a comment. Are the invalid views culled somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no culling yet, though we could add it. At the moment, instead the layout remains unchanged, but the invalid view just shows a warning message. My thought was that once we get to a point where only some views can be invalid, it's better to make the user aware that something went wrong so they can handle it than to just magically cull them behind the scenes.

}

/// 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