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.

Sign up for GitHub

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

Make sure that the coordinate systems of locally defined curves match #1982

Merged
merged 5 commits into from
Aug 3, 2023
Merged

Make sure that the coordinate systems of locally defined curves match #1982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
071d9f8
Document assumptions of join operation
hannobraun Aug 2, 2023
a03a636
Fix curve coordinate systems
hannobraun Aug 3, 2023
9b14782
Validate that curve coordinate systems match
hannobraun Aug 1, 2023
c9c3f96
Don't do unnecessary work in validation check
hannobraun Aug 3, 2023
f611694
Simplify validation check
hannobraun Aug 3, 2023
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
29 changes: 27 additions & 2 deletions crates/fj-core/src/operations/build/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ use fj_math::Point;
use crate::{
objects::{Face, Shell},
operations::{
update::region::UpdateRegion, BuildFace, Insert, IsInserted,
IsInsertedNo, IsInsertedYes, JoinCycle, Polygon, UpdateFace,
reverse::ReverseCurveCoordinateSystems, update::region::UpdateRegion,
BuildFace, Insert, IsInserted, IsInsertedNo, IsInsertedYes, JoinCycle,
Polygon, UpdateCycle, UpdateFace,
},
services::Services,
};
Expand Down Expand Up @@ -45,6 +46,10 @@ pub trait BuildShell {
region
.update_exterior(|cycle| {
cycle
.update_nth_half_edge(0, |edge| {
edge.reverse_curve_coordinate_systems(services)
.insert(services)
})
.join_to(
abc.face.region().exterior(),
0..=0,
Expand All @@ -59,12 +64,20 @@ pub trait BuildShell {
region
.update_exterior(|cycle| {
cycle
.update_nth_half_edge(1, |edge| {
edge.reverse_curve_coordinate_systems(services)
.insert(services)
})
.join_to(
abc.face.region().exterior(),
1..=1,
2..=2,
services,
)
.update_nth_half_edge(0, |edge| {
edge.reverse_curve_coordinate_systems(services)
.insert(services)
})
.join_to(
bad.face.region().exterior(),
0..=0,
Expand All @@ -79,18 +92,30 @@ pub trait BuildShell {
region
.update_exterior(|cycle| {
cycle
.update_nth_half_edge(0, |edge| {
edge.reverse_curve_coordinate_systems(services)
.insert(services)
})
.join_to(
abc.face.region().exterior(),
0..=0,
1..=1,
services,
)
.update_nth_half_edge(1, |edge| {
edge.reverse_curve_coordinate_systems(services)
.insert(services)
})
.join_to(
bad.face.region().exterior(),
1..=1,
2..=2,
services,
)
.update_nth_half_edge(2, |edge| {
edge.reverse_curve_coordinate_systems(services)
.insert(services)
})
.join_to(
dac.face.region().exterior(),
2..=2,
Expand Down
12 changes: 12 additions & 0 deletions crates/fj-core/src/operations/join/cycle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ pub trait JoinCycle {
///
/// Panics, if the ranges have different lengths.
///
/// # Assumptions
///
/// This method makes some assumptions that need to be met, if the operation
/// is to result in a valid shape:
///
/// - **The joined half-edges must be coincident.**
/// - **The locally defined curve coordinate systems of the edges must
/// match.**
///
/// If either of those assumptions are not met, this will result in a
/// validation error down the line.
///
/// # Implementation Note
///
/// The use of the `RangeInclusive` type might be a bit limiting, as other
Expand Down
174 changes: 162 additions & 12 deletions crates/fj-core/src/validate/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ impl Validate for Shell {
config: &ValidationConfig,
errors: &mut Vec<ValidationError>,
) {
ShellValidationError::validate_curve_coordinates(self, config, errors);
ShellValidationError::validate_edges_coincident(self, config, errors);
ShellValidationError::validate_watertight(self, config, errors);
}
Expand All @@ -25,6 +26,14 @@ impl Validate for Shell {
/// [`Shell`] validation failed
#[derive(Clone, Debug, thiserror::Error)]
pub enum ShellValidationError {
/// [`Shell`] contains curves whose coordinate systems don't match
#[error(
"Curve coordinate system mismatch ({} errors): {:#?}",
.0.len(),
.0
)]
CurveCoordinateSystemMismatch(Vec<CurveCoordinateSystemMismatch>),

/// [`Shell`] contains global_edges not referred to by two half-edges
#[error("Shell is not watertight")]
NotWatertight,
Expand Down Expand Up @@ -66,7 +75,6 @@ pub enum ShellValidationError {
///
/// Returns an [`Iterator`] of the distance at each sample.
fn distances(
config: &ValidationConfig,
edge_a: Handle<HalfEdge>,
surface_a: Handle<Surface>,
edge_b: Handle<HalfEdge>,
Expand All @@ -82,11 +90,6 @@ fn distances(
surface.point_from_surface_coords(surface_coords)
}

// Check whether start positions do not match. If they don't treat second edge as flipped
let flip = sample(0.0, (&edge_a, surface_a.geometry()))
.distance_to(&sample(0.0, (&edge_b, surface_b.geometry())))
> config.identical_max_distance;

// Three samples (start, middle, end), are enough to detect weather lines
// and circles match. If we were to add more complicated curves, this might
// need to change.
Expand All @@ -97,16 +100,105 @@ fn distances(
for i in 0..sample_count {
let percent = i as f64 * step;
let sample1 = sample(percent, (&edge_a, surface_a.geometry()));
let sample2 = sample(
if flip { 1.0 - percent } else { percent },
(&edge_b, surface_b.geometry()),
);
let sample2 = sample(1.0 - percent, (&edge_b, surface_b.geometry()));
distances.push(sample1.distance_to(&sample2))
}
distances.into_iter()
}

impl ShellValidationError {
fn validate_curve_coordinates(
shell: &Shell,
config: &ValidationConfig,
errors: &mut Vec<ValidationError>,
) {
let mut edges_and_surfaces = Vec::new();
shell.all_edges_with_surface(&mut edges_and_surfaces);

for (edge_a, surface_a) in &edges_and_surfaces {
for (edge_b, surface_b) in &edges_and_surfaces {
// We only care about edges referring to the same curve.
if edge_a.curve().id() != edge_b.curve().id() {
continue;
}

// No need to check an edge against itself.
if edge_a.id() == edge_b.id() {
continue;
}

fn compare_curve_coords(
edge_a: &Handle<HalfEdge>,
surface_a: &Handle<Surface>,
edge_b: &Handle<HalfEdge>,
surface_b: &Handle<Surface>,
config: &ValidationConfig,
mismatches: &mut Vec<CurveCoordinateSystemMismatch>,
) {
// Let's check 4 points. Given that the most complex curves
// we have right now are circles, 3 would be enough to check
// for coincidence. But the first and last might be
// identical, so let's add an extra one.
let [a, d] = edge_a.boundary().inner;
let b = a + (d - a) * 1. / 3.;
let c = a + (d - a) * 2. / 3.;

for point_curve in [a, b, c, d] {
let a_surface =
edge_a.path().point_from_path_coords(point_curve);
let b_surface =
edge_b.path().point_from_path_coords(point_curve);

let a_global = surface_a
.geometry()
.point_from_surface_coords(a_surface);
let b_global = surface_b
.geometry()
.point_from_surface_coords(b_surface);

let distance = (a_global - b_global).magnitude();

if distance > config.identical_max_distance {
mismatches.push(CurveCoordinateSystemMismatch {
edge_a: edge_a.clone(),
edge_b: edge_b.clone(),
point_curve,
point_a: a_global,
point_b: b_global,
distance,
});
}
}
}

let mut mismatches = Vec::new();

compare_curve_coords(
edge_a,
surface_a,
edge_b,
surface_b,
config,
&mut mismatches,
);
compare_curve_coords(
edge_b,
surface_b,
edge_a,
surface_a,
config,
&mut mismatches,
);

if !mismatches.is_empty() {
errors.push(
Self::CurveCoordinateSystemMismatch(mismatches).into(),
);
}
}
}
}

fn validate_edges_coincident(
shell: &Shell,
config: &ValidationConfig,
Expand All @@ -120,6 +212,11 @@ impl ShellValidationError {
// data-structure like an octree.
for (edge_a, surface_a) in &edges_and_surfaces {
for (edge_b, surface_b) in &edges_and_surfaces {
// No need to check an edge against itself.
if edge_a.id() == edge_b.id() {
continue;
}

let identical_according_to_global_form =
edge_a.global_form().id() == edge_b.global_form().id();

Expand Down Expand Up @@ -153,7 +250,6 @@ impl ShellValidationError {
// identical_max_distance, so we shouldn't have any
// greater than the max
if distances(
config,
edge_a.clone(),
surface_a.clone(),
edge_b.clone(),
Expand All @@ -176,7 +272,6 @@ impl ShellValidationError {
// If all points on distinct curves are within
// distinct_min_distance, that's a problem.
if distances(
config,
edge_a.clone(),
surface_a.clone(),
edge_b.clone(),
Expand Down Expand Up @@ -249,6 +344,16 @@ impl ShellValidationError {
}
}

#[derive(Clone, Debug)]
pub struct CurveCoordinateSystemMismatch {
pub edge_a: Handle<HalfEdge>,
pub edge_b: Handle<HalfEdge>,
pub point_curve: Point<1>,
pub point_a: Point<3>,
pub point_b: Point<3>,
pub distance: Scalar,
}

#[cfg(test)]
mod tests {
use crate::{
Expand All @@ -262,6 +367,51 @@ mod tests {
validate::{shell::ShellValidationError, Validate, ValidationError},
};

#[test]
fn curve_coordinate_system_mismatch() -> anyhow::Result<()> {
let mut services = Services::new();

let valid = Shell::tetrahedron(
[[0., 0., 0.], [0., 1., 0.], [1., 0., 0.], [0., 0., 1.]],
&mut services,
);
let invalid = valid.shell.replace_face(
&valid.abc.face,
valid
.abc
.face
.update_region(|region| {
region
.update_exterior(|cycle| {
cycle
.update_nth_half_edge(0, |half_edge| {
half_edge
.replace_path(
half_edge.path().reverse(),
)
.replace_boundary(
half_edge.boundary().reverse(),
)
.insert(&mut services)
})
.insert(&mut services)
})
.insert(&mut services)
})
.insert(&mut services),
);

valid.shell.validate_and_return_first_error()?;
assert_contains_err!(
invalid,
ValidationError::Shell(
ShellValidationError::CurveCoordinateSystemMismatch(..)
)
);

Ok(())
}

#[test]
fn coincident_not_identical() -> anyhow::Result<()> {
let mut services = Services::new();
Expand Down