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

Shell and Solid validation #1695

Merged
merged 10 commits into from
Mar 21, 2023
Merged
2 changes: 1 addition & 1 deletion crates/fj-kernel/src/algorithms/sweep/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ impl Sweep for (&HalfEdge, &Handle<Vertex>, &Surface, Option<Color>) {
[
Some(edge.global_form().clone()),
Some(edge_up),
Some(edge_down),
None,
Some(edge_down),
],
)
};
Expand Down
4 changes: 3 additions & 1 deletion crates/fj-kernel/src/algorithms/sweep/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use std::collections::BTreeMap;
use fj_math::Vector;

use crate::{
objects::{Objects, Vertex},
objects::{GlobalEdge, Objects, Vertex},
services::Service,
storage::{Handle, ObjectId},
};
Expand Down Expand Up @@ -47,4 +47,6 @@ pub trait Sweep: Sized {
pub struct SweepCache {
/// Cache for global vertices
pub global_vertex: BTreeMap<ObjectId, Handle<Vertex>>,
/// Cache for global edges
pub global_edge: BTreeMap<ObjectId, Handle<GlobalEdge>>,
Comment on lines +50 to +51
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have a more general caching solution. This seems to work for now but IDK if it's ideal.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know, this seems fine to me. If you come up with something better, feel free to suggest it of course, but I think we can keep it as-is.

}
6 changes: 5 additions & 1 deletion crates/fj-kernel/src/algorithms/sweep/vertex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ impl Sweep for Handle<Vertex> {
.clone();

let vertices = [a, b];
let global_edge = GlobalEdge::new().insert(objects);
let global_edge = cache
.global_edge
.entry(self.id())
.or_insert_with(|| GlobalEdge::new().insert(objects))
.clone();
Comment on lines -29 to +33
Copy link
Owner

Choose a reason for hiding this comment

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

I swear, I was looking for exactly this code yesterday. I found the vertex caching code instead, and my brain was like "oh, so we do have the required caching". I'm pretty stupid sometimes 😄

In any case, good find!


// The vertices of the returned `GlobalEdge` are in normalized order,
// which means the order can't be relied upon by the caller. Return the
Expand Down
3 changes: 2 additions & 1 deletion crates/fj-kernel/src/builder/cycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ impl CycleBuilder {
let half_edges = edges
.into_iter()
.circular_tuple_windows()
.map(|((prev, _, _), (_, curve, boundary))| {
.map(|((prev, _, _), (half_edge, curve, boundary))| {
HalfEdgeBuilder::new(curve, boundary)
.with_start_vertex(prev.start_vertex().clone())
.with_global_form(half_edge.global_form().clone())
})
.collect();

Expand Down
2 changes: 1 addition & 1 deletion crates/fj-kernel/src/objects/full/face.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl Face {
self.interiors.iter()
}

/// Access all cycles of the face
/// Access all cycles of the face (both exterior and interior)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the unrelated change to the docs, just wanted to clear it up

Copy link
Owner

Choose a reason for hiding this comment

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

That's fine! In general, I prefer to have unrelated changes in different commits, as I think that makes things a bit clearer overall, but that is a weak preference. We can leave it as-is.

pub fn all_cycles(&self) -> impl Iterator<Item = &Handle<Cycle>> + '_ {
[self.exterior()].into_iter().chain(self.interiors())
}
Expand Down
24 changes: 12 additions & 12 deletions crates/fj-kernel/src/validate/cycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub enum CycleValidationError {
/// The half-edge
half_edges: Box<(HalfEdge, HalfEdge)>,
},
/// [`Cycle`]'s should have at least one `HalfEdge`
#[error("Expected at least one `HalfEdge`\n")]
NotEnoughHalfEdges,
}
Expand Down Expand Up @@ -94,6 +95,7 @@ impl CycleValidationError {
mod tests {

use crate::{
assert_contains_err,
builder::{CycleBuilder, HalfEdgeBuilder},
objects::Cycle,
services::Services,
Expand All @@ -120,21 +122,19 @@ mod tests {
.add_half_edge(second)
.build(&mut services.objects)
};
assert!(matches!(
disconnected.validate_and_return_first_error(),
Err(ValidationError::Cycle(

assert_contains_err!(
disconnected,
ValidationError::Cycle(
CycleValidationError::HalfEdgesDisconnected { .. }
))
));
)
);

let empty = Cycle::new([]);
assert!(matches!(
empty.validate_and_return_first_error(),
Err(ValidationError::Cycle(
CycleValidationError::NotEnoughHalfEdges
))
));

assert_contains_err!(
empty,
ValidationError::Cycle(CycleValidationError::NotEnoughHalfEdges)
);
Ok(())
}
}
11 changes: 6 additions & 5 deletions crates/fj-kernel/src/validate/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ mod tests {
use fj_math::Point;

use crate::{
assert_contains_err,
builder::HalfEdgeBuilder,
objects::HalfEdge,
services::Services,
Expand All @@ -100,12 +101,12 @@ mod tests {
};

valid.validate_and_return_first_error()?;
assert!(matches!(
invalid.validate_and_return_first_error(),
Err(ValidationError::HalfEdge(
assert_contains_err!(
invalid,
ValidationError::HalfEdge(
HalfEdgeValidationError::VerticesAreCoincident { .. }
))
));
)
);

Ok(())
}
Expand Down
11 changes: 6 additions & 5 deletions crates/fj-kernel/src/validate/face.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ impl FaceValidationError {
mod tests {
use crate::{
algorithms::reverse::Reverse,
assert_contains_err,
builder::{CycleBuilder, FaceBuilder},
objects::Face,
services::Services,
Expand Down Expand Up @@ -110,12 +111,12 @@ mod tests {
};

valid.validate_and_return_first_error()?;
assert!(matches!(
invalid.validate_and_return_first_error(),
Err(ValidationError::Face(
assert_contains_err!(
invalid,
ValidationError::Face(
FaceValidationError::InvalidInteriorWinding { .. }
))
));
)
);

Ok(())
}
Expand Down
28 changes: 26 additions & 2 deletions crates/fj-kernel/src/validate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,29 @@ mod solid;
mod surface;
mod vertex;

use self::cycle::CycleValidationError;
pub use self::{edge::HalfEdgeValidationError, face::FaceValidationError};
pub use self::{
cycle::CycleValidationError, edge::HalfEdgeValidationError,
face::FaceValidationError, shell::ShellValidationError,
solid::SolidValidationError,
};

use std::convert::Infallible;

use fj_math::Scalar;

/// Assert that some object has a validation error which matches a specifc pattern.
/// This is preferred to matching on [`Validate::validate_and_return_first_error`], since usually we don't care about the order.
#[macro_export]
macro_rules! assert_contains_err {
($o:tt,$p:pat) => {
assert!({
let mut errors = Vec::new();
$o.validate(&mut errors);
errors.iter().any(|e| matches!(e, $p))
})
};
}

/// Validate an object
///
/// This trait is used automatically when inserting an object into a store.
Expand Down Expand Up @@ -92,6 +108,14 @@ pub enum ValidationError {
/// `Cycle` validation error
#[error("`Cycle` validation error:\n{0}")]
Cycle(#[from] CycleValidationError),

/// `Shell` validation error
#[error("`Shell` validation error:\n{0}")]
Shell(#[from] ShellValidationError),

/// `Solid` validation error
#[error("`Solid` validation error:\n{0}")]
Solid(#[from] SolidValidationError),
}

impl From<Infallible> for ValidationError {
Expand Down
Loading