From 72ff36bdb5dc517564e85580ac79b3f999b90217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isa=C3=AFe?= Date: Tue, 3 Dec 2024 09:32:55 +0100 Subject: [PATCH] refactor!(core): update `AttributeUpdate` methods returns (#237) * update AttributeUpdate sigs & default impl * update merge impls * add split fallback method * fix errors * fix doc --- honeycomb-core/src/attributes/collections.rs | 55 ++++++--------- honeycomb-core/src/attributes/tests.rs | 14 ++-- honeycomb-core/src/attributes/traits.rs | 73 +++++++++++++++----- honeycomb-core/src/cmap/error.rs | 2 +- honeycomb-core/src/prelude.rs | 6 +- honeycomb-kernels/src/grisubal/model.rs | 5 +- 6 files changed, 92 insertions(+), 63 deletions(-) diff --git a/honeycomb-core/src/attributes/collections.rs b/honeycomb-core/src/attributes/collections.rs index d16d5830..36843c22 100644 --- a/honeycomb-core/src/attributes/collections.rs +++ b/honeycomb-core/src/attributes/collections.rs @@ -6,10 +6,7 @@ // ------ IMPORTS use super::{AttributeBind, AttributeStorage, AttributeUpdate, UnknownAttributeStorage}; -use crate::{ - cmap::{CMapError, CMapResult}, - prelude::DartIdType, -}; +use crate::{cmap::CMapResult, prelude::DartIdType}; use num_traits::ToPrimitive; use stm::{atomically, StmResult, TVar, Transaction}; @@ -90,17 +87,17 @@ impl UnknownAttributeStorage for AttrSparseV self.data[lhs_inp as usize].read(trans)?, self.data[rhs_inp as usize].read(trans)?, ) { - (Some(v1), Some(v2)) => Some(AttributeUpdate::merge(v1, v2)), - (Some(v), None) | (None, Some(v)) => Some(AttributeUpdate::merge_incomplete(v)), + (Some(v1), Some(v2)) => Ok(AttributeUpdate::merge(v1, v2)), + (Some(v), None) | (None, Some(v)) => AttributeUpdate::merge_incomplete(v), (None, None) => AttributeUpdate::merge_from_none(), }; - if new_v.is_none() { + if new_v.is_err() { eprintln!("W: cannot merge two null attribute value"); eprintln!(" setting new target value to `None`"); } self.data[rhs_inp as usize].write(trans, None)?; self.data[lhs_inp as usize].write(trans, None)?; - self.data[out as usize].write(trans, new_v)?; + self.data[out as usize].write(trans, new_v.ok())?; Ok(()) } @@ -115,25 +112,13 @@ impl UnknownAttributeStorage for AttrSparseV self.data[lhs_inp as usize].read(trans)?, self.data[rhs_inp as usize].read(trans)?, ) { - (Some(v1), Some(v2)) => Some(AttributeUpdate::merge(v1, v2)), - (Some(_), None) | (None, Some(_)) => { - return Err(CMapError::FailedAttributeMerge( - "missing one value for merge", - )) - } - (None, None) => { - return Err(CMapError::FailedAttributeMerge( - "missing both values for merge", - )) - } + (Some(v1), Some(v2)) => AttributeUpdate::merge(v1, v2), + (Some(v), None) | (None, Some(v)) => AttributeUpdate::merge_incomplete(v)?, + (None, None) => AttributeUpdate::merge_from_none()?, }; - if new_v.is_none() { - eprintln!("W: cannot merge two null attribute value"); - eprintln!(" setting new target value to `None`"); - } self.data[rhs_inp as usize].write(trans, None)?; self.data[lhs_inp as usize].write(trans, None)?; - self.data[out as usize].write(trans, new_v)?; + self.data[out as usize].write(trans, Some(new_v))?; Ok(()) } @@ -144,8 +129,12 @@ impl UnknownAttributeStorage for AttrSparseV rhs_out: DartIdType, inp: DartIdType, ) -> StmResult<()> { - if let Some(val) = self.data[inp as usize].read(trans)? { - let (lhs_val, rhs_val) = AttributeUpdate::split(val); + let res = if let Some(val) = self.data[inp as usize].read(trans)? { + Ok(AttributeUpdate::split(val)) + } else { + AttributeUpdate::split_from_none() + }; + if let Ok((lhs_val, rhs_val)) = res { self.data[inp as usize].write(trans, None)?; self.data[lhs_out as usize].write(trans, Some(lhs_val))?; self.data[rhs_out as usize].write(trans, Some(rhs_val))?; @@ -165,14 +154,14 @@ impl UnknownAttributeStorage for AttrSparseV rhs_out: DartIdType, inp: DartIdType, ) -> CMapResult<()> { - if let Some(val) = self.data[inp as usize].read(trans)? { - let (lhs_val, rhs_val) = AttributeUpdate::split(val); - self.data[inp as usize].write(trans, None)?; - self.data[lhs_out as usize].write(trans, Some(lhs_val))?; - self.data[rhs_out as usize].write(trans, Some(rhs_val))?; + let (lhs_val, rhs_val) = if let Some(val) = self.data[inp as usize].read(trans)? { + AttributeUpdate::split(val) } else { - return Err(CMapError::FailedAttributeSplit("no value to split from")); - } + AttributeUpdate::split_from_none()? + }; + self.data[inp as usize].write(trans, None)?; + self.data[lhs_out as usize].write(trans, Some(lhs_val))?; + self.data[rhs_out as usize].write(trans, Some(rhs_val))?; Ok(()) } } diff --git a/honeycomb-core/src/attributes/tests.rs b/honeycomb-core/src/attributes/tests.rs index 6ab44fdd..52e4c3c8 100644 --- a/honeycomb-core/src/attributes/tests.rs +++ b/honeycomb-core/src/attributes/tests.rs @@ -8,7 +8,7 @@ use super::{ UnknownAttributeStorage, }; use crate::{ - cmap::EdgeIdType, + cmap::{CMapResult, EdgeIdType}, prelude::{CMap2, CMapBuilder, FaceIdType, OrbitPolicy, Vertex2, VertexIdType}, }; use std::any::Any; @@ -35,12 +35,12 @@ impl AttributeUpdate for Temperature { (attr, attr) } - fn merge_incomplete(attr: Self) -> Self { - Temperature::from(attr.val / 2.0) + fn merge_incomplete(attr: Self) -> CMapResult { + Ok(Temperature::from(attr.val / 2.0)) } - fn merge_from_none() -> Option { - Some(Temperature::from(0.0)) + fn merge_from_none() -> CMapResult { + Ok(Temperature::from(0.0)) } } @@ -449,9 +449,9 @@ fn attribute_update() { assert_eq!(Temperature::split(t_new), (t_ref, t_ref)); // or Temperature::_ assert_eq!( Temperature::merge_incomplete(t_ref), - Temperature::from(t_ref.val / 2.0) + Ok(Temperature::from(t_ref.val / 2.0)) ); - assert_eq!(Temperature::merge_from_none(), Some(Temperature::from(0.0))); + assert_eq!(Temperature::merge_from_none(), Ok(Temperature::from(0.0))); } #[test] diff --git a/honeycomb-core/src/attributes/traits.rs b/honeycomb-core/src/attributes/traits.rs index ead9ba67..b2fc7794 100644 --- a/honeycomb-core/src/attributes/traits.rs +++ b/honeycomb-core/src/attributes/traits.rs @@ -6,11 +6,11 @@ // ------ IMPORTS use crate::{ - cmap::CMapResult, + cmap::{CMapError, CMapResult}, prelude::{DartIdType, OrbitPolicy}, }; use downcast_rs::{impl_downcast, Downcast}; -use std::any::Any; +use std::any::{type_name, Any}; use std::fmt::Debug; use stm::{atomically, StmResult, Transaction}; @@ -27,7 +27,7 @@ use stm::{atomically, StmResult, Transaction}; /// like this: /// /// ```rust -/// use honeycomb_core::prelude::AttributeUpdate; +/// use honeycomb_core::prelude::{AttributeUpdate, CMapResult}; /// /// #[derive(Clone, Copy, Debug, PartialEq)] /// pub struct Temperature { @@ -43,12 +43,12 @@ use stm::{atomically, StmResult, Transaction}; /// (attr, attr) /// } /// -/// fn merge_incomplete(attr: Self) -> Self { -/// Temperature { val: attr.val / 2.0 } +/// fn merge_incomplete(attr: Self) -> CMapResult { +/// Ok(Temperature { val: attr.val / 2.0 }) /// } /// -/// fn merge_from_none() -> Option { -/// Some(Temperature { val: 0.0 }) +/// fn merge_from_none() -> CMapResult { +/// Ok(Temperature { val: 0.0 }) /// } /// } /// @@ -70,18 +70,57 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// Fallback merging routine, i.e. how to obtain the new attribute value from a single existing /// value. /// + /// The returned value directly affects the behavior of [`UnknownAttributeStorage::merge`], + /// [`UnknownAttributeStorage::try_merge`], therefore of sewing methods too. + /// + /// For example, if this method returns an error for a given attribute, the `try_merge` method + /// will fail. This allow the user to define some attributes as essential (fail if the merge + /// isn't done properly from two values) and other as mores flexible (can fallback to a default + /// value). + /// + /// # Errors + /// /// The default implementation simply returns the passed value. - fn merge_incomplete(attr: Self) -> Self { - attr + fn merge_incomplete(attr: Self) -> CMapResult { + Ok(attr) } /// Fallback merging routine, i.e. how to obtain the new attribute value from no existing /// value. /// - /// The default implementation return `None`. + /// The returned value directly affects the behavior of [`UnknownAttributeStorage::merge`], + /// [`UnknownAttributeStorage::try_merge`], therefore of sewing methods too. + /// + /// For example, if this method returns an error for a given attribute, the `try_merge` method + /// will fail. This allow the user to define some attributes as essential (fail if the merge + /// isn't done properly from two values) and others as more flexible (can fallback to a default + /// value). + /// + /// # Errors + /// + /// The default implementation return `Err(CMapError::FailedAttributeMerge)`. + #[allow(clippy::must_use_candidate)] + fn merge_from_none() -> CMapResult { + Err(CMapError::FailedAttributeMerge(type_name::())) + } + + /// Fallback splitting routine, i.e. how to obtain the new attribute value from no existing + /// value. + /// + /// The returned value directly affects the behavior of [`UnknownAttributeStorage::split`], + /// [`UnknownAttributeStorage::try_split`], therefore of sewing methods too. + /// + /// For example, if this method returns an error for a given attribute, the `try_split` method + /// will fail. This allow the user to define some attributes as essential (fail if the split + /// isn't done properly from a value) and others as more flexible (can fallback to a default + /// value). + /// + /// # Errors + /// + /// The default implementation return `Err(CMapError::FailedAttributeSplit)`. #[allow(clippy::must_use_candidate)] - fn merge_from_none() -> Option { - None + fn split_from_none() -> CMapResult<(Self, Self)> { + Err(CMapError::FailedAttributeSplit(type_name::())) } } @@ -96,7 +135,7 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// to faces if we're modeling a 2D mesh: /// /// ```rust -/// use honeycomb_core::prelude::{AttributeBind, AttributeUpdate, FaceIdType, OrbitPolicy}; +/// use honeycomb_core::prelude::{AttributeBind, AttributeUpdate, CMapResult, FaceIdType, OrbitPolicy}; /// use honeycomb_core::attributes::AttrSparseVec; /// /// #[derive(Clone, Copy, Debug, PartialEq)] @@ -112,12 +151,12 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// # (attr, attr) /// # } /// # -/// # fn merge_incomplete(attr: Self) -> Self { -/// # Temperature { val: attr.val / 2.0 } +/// # fn merge_incomplete(attr: Self) -> CMapResult { +/// # Ok(Temperature { val: attr.val / 2.0 }) /// # } /// # -/// # fn merge_from_none() -> Option { -/// # Some(Temperature { val: 0.0 }) +/// # fn merge_from_none() -> CMapResult { +/// # Ok(Temperature { val: 0.0 }) /// # } /// # } /// diff --git a/honeycomb-core/src/cmap/error.rs b/honeycomb-core/src/cmap/error.rs index 3bdcf34c..f94d4633 100644 --- a/honeycomb-core/src/cmap/error.rs +++ b/honeycomb-core/src/cmap/error.rs @@ -6,7 +6,7 @@ use stm::StmError; pub type CMapResult = Result; /// `CMap` error enum. -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, PartialEq, Eq)] pub enum CMapError { /// STM transaction failed. #[error("transaction failed")] diff --git a/honeycomb-core/src/prelude.rs b/honeycomb-core/src/prelude.rs index f5075d87..92d470bf 100644 --- a/honeycomb-core/src/prelude.rs +++ b/honeycomb-core/src/prelude.rs @@ -2,9 +2,9 @@ pub use crate::attributes::{AttributeBind, AttributeUpdate}; pub use crate::cmap::{ - BuilderError, CMap2, CMapBuilder, DartIdType, EdgeIdType, FaceIdType, Orbit2, OrbitPolicy, - VertexIdType, VolumeIdType, NULL_DART_ID, NULL_EDGE_ID, NULL_FACE_ID, NULL_VERTEX_ID, - NULL_VOLUME_ID, + BuilderError, CMap2, CMapBuilder, CMapResult, DartIdType, EdgeIdType, FaceIdType, Orbit2, + OrbitPolicy, VertexIdType, VolumeIdType, NULL_DART_ID, NULL_EDGE_ID, NULL_FACE_ID, + NULL_VERTEX_ID, NULL_VOLUME_ID, }; pub use crate::geometry::{CoordsError, CoordsFloat, Vector2, Vertex2}; diff --git a/honeycomb-kernels/src/grisubal/model.rs b/honeycomb-kernels/src/grisubal/model.rs index 9b79a21e..25f3f5ff 100644 --- a/honeycomb-kernels/src/grisubal/model.rs +++ b/honeycomb-kernels/src/grisubal/model.rs @@ -8,6 +8,7 @@ use crate::grisubal::GrisubalError; use honeycomb_core::attributes::AttrSparseVec; +use honeycomb_core::cmap::CMapResult; use honeycomb_core::prelude::{ AttributeBind, AttributeUpdate, CoordsFloat, DartIdType, OrbitPolicy, Vertex2, }; @@ -263,8 +264,8 @@ impl AttributeUpdate for Boundary { unreachable!() } - fn merge_from_none() -> Option { - Some(Boundary::None) + fn merge_from_none() -> CMapResult { + Ok(Boundary::None) } }