From e215d4ea6c8855c64faf04e50aa87fc5dffc442c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 12:39:59 +0100 Subject: [PATCH 1/8] Fix word in documentation --- crates/fj-core/src/operations/replace/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index 5a7f6faf8a..9e60b151bc 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -62,7 +62,7 @@ //! Only a few replace operations are implemented so far. More can be added, as //! the need arises. //! -//! As of this writing, replace operations are generally implemented in a most +//! As of this writing, replace operations are generally implemented in the most //! simple and naive way possible: Iterating over all referenced objects and //! calling the replace operation recursively. This might have performance //! implications for large object graphs. From 6dfd48e5d5a5a079cfc04e8bd70ee55e8909c2af Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 12:40:07 +0100 Subject: [PATCH 2/8] Improve wording in documentation --- crates/fj-core/src/operations/replace/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index 9e60b151bc..a1520abde2 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -70,10 +70,10 @@ //! There are some update operations that are straight-up redundant with what //! replace operations are doing. Some of the methods even have the same names. //! Those haven't been removed yet, as update operations generally require a -//! reference to an object, while replace operations require a `Handle`. There -//! are some open questions about how operations in general should deal with -//! objects being inserted or not, so it's probably not worth to address this -//! before doing a general revamp of how operations deal with inserting. +//! reference to a bare object, while replace operations require a `Handle`. +//! There are some open questions about how operations in general should deal +//! with objects being inserted or not, so it's probably not worth addressing +//! this before doing a general revamp of how operations deal with inserting. //! //! //! [`Handle`]: crate::storage::Handle From 759a21dd230c956c1f010d31522682434471675f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 13:08:42 +0100 Subject: [PATCH 3/8] Add associated types to replace trait Those will be used to adjust the return type, making the replace traits more flexible. --- crates/fj-core/src/operations/replace/curve.rs | 17 +++++++++++++++++ .../fj-core/src/operations/replace/half_edge.rs | 15 +++++++++++++++ crates/fj-core/src/operations/replace/vertex.rs | 17 +++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/crates/fj-core/src/operations/replace/curve.rs b/crates/fj-core/src/operations/replace/curve.rs index 2a058c96a7..b48a52df30 100644 --- a/crates/fj-core/src/operations/replace/curve.rs +++ b/crates/fj-core/src/operations/replace/curve.rs @@ -13,6 +13,9 @@ use super::ReplaceOutput; /// /// [module documentation]: super pub trait ReplaceCurve: Sized { + /// The bare object type that this trait is implemented for + type BareObject; + /// Replace the curve #[must_use] fn replace_curve( @@ -24,6 +27,8 @@ pub trait ReplaceCurve: Sized { } impl ReplaceCurve for Handle { + type BareObject = HalfEdge; + fn replace_curve( self, original: &Handle, @@ -41,6 +46,8 @@ impl ReplaceCurve for Handle { } impl ReplaceCurve for Handle { + type BareObject = Cycle; + fn replace_curve( self, original: &Handle, @@ -69,6 +76,8 @@ impl ReplaceCurve for Handle { } impl ReplaceCurve for Handle { + type BareObject = Region; + fn replace_curve( self, original: &Handle, @@ -107,6 +116,8 @@ impl ReplaceCurve for Handle { } impl ReplaceCurve for Handle { + type BareObject = Sketch; + fn replace_curve( self, original: &Handle, @@ -135,6 +146,8 @@ impl ReplaceCurve for Handle { } impl ReplaceCurve for Handle { + type BareObject = Face; + fn replace_curve( self, original: &Handle, @@ -159,6 +172,8 @@ impl ReplaceCurve for Handle { } impl ReplaceCurve for Handle { + type BareObject = Shell; + fn replace_curve( self, original: &Handle, @@ -187,6 +202,8 @@ impl ReplaceCurve for Handle { } impl ReplaceCurve for Handle { + type BareObject = Solid; + fn replace_curve( self, original: &Handle, diff --git a/crates/fj-core/src/operations/replace/half_edge.rs b/crates/fj-core/src/operations/replace/half_edge.rs index e3f23fad9b..6e0b3832fe 100644 --- a/crates/fj-core/src/operations/replace/half_edge.rs +++ b/crates/fj-core/src/operations/replace/half_edge.rs @@ -13,6 +13,9 @@ use super::ReplaceOutput; /// /// [module documentation]: super pub trait ReplaceHalfEdge: Sized { + /// The bare object type that this trait is implemented for + type BareObject; + /// Replace the half-edge #[must_use] fn replace_half_edge( @@ -24,6 +27,8 @@ pub trait ReplaceHalfEdge: Sized { } impl ReplaceHalfEdge for Handle { + type BareObject = Cycle; + fn replace_half_edge( self, original: &Handle, @@ -41,6 +46,8 @@ impl ReplaceHalfEdge for Handle { } impl ReplaceHalfEdge for Handle { + type BareObject = Region; + fn replace_half_edge( self, original: &Handle, @@ -79,6 +86,8 @@ impl ReplaceHalfEdge for Handle { } impl ReplaceHalfEdge for Handle { + type BareObject = Sketch; + fn replace_half_edge( self, original: &Handle, @@ -107,6 +116,8 @@ impl ReplaceHalfEdge for Handle { } impl ReplaceHalfEdge for Handle { + type BareObject = Face; + fn replace_half_edge( self, original: &Handle, @@ -131,6 +142,8 @@ impl ReplaceHalfEdge for Handle { } impl ReplaceHalfEdge for Handle { + type BareObject = Shell; + fn replace_half_edge( self, original: &Handle, @@ -159,6 +172,8 @@ impl ReplaceHalfEdge for Handle { } impl ReplaceHalfEdge for Handle { + type BareObject = Solid; + fn replace_half_edge( self, original: &Handle, diff --git a/crates/fj-core/src/operations/replace/vertex.rs b/crates/fj-core/src/operations/replace/vertex.rs index bd2ba40128..63bf6c42fb 100644 --- a/crates/fj-core/src/operations/replace/vertex.rs +++ b/crates/fj-core/src/operations/replace/vertex.rs @@ -13,6 +13,9 @@ use super::ReplaceOutput; /// /// [module documentation]: super pub trait ReplaceVertex: Sized { + /// The bare object type that this trait is implemented for + type BareObject; + /// Replace the vertex #[must_use] fn replace_vertex( @@ -24,6 +27,8 @@ pub trait ReplaceVertex: Sized { } impl ReplaceVertex for Handle { + type BareObject = HalfEdge; + fn replace_vertex( self, original: &Handle, @@ -41,6 +46,8 @@ impl ReplaceVertex for Handle { } impl ReplaceVertex for Handle { + type BareObject = Cycle; + fn replace_vertex( self, original: &Handle, @@ -69,6 +76,8 @@ impl ReplaceVertex for Handle { } impl ReplaceVertex for Handle { + type BareObject = Region; + fn replace_vertex( self, original: &Handle, @@ -107,6 +116,8 @@ impl ReplaceVertex for Handle { } impl ReplaceVertex for Handle { + type BareObject = Sketch; + fn replace_vertex( self, original: &Handle, @@ -135,6 +146,8 @@ impl ReplaceVertex for Handle { } impl ReplaceVertex for Handle { + type BareObject = Face; + fn replace_vertex( self, original: &Handle, @@ -159,6 +172,8 @@ impl ReplaceVertex for Handle { } impl ReplaceVertex for Handle { + type BareObject = Shell; + fn replace_vertex( self, original: &Handle, @@ -187,6 +202,8 @@ impl ReplaceVertex for Handle { } impl ReplaceVertex for Handle { + type BareObject = Solid; + fn replace_vertex( self, original: &Handle, From 860bcb278a0606ff99f9eb14f54d22363f4b7dcb Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 13:15:13 +0100 Subject: [PATCH 4/8] Refactor to prepare for follow-on change --- crates/fj-core/src/operations/replace/curve.rs | 16 ++++++++-------- .../fj-core/src/operations/replace/half_edge.rs | 14 +++++++------- crates/fj-core/src/operations/replace/mod.rs | 8 +++++--- crates/fj-core/src/operations/replace/vertex.rs | 16 ++++++++-------- 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/crates/fj-core/src/operations/replace/curve.rs b/crates/fj-core/src/operations/replace/curve.rs index b48a52df30..e2f4c7cb22 100644 --- a/crates/fj-core/src/operations/replace/curve.rs +++ b/crates/fj-core/src/operations/replace/curve.rs @@ -23,7 +23,7 @@ pub trait ReplaceCurve: Sized { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput; + ) -> ReplaceOutput; } impl ReplaceCurve for Handle { @@ -34,7 +34,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { if original.id() == self.curve().id() { ReplaceOutput::Updated( self.update_curve(|_| replacement).insert(services), @@ -53,7 +53,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut half_edges = Vec::new(); @@ -83,7 +83,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let exterior = self.exterior().clone().replace_curve( @@ -123,7 +123,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut regions = Vec::new(); @@ -153,7 +153,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let region = self.region().clone().replace_curve( original, replacement, @@ -179,7 +179,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut faces = Vec::new(); @@ -209,7 +209,7 @@ impl ReplaceCurve for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut shells = Vec::new(); diff --git a/crates/fj-core/src/operations/replace/half_edge.rs b/crates/fj-core/src/operations/replace/half_edge.rs index 6e0b3832fe..b61ba49b44 100644 --- a/crates/fj-core/src/operations/replace/half_edge.rs +++ b/crates/fj-core/src/operations/replace/half_edge.rs @@ -23,7 +23,7 @@ pub trait ReplaceHalfEdge: Sized { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput; + ) -> ReplaceOutput; } impl ReplaceHalfEdge for Handle { @@ -34,7 +34,7 @@ impl ReplaceHalfEdge for Handle { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { if let Some(half_edges) = self.half_edges().replace(original, replacements) { @@ -53,7 +53,7 @@ impl ReplaceHalfEdge for Handle { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let exterior = self.exterior().clone().replace_half_edge( @@ -93,7 +93,7 @@ impl ReplaceHalfEdge for Handle { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut regions = Vec::new(); @@ -123,7 +123,7 @@ impl ReplaceHalfEdge for Handle { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let region = self.region().clone().replace_half_edge( original, replacements, @@ -149,7 +149,7 @@ impl ReplaceHalfEdge for Handle { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut faces = Vec::new(); @@ -179,7 +179,7 @@ impl ReplaceHalfEdge for Handle { original: &Handle, replacements: [Handle; N], services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut shells = Vec::new(); diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index a1520abde2..0a101730db 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -87,6 +87,8 @@ pub use self::{ curve::ReplaceCurve, half_edge::ReplaceHalfEdge, vertex::ReplaceVertex, }; +use crate::storage::Handle; + /// The output of a replace operation /// /// See [module documentation] for more information. @@ -97,13 +99,13 @@ pub enum ReplaceOutput { /// /// If this variant is returned, the object to be replaced was not /// referenced, and no replacement happened. - Original(T), + Original(Handle), /// The updated version of the object that the operation was called on /// /// If this variant is returned, a replacement happened, and this is the new /// version of the object that reflects that. - Updated(T), + Updated(Handle), } impl ReplaceOutput { @@ -113,7 +115,7 @@ impl ReplaceOutput { } /// Convert `self` into a `T`, regardless of variant - pub fn into_inner(self) -> T { + pub fn into_inner(self) -> Handle { match self { ReplaceOutput::Original(inner) => inner, ReplaceOutput::Updated(inner) => inner, diff --git a/crates/fj-core/src/operations/replace/vertex.rs b/crates/fj-core/src/operations/replace/vertex.rs index 63bf6c42fb..cd02dd1e22 100644 --- a/crates/fj-core/src/operations/replace/vertex.rs +++ b/crates/fj-core/src/operations/replace/vertex.rs @@ -23,7 +23,7 @@ pub trait ReplaceVertex: Sized { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput; + ) -> ReplaceOutput; } impl ReplaceVertex for Handle { @@ -34,7 +34,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { if original.id() == self.start_vertex().id() { ReplaceOutput::Updated( self.update_start_vertex(|_| replacement).insert(services), @@ -53,7 +53,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut half_edges = Vec::new(); @@ -83,7 +83,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let exterior = self.exterior().clone().replace_vertex( @@ -123,7 +123,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut regions = Vec::new(); @@ -153,7 +153,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let region = self.region().clone().replace_vertex( original, replacement, @@ -179,7 +179,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut faces = Vec::new(); @@ -209,7 +209,7 @@ impl ReplaceVertex for Handle { original: &Handle, replacement: Handle, services: &mut Services, - ) -> ReplaceOutput { + ) -> ReplaceOutput { let mut replacement_happened = false; let mut shells = Vec::new(); From 5be097d3171538b6e4095043587a8aa8ea83160b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 13:21:52 +0100 Subject: [PATCH 5/8] Add argument to prepare for follow-on change --- .../fj-core/src/operations/replace/curve.rs | 20 +++++++++++-------- .../src/operations/replace/half_edge.rs | 18 ++++++++++------- crates/fj-core/src/operations/replace/mod.rs | 4 ++-- .../fj-core/src/operations/replace/vertex.rs | 20 +++++++++++-------- 4 files changed, 37 insertions(+), 25 deletions(-) diff --git a/crates/fj-core/src/operations/replace/curve.rs b/crates/fj-core/src/operations/replace/curve.rs index e2f4c7cb22..a11f88b2a9 100644 --- a/crates/fj-core/src/operations/replace/curve.rs +++ b/crates/fj-core/src/operations/replace/curve.rs @@ -64,7 +64,7 @@ impl ReplaceCurve for Handle { services, ); replacement_happened |= half_edge.was_updated(); - half_edges.push(half_edge.into_inner()); + half_edges.push(half_edge.into_inner(services)); } if replacement_happened { @@ -101,13 +101,17 @@ impl ReplaceCurve for Handle { services, ); replacement_happened |= cycle.was_updated(); - interiors.push(cycle.into_inner()); + interiors.push(cycle.into_inner(services)); } if replacement_happened { ReplaceOutput::Updated( - Region::new(exterior.into_inner(), interiors, self.color()) - .insert(services), + Region::new( + exterior.into_inner(services), + interiors, + self.color(), + ) + .insert(services), ) } else { ReplaceOutput::Original(self) @@ -134,7 +138,7 @@ impl ReplaceCurve for Handle { services, ); replacement_happened |= region.was_updated(); - regions.push(region.into_inner()); + regions.push(region.into_inner(services)); } if replacement_happened { @@ -162,7 +166,7 @@ impl ReplaceCurve for Handle { if region.was_updated() { ReplaceOutput::Updated( - Face::new(self.surface().clone(), region.into_inner()) + Face::new(self.surface().clone(), region.into_inner(services)) .insert(services), ) } else { @@ -190,7 +194,7 @@ impl ReplaceCurve for Handle { services, ); replacement_happened |= face.was_updated(); - faces.push(face.into_inner()); + faces.push(face.into_inner(services)); } if replacement_happened { @@ -220,7 +224,7 @@ impl ReplaceCurve for Handle { services, ); replacement_happened |= shell.was_updated(); - shells.push(shell.into_inner()); + shells.push(shell.into_inner(services)); } if replacement_happened { diff --git a/crates/fj-core/src/operations/replace/half_edge.rs b/crates/fj-core/src/operations/replace/half_edge.rs index b61ba49b44..7126b66461 100644 --- a/crates/fj-core/src/operations/replace/half_edge.rs +++ b/crates/fj-core/src/operations/replace/half_edge.rs @@ -71,13 +71,17 @@ impl ReplaceHalfEdge for Handle { services, ); replacement_happened |= cycle.was_updated(); - interiors.push(cycle.into_inner()); + interiors.push(cycle.into_inner(services)); } if replacement_happened { ReplaceOutput::Updated( - Region::new(exterior.into_inner(), interiors, self.color()) - .insert(services), + Region::new( + exterior.into_inner(services), + interiors, + self.color(), + ) + .insert(services), ) } else { ReplaceOutput::Original(self) @@ -104,7 +108,7 @@ impl ReplaceHalfEdge for Handle { services, ); replacement_happened |= region.was_updated(); - regions.push(region.into_inner()); + regions.push(region.into_inner(services)); } if replacement_happened { @@ -132,7 +136,7 @@ impl ReplaceHalfEdge for Handle { if region.was_updated() { ReplaceOutput::Updated( - Face::new(self.surface().clone(), region.into_inner()) + Face::new(self.surface().clone(), region.into_inner(services)) .insert(services), ) } else { @@ -160,7 +164,7 @@ impl ReplaceHalfEdge for Handle { services, ); replacement_happened |= face.was_updated(); - faces.push(face.into_inner()); + faces.push(face.into_inner(services)); } if replacement_happened { @@ -190,7 +194,7 @@ impl ReplaceHalfEdge for Handle { services, ); replacement_happened |= shell.was_updated(); - shells.push(shell.into_inner()); + shells.push(shell.into_inner(services)); } if replacement_happened { diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index 0a101730db..2f60e1adb8 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -87,7 +87,7 @@ pub use self::{ curve::ReplaceCurve, half_edge::ReplaceHalfEdge, vertex::ReplaceVertex, }; -use crate::storage::Handle; +use crate::{services::Services, storage::Handle}; /// The output of a replace operation /// @@ -115,7 +115,7 @@ impl ReplaceOutput { } /// Convert `self` into a `T`, regardless of variant - pub fn into_inner(self) -> Handle { + pub fn into_inner(self, _: &mut Services) -> Handle { match self { ReplaceOutput::Original(inner) => inner, ReplaceOutput::Updated(inner) => inner, diff --git a/crates/fj-core/src/operations/replace/vertex.rs b/crates/fj-core/src/operations/replace/vertex.rs index cd02dd1e22..86d3adec96 100644 --- a/crates/fj-core/src/operations/replace/vertex.rs +++ b/crates/fj-core/src/operations/replace/vertex.rs @@ -64,7 +64,7 @@ impl ReplaceVertex for Handle { services, ); replacement_happened |= half_edge.was_updated(); - half_edges.push(half_edge.into_inner()); + half_edges.push(half_edge.into_inner(services)); } if replacement_happened { @@ -101,13 +101,17 @@ impl ReplaceVertex for Handle { services, ); replacement_happened |= cycle.was_updated(); - interiors.push(cycle.into_inner()); + interiors.push(cycle.into_inner(services)); } if replacement_happened { ReplaceOutput::Updated( - Region::new(exterior.into_inner(), interiors, self.color()) - .insert(services), + Region::new( + exterior.into_inner(services), + interiors, + self.color(), + ) + .insert(services), ) } else { ReplaceOutput::Original(self) @@ -134,7 +138,7 @@ impl ReplaceVertex for Handle { services, ); replacement_happened |= region.was_updated(); - regions.push(region.into_inner()); + regions.push(region.into_inner(services)); } if replacement_happened { @@ -162,7 +166,7 @@ impl ReplaceVertex for Handle { if region.was_updated() { ReplaceOutput::Updated( - Face::new(self.surface().clone(), region.into_inner()) + Face::new(self.surface().clone(), region.into_inner(services)) .insert(services), ) } else { @@ -190,7 +194,7 @@ impl ReplaceVertex for Handle { services, ); replacement_happened |= face.was_updated(); - faces.push(face.into_inner()); + faces.push(face.into_inner(services)); } if replacement_happened { @@ -220,7 +224,7 @@ impl ReplaceVertex for Handle { services, ); replacement_happened |= shell.was_updated(); - shells.push(shell.into_inner()); + shells.push(shell.into_inner(services)); } if replacement_happened { From 2c0e2c24c841b1e260cc2ca874d94b3139ffe7ce Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 13:23:46 +0100 Subject: [PATCH 6/8] Add trait bound to prepare for follow-on change --- crates/fj-core/src/operations/replace/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index 2f60e1adb8..f0ebdb54c8 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -89,6 +89,8 @@ pub use self::{ use crate::{services::Services, storage::Handle}; +use super::insert::Insert; + /// The output of a replace operation /// /// See [module documentation] for more information. @@ -115,7 +117,10 @@ impl ReplaceOutput { } /// Convert `self` into a `T`, regardless of variant - pub fn into_inner(self, _: &mut Services) -> Handle { + pub fn into_inner(self, _: &mut Services) -> Handle + where + T: Insert>, + { match self { ReplaceOutput::Original(inner) => inner, ReplaceOutput::Updated(inner) => inner, From b452a6ba2c62e09e5b5ad8ae21fc524aa52030c2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 13:25:38 +0100 Subject: [PATCH 7/8] Don't insert updated replacement result This makes replace operations more flexible, as it gives control over insertion to the caller. This is especially relevant, if the replacement produces an invalid intermediate step. Currently, everything that is inserted is validated, and validation provides no good mechanism to deal with invalid intermediate objects, even if the final object ends up being valid. --- .../fj-core/src/operations/replace/curve.rs | 37 ++++++++----------- .../src/operations/replace/half_edge.rs | 32 +++++++--------- crates/fj-core/src/operations/replace/mod.rs | 6 +-- .../fj-core/src/operations/replace/vertex.rs | 37 ++++++++----------- 4 files changed, 49 insertions(+), 63 deletions(-) diff --git a/crates/fj-core/src/operations/replace/curve.rs b/crates/fj-core/src/operations/replace/curve.rs index a11f88b2a9..13f8a491b7 100644 --- a/crates/fj-core/src/operations/replace/curve.rs +++ b/crates/fj-core/src/operations/replace/curve.rs @@ -1,6 +1,6 @@ use crate::{ objects::{Curve, Cycle, Face, HalfEdge, Region, Shell, Sketch, Solid}, - operations::{insert::Insert, update::UpdateHalfEdge}, + operations::update::UpdateHalfEdge, services::Services, storage::Handle, }; @@ -33,12 +33,10 @@ impl ReplaceCurve for Handle { self, original: &Handle, replacement: Handle, - services: &mut Services, + _: &mut Services, ) -> ReplaceOutput { if original.id() == self.curve().id() { - ReplaceOutput::Updated( - self.update_curve(|_| replacement).insert(services), - ) + ReplaceOutput::Updated(self.update_curve(|_| replacement)) } else { ReplaceOutput::Original(self) } @@ -68,7 +66,7 @@ impl ReplaceCurve for Handle { } if replacement_happened { - ReplaceOutput::Updated(Cycle::new(half_edges).insert(services)) + ReplaceOutput::Updated(Cycle::new(half_edges)) } else { ReplaceOutput::Original(self) } @@ -105,14 +103,11 @@ impl ReplaceCurve for Handle { } if replacement_happened { - ReplaceOutput::Updated( - Region::new( - exterior.into_inner(services), - interiors, - self.color(), - ) - .insert(services), - ) + ReplaceOutput::Updated(Region::new( + exterior.into_inner(services), + interiors, + self.color(), + )) } else { ReplaceOutput::Original(self) } @@ -142,7 +137,7 @@ impl ReplaceCurve for Handle { } if replacement_happened { - ReplaceOutput::Updated(Sketch::new(regions).insert(services)) + ReplaceOutput::Updated(Sketch::new(regions)) } else { ReplaceOutput::Original(self) } @@ -165,10 +160,10 @@ impl ReplaceCurve for Handle { ); if region.was_updated() { - ReplaceOutput::Updated( - Face::new(self.surface().clone(), region.into_inner(services)) - .insert(services), - ) + ReplaceOutput::Updated(Face::new( + self.surface().clone(), + region.into_inner(services), + )) } else { ReplaceOutput::Original(self) } @@ -198,7 +193,7 @@ impl ReplaceCurve for Handle { } if replacement_happened { - ReplaceOutput::Updated(Shell::new(faces).insert(services)) + ReplaceOutput::Updated(Shell::new(faces)) } else { ReplaceOutput::Original(self) } @@ -228,7 +223,7 @@ impl ReplaceCurve for Handle { } if replacement_happened { - ReplaceOutput::Updated(Solid::new(shells).insert(services)) + ReplaceOutput::Updated(Solid::new(shells)) } else { ReplaceOutput::Original(self) } diff --git a/crates/fj-core/src/operations/replace/half_edge.rs b/crates/fj-core/src/operations/replace/half_edge.rs index 7126b66461..b5c63b6c8f 100644 --- a/crates/fj-core/src/operations/replace/half_edge.rs +++ b/crates/fj-core/src/operations/replace/half_edge.rs @@ -1,6 +1,5 @@ use crate::{ objects::{Cycle, Face, HalfEdge, Region, Shell, Sketch, Solid}, - operations::insert::Insert, services::Services, storage::Handle, }; @@ -33,12 +32,12 @@ impl ReplaceHalfEdge for Handle { self, original: &Handle, replacements: [Handle; N], - services: &mut Services, + _: &mut Services, ) -> ReplaceOutput { if let Some(half_edges) = self.half_edges().replace(original, replacements) { - ReplaceOutput::Updated(Cycle::new(half_edges).insert(services)) + ReplaceOutput::Updated(Cycle::new(half_edges)) } else { ReplaceOutput::Original(self) } @@ -75,14 +74,11 @@ impl ReplaceHalfEdge for Handle { } if replacement_happened { - ReplaceOutput::Updated( - Region::new( - exterior.into_inner(services), - interiors, - self.color(), - ) - .insert(services), - ) + ReplaceOutput::Updated(Region::new( + exterior.into_inner(services), + interiors, + self.color(), + )) } else { ReplaceOutput::Original(self) } @@ -112,7 +108,7 @@ impl ReplaceHalfEdge for Handle { } if replacement_happened { - ReplaceOutput::Updated(Sketch::new(regions).insert(services)) + ReplaceOutput::Updated(Sketch::new(regions)) } else { ReplaceOutput::Original(self) } @@ -135,10 +131,10 @@ impl ReplaceHalfEdge for Handle { ); if region.was_updated() { - ReplaceOutput::Updated( - Face::new(self.surface().clone(), region.into_inner(services)) - .insert(services), - ) + ReplaceOutput::Updated(Face::new( + self.surface().clone(), + region.into_inner(services), + )) } else { ReplaceOutput::Original(self) } @@ -168,7 +164,7 @@ impl ReplaceHalfEdge for Handle { } if replacement_happened { - ReplaceOutput::Updated(Shell::new(faces).insert(services)) + ReplaceOutput::Updated(Shell::new(faces)) } else { ReplaceOutput::Original(self) } @@ -198,7 +194,7 @@ impl ReplaceHalfEdge for Handle { } if replacement_happened { - ReplaceOutput::Updated(Solid::new(shells).insert(services)) + ReplaceOutput::Updated(Solid::new(shells)) } else { ReplaceOutput::Original(self) } diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index f0ebdb54c8..fc7a425365 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -107,7 +107,7 @@ pub enum ReplaceOutput { /// /// If this variant is returned, a replacement happened, and this is the new /// version of the object that reflects that. - Updated(Handle), + Updated(T), } impl ReplaceOutput { @@ -117,13 +117,13 @@ impl ReplaceOutput { } /// Convert `self` into a `T`, regardless of variant - pub fn into_inner(self, _: &mut Services) -> Handle + pub fn into_inner(self, services: &mut Services) -> Handle where T: Insert>, { match self { ReplaceOutput::Original(inner) => inner, - ReplaceOutput::Updated(inner) => inner, + ReplaceOutput::Updated(inner) => inner.insert(services), } } } diff --git a/crates/fj-core/src/operations/replace/vertex.rs b/crates/fj-core/src/operations/replace/vertex.rs index 86d3adec96..8cf982e7dc 100644 --- a/crates/fj-core/src/operations/replace/vertex.rs +++ b/crates/fj-core/src/operations/replace/vertex.rs @@ -1,6 +1,6 @@ use crate::{ objects::{Cycle, Face, HalfEdge, Region, Shell, Sketch, Solid, Vertex}, - operations::{insert::Insert, update::UpdateHalfEdge}, + operations::update::UpdateHalfEdge, services::Services, storage::Handle, }; @@ -33,12 +33,10 @@ impl ReplaceVertex for Handle { self, original: &Handle, replacement: Handle, - services: &mut Services, + _: &mut Services, ) -> ReplaceOutput { if original.id() == self.start_vertex().id() { - ReplaceOutput::Updated( - self.update_start_vertex(|_| replacement).insert(services), - ) + ReplaceOutput::Updated(self.update_start_vertex(|_| replacement)) } else { ReplaceOutput::Original(self) } @@ -68,7 +66,7 @@ impl ReplaceVertex for Handle { } if replacement_happened { - ReplaceOutput::Updated(Cycle::new(half_edges).insert(services)) + ReplaceOutput::Updated(Cycle::new(half_edges)) } else { ReplaceOutput::Original(self) } @@ -105,14 +103,11 @@ impl ReplaceVertex for Handle { } if replacement_happened { - ReplaceOutput::Updated( - Region::new( - exterior.into_inner(services), - interiors, - self.color(), - ) - .insert(services), - ) + ReplaceOutput::Updated(Region::new( + exterior.into_inner(services), + interiors, + self.color(), + )) } else { ReplaceOutput::Original(self) } @@ -142,7 +137,7 @@ impl ReplaceVertex for Handle { } if replacement_happened { - ReplaceOutput::Updated(Sketch::new(regions).insert(services)) + ReplaceOutput::Updated(Sketch::new(regions)) } else { ReplaceOutput::Original(self) } @@ -165,10 +160,10 @@ impl ReplaceVertex for Handle { ); if region.was_updated() { - ReplaceOutput::Updated( - Face::new(self.surface().clone(), region.into_inner(services)) - .insert(services), - ) + ReplaceOutput::Updated(Face::new( + self.surface().clone(), + region.into_inner(services), + )) } else { ReplaceOutput::Original(self) } @@ -198,7 +193,7 @@ impl ReplaceVertex for Handle { } if replacement_happened { - ReplaceOutput::Updated(Shell::new(faces).insert(services)) + ReplaceOutput::Updated(Shell::new(faces)) } else { ReplaceOutput::Original(self) } @@ -228,7 +223,7 @@ impl ReplaceVertex for Handle { } if replacement_happened { - ReplaceOutput::Updated(Solid::new(shells).insert(services)) + ReplaceOutput::Updated(Solid::new(shells)) } else { ReplaceOutput::Original(self) } From 614c74091623ed3b39f182c6554b20ed3317b5ce Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Nov 2023 13:02:57 +0100 Subject: [PATCH 8/8] Update documentation on replace operations --- crates/fj-core/src/operations/replace/mod.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index fc7a425365..8b8e721161 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -24,6 +24,9 @@ //! //! All replace operations follow the same structure: //! +//! - They are implemented for `Handle`, where `T` is a bare object type. +//! - They have an associated type to identify `T`. See implementation note +//! below. //! - They take a reference to the [`Handle`] of the original object that //! should be replaced. //! - Based on the specific replace operations, they take the [`Handle`] of the @@ -75,6 +78,14 @@ //! with objects being inserted or not, so it's probably not worth addressing //! this before doing a general revamp of how operations deal with inserting. //! +//! All replace traits have an associated type. This is required, because the +//! traits are implemented for `Handle`, but they must refer to the bare +//! object type (`T`) in their method return values. It should be possible to +//! avoid this additional complexity by implementing the traits for `T` +//! directly, but then we would need arbitrary self types to keep the current +//! level of convenience: +//! +//! //! //! [`Handle`]: crate::storage::Handle //! [update operations]: crate::operations::update @@ -107,6 +118,13 @@ pub enum ReplaceOutput { /// /// If this variant is returned, a replacement happened, and this is the new /// version of the object that reflects that. + /// + /// This is a bare object, not a `Handle`, to leave the decision of whether + /// to insert the object to the caller. This might be relevant, if the + /// result of the replacement is an invalid intermediate step in the + /// modeling process. The validation infrastructure currently provides no + /// good ways to deal with invalid intermediate results, even if the end + /// result ends up valid. Updated(T), } @@ -116,7 +134,7 @@ impl ReplaceOutput { matches!(self, ReplaceOutput::Updated(_)) } - /// Convert `self` into a `T`, regardless of variant + /// Return the original object, or insert the updated on and return handle pub fn into_inner(self, services: &mut Services) -> Handle where T: Insert>,