From a22d6e1369b48ab56d9905d0c935e1d8b2358306 Mon Sep 17 00:00:00 2001 From: imrn99 <95699343+imrn99@users.noreply.github.com> Date: Mon, 25 Nov 2024 08:26:28 +0100 Subject: [PATCH 1/5] update AttributeUpdate sigs & default impl --- honeycomb-core/src/attributes/traits.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/honeycomb-core/src/attributes/traits.rs b/honeycomb-core/src/attributes/traits.rs index ead9ba67d..52a70165e 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}; @@ -71,8 +71,8 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// value. /// /// 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 @@ -80,8 +80,8 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// /// The default implementation return `None`. #[allow(clippy::must_use_candidate)] - fn merge_from_none() -> Option { - None + fn merge_from_none() -> CMapResult { + Err(CMapError::FailedAttributeMerge(type_name::())) } } From bb81d72c5f26455c1e6806a75242d436c8b08a57 Mon Sep 17 00:00:00 2001 From: imrn99 <95699343+imrn99@users.noreply.github.com> Date: Mon, 25 Nov 2024 08:39:31 +0100 Subject: [PATCH 2/5] update merge impls --- honeycomb-core/src/attributes/collections.rs | 28 ++++++-------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/honeycomb-core/src/attributes/collections.rs b/honeycomb-core/src/attributes/collections.rs index d16d58307..283140b2f 100644 --- a/honeycomb-core/src/attributes/collections.rs +++ b/honeycomb-core/src/attributes/collections.rs @@ -90,17 +90,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 +115,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(()) } From 9d8cc5aa4250cd75865299caea5910baf68bccea Mon Sep 17 00:00:00 2001 From: imrn99 <95699343+imrn99@users.noreply.github.com> Date: Mon, 25 Nov 2024 08:52:37 +0100 Subject: [PATCH 3/5] add split fallback method --- honeycomb-core/src/attributes/collections.rs | 27 ++++++++++---------- honeycomb-core/src/attributes/traits.rs | 11 +++++++- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/honeycomb-core/src/attributes/collections.rs b/honeycomb-core/src/attributes/collections.rs index 283140b2f..36843c222 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}; @@ -132,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))?; @@ -153,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/traits.rs b/honeycomb-core/src/attributes/traits.rs index 52a70165e..b1ec42326 100644 --- a/honeycomb-core/src/attributes/traits.rs +++ b/honeycomb-core/src/attributes/traits.rs @@ -78,11 +78,20 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// Fallback merging routine, i.e. how to obtain the new attribute value from no existing /// value. /// - /// The default implementation return `None`. + /// 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 default implementation return `Err(CMapError::FailedAttributeSplit)`. + #[allow(clippy::must_use_candidate)] + fn split_from_none() -> CMapResult<(Self, Self)> { + Err(CMapError::FailedAttributeSplit(type_name::())) + } } /// Generic attribute trait for support description From d3ca95c5bba60e08becb954229ab1318429b6658 Mon Sep 17 00:00:00 2001 From: imrn99 <95699343+imrn99@users.noreply.github.com> Date: Mon, 25 Nov 2024 09:00:26 +0100 Subject: [PATCH 4/5] fix errors --- honeycomb-core/src/attributes/tests.rs | 14 +++++++------- honeycomb-core/src/attributes/traits.rs | 20 ++++++++++---------- honeycomb-core/src/cmap/error.rs | 2 +- honeycomb-core/src/prelude.rs | 6 +++--- honeycomb-kernels/src/grisubal/model.rs | 5 +++-- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/honeycomb-core/src/attributes/tests.rs b/honeycomb-core/src/attributes/tests.rs index 6ab44fddf..52e4c3c8f 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 b1ec42326..f513ca714 100644 --- a/honeycomb-core/src/attributes/traits.rs +++ b/honeycomb-core/src/attributes/traits.rs @@ -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 }) /// } /// } /// @@ -105,7 +105,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)] @@ -121,12 +121,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 3bdcf34c3..f94d46338 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 f5075d873..92d470bfc 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 9b79a21e8..25f3f5ff5 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) } } From e6f3563d69e48631035bcf02456a24030140a767 Mon Sep 17 00:00:00 2001 From: imrn99 <95699343+imrn99@users.noreply.github.com> Date: Mon, 25 Nov 2024 09:27:48 +0100 Subject: [PATCH 5/5] fix doc --- honeycomb-core/src/attributes/traits.rs | 30 +++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/honeycomb-core/src/attributes/traits.rs b/honeycomb-core/src/attributes/traits.rs index f513ca714..b2fc77947 100644 --- a/honeycomb-core/src/attributes/traits.rs +++ b/honeycomb-core/src/attributes/traits.rs @@ -70,6 +70,16 @@ 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) -> CMapResult { Ok(attr) @@ -78,6 +88,16 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// Fallback merging routine, i.e. how to obtain the new attribute value from no 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 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 { @@ -87,6 +107,16 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy { /// 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 split_from_none() -> CMapResult<(Self, Self)> {