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

Simplify local representation of edge vertices #625

Merged
merged 5 commits into from
May 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions crates/fj-kernel/src/algorithms/approx/edges.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use fj_math::Point;

use crate::{geometry, shape::LocalForm, topology::Vertex};

pub fn approximate_edge(
vertices: Option<[LocalForm<Vertex<1>, Vertex<3>>; 2]>,
vertices: Option<[LocalForm<Point<1>, Vertex>; 2]>,
points: &mut Vec<geometry::Point<1, 3>>,
) {
// Insert the exact vertices of this edge into the approximation. This means
Expand All @@ -15,7 +17,7 @@ pub fn approximate_edge(
let vertices = vertices.map(|vertices| {
vertices.map(|vertex| {
geometry::Point::new(
*vertex.local().point.local(),
*vertex.local(),
vertex.canonical().get().point(),
)
})
Expand Down Expand Up @@ -58,8 +60,8 @@ mod test {
let v2 = Vertex::builder(&mut shape).build_from_point(d)?;

let vertices = [
LocalForm::new(v1.get().with_local_form([0.]), v1),
LocalForm::new(v2.get().with_local_form([1.]), v2),
LocalForm::new(Point::from([0.]), v1),
LocalForm::new(Point::from([1.]), v2),
];

let a = geometry::Point::new([0.0], a);
Expand Down
12 changes: 6 additions & 6 deletions crates/fj-kernel/src/shape/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ impl Shape {
/// Access iterator over all vertices
///
/// The caller must not make any assumptions about the order of vertices.
pub fn vertices(&self) -> Iter<Vertex<3>> {
pub fn vertices(&self) -> Iter<Vertex> {
self.stores.vertices.iter()
}

Expand Down Expand Up @@ -293,7 +293,7 @@ mod tests {
assert!(shape.get_handle(&curve.get()).as_ref() == Some(&curve));
assert!(shape.get_handle(&surface.get()).as_ref() == Some(&surface));

let vertex = Vertex::new(point);
let vertex = Vertex { point };
let edge = Edge::new(curve, None);

assert!(shape.get_handle(&vertex).is_none());
Expand Down Expand Up @@ -326,23 +326,23 @@ mod tests {
let mut other = Shape::new();

let point = shape.insert(Point::from([0., 0., 0.]))?;
shape.insert(Vertex::new(point))?;
shape.insert(Vertex { point })?;

// Should fail, as `point` is not part of the shape.
let point = other.insert(Point::from([1., 0., 0.]))?;
let result = shape.insert(Vertex::new(point));
let result = shape.insert(Vertex { point });
assert!(matches!(result, Err(ValidationError::Structural(_))));

// `point` is too close to the original point. `assert!` is commented,
// because that only causes a warning to be logged right now.
let point = shape.insert(Point::from([5e-8, 0., 0.]))?;
let result = shape.insert(Vertex::new(point));
let result = shape.insert(Vertex { point });
assert!(matches!(result, Err(ValidationError::Uniqueness(_))));

// `point` is farther than `MIN_DISTANCE` away from original point.
// Should work.
let point = shape.insert(Point::from([5e-6, 0., 0.]))?;
shape.insert(Vertex::new(point))?;
shape.insert(Vertex { point })?;

Ok(())
}
Expand Down
4 changes: 2 additions & 2 deletions crates/fj-kernel/src/shape/mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub struct Mapping {
pub(super) points: OneMapping<Point<3>>,
pub(super) curves: OneMapping<Curve<3>>,
pub(super) surfaces: OneMapping<Surface>,
pub(super) vertices: OneMapping<Vertex<3>>,
pub(super) vertices: OneMapping<Vertex>,
pub(super) edges: OneMapping<Edge<3>>,
pub(super) cycles: OneMapping<Cycle<3>>,
pub(super) faces: OneMapping<Face>,
Expand Down Expand Up @@ -49,7 +49,7 @@ impl Mapping {
}

/// Access iterator over the mapped vertices
pub fn vertices(&self) -> &OneMapping<Vertex<3>> {
pub fn vertices(&self) -> &OneMapping<Vertex> {
&self.vertices
}

Expand Down
13 changes: 5 additions & 8 deletions crates/fj-kernel/src/shape/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl private::Sealed for Point<3> {}
impl private::Sealed for Curve<3> {}
impl private::Sealed for Surface {}

impl private::Sealed for Vertex<3> {}
impl private::Sealed for Vertex {}
impl private::Sealed for Edge<3> {}
impl private::Sealed for Cycle<3> {}
impl private::Sealed for Face {}
Expand Down Expand Up @@ -82,19 +82,16 @@ impl Object for Surface {
}
}

impl Object for Vertex<3> {
impl Object for Vertex {
fn merge_into(
self,
handle: Option<Handle<Self>>,
shape: &mut Shape,
mapping: &mut Mapping,
) -> ValidationResult<Self> {
let point = self.point().merge_into(
Some(self.point.canonical()),
shape,
mapping,
)?;
let merged = shape.get_handle_or_insert(Vertex::new(point))?;
let point =
self.point().merge_into(Some(self.point), shape, mapping)?;
let merged = shape.get_handle_or_insert(Vertex { point })?;

if let Some(handle) = handle {
mapping.vertices.insert(handle, merged.clone());
Expand Down
2 changes: 1 addition & 1 deletion crates/fj-kernel/src/shape/stores.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub struct Stores {
pub curves: Store<Curve<3>>,
pub surfaces: Store<Surface>,

pub vertices: Store<Vertex<3>>,
pub vertices: Store<Vertex>,
pub edges: Store<Edge<3>>,
pub cycles: Store<Cycle<3>>,
pub faces: Store<Face>,
Expand Down
4 changes: 2 additions & 2 deletions crates/fj-kernel/src/shape/validate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl Validate for Surface {
}
}

impl Validate for Vertex<3> {
impl Validate for Vertex {
/// Validate the vertex
///
/// # Implementation note
Expand Down Expand Up @@ -163,7 +163,7 @@ impl ValidationError {

/// Indicate whether validation found a missing vertex
#[cfg(test)]
pub fn missing_vertex(&self, vertex: &Handle<Vertex<3>>) -> bool {
pub fn missing_vertex(&self, vertex: &Handle<Vertex>) -> bool {
if let Self::Structural(StructuralIssues {
missing_vertices, ..
}) = self
Expand Down
10 changes: 4 additions & 6 deletions crates/fj-kernel/src/shape/validate/structural.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ use crate::{
};

pub fn validate_vertex(
vertex: &Vertex<3>,
vertex: &Vertex,
stores: &Stores,
) -> Result<(), StructuralIssues> {
let point = vertex.point.canonical();

if !stores.points.contains(&point) {
if !stores.points.contains(&vertex.point) {
return Err(StructuralIssues {
missing_point: Some(point),
missing_point: Some(vertex.point.clone()),
..StructuralIssues::default()
});
}
Expand Down Expand Up @@ -119,7 +117,7 @@ pub struct StructuralIssues {
pub missing_curve: Option<Handle<Curve<3>>>,

/// Missing vertices found in edge validation
pub missing_vertices: HashSet<Handle<Vertex<3>>>,
pub missing_vertices: HashSet<Handle<Vertex>>,

/// Missing edges found in cycle validation
pub missing_edges: HashSet<Handle<Edge<3>>>,
Expand Down
4 changes: 2 additions & 2 deletions crates/fj-kernel/src/shape/validate/uniqueness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use crate::{
};

pub fn validate_vertex(
vertex: &Vertex<3>,
handle: Option<&Handle<Vertex<3>>>,
vertex: &Vertex,
handle: Option<&Handle<Vertex>>,
min_distance: Scalar,
stores: &Stores,
) -> Result<(), UniquenessIssues> {
Expand Down
6 changes: 3 additions & 3 deletions crates/fj-kernel/src/topology/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ impl<'r> VertexBuilder<'r> {
pub fn build_from_point(
self,
point: impl Into<Point<3>>,
) -> ValidationResult<Vertex<3>> {
) -> ValidationResult<Vertex> {
let point = self.shape.get_handle_or_insert(point.into())?;
let vertex = self.shape.get_handle_or_insert(Vertex::new(point))?;
let vertex = self.shape.get_handle_or_insert(Vertex { point })?;

Ok(vertex)
}
Expand Down Expand Up @@ -80,7 +80,7 @@ impl<'r> EdgeBuilder<'r> {
/// Build a line segment from two vertices
pub fn build_line_segment_from_vertices(
self,
vertices: [Handle<Vertex<3>>; 2],
vertices: [Handle<Vertex>; 2],
) -> ValidationResult<Edge<3>> {
let curve = self.shape.insert(Curve::Line(Line::from_points(
vertices.clone().map(|vertex| vertex.get().point()),
Expand Down
9 changes: 5 additions & 4 deletions crates/fj-kernel/src/topology/edge.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::fmt;

use fj_math::Point;

use crate::{
geometry::Curve,
shape::{Handle, LocalForm, Shape},
Expand Down Expand Up @@ -32,7 +34,7 @@ pub struct Edge<const D: usize> {
///
/// If there are no such vertices, that means that both the curve and the
/// edge are continuous (i.e. connected to themselves).
pub vertices: Option<[LocalForm<Vertex<1>, Vertex<3>>; 2]>,
pub vertices: Option<[LocalForm<Point<1>, Vertex>; 2]>,
}

impl Edge<3> {
Expand All @@ -58,7 +60,7 @@ impl Edge<3> {
/// <https://github.com/hannobraun/Fornjot/issues/399>
pub fn new(
curve: Handle<Curve<3>>,
vertices: Option<[Handle<Vertex<3>>; 2]>,
vertices: Option<[Handle<Vertex>; 2]>,
) -> Self {
let curve = LocalForm::canonical_only(curve);

Expand All @@ -69,7 +71,6 @@ impl Edge<3> {
.get()
.point_to_curve_coords(canonical.get().point())
.local();
let local = canonical.get().with_local_form(local);
LocalForm::new(local, canonical)
})
});
Expand All @@ -96,7 +97,7 @@ impl<const D: usize> Edge<D> {
///
/// This is a convenience method that saves the caller from dealing with the
/// [`Handle`]s.
pub fn vertices(&self) -> Option<[Vertex<3>; 2]> {
pub fn vertices(&self) -> Option<[Vertex; 2]> {
self.vertices
.as_ref()
.map(|[a, b]| [a.canonical().get(), b.canonical().get()])
Expand Down
35 changes: 9 additions & 26 deletions crates/fj-kernel/src/topology/vertex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::hash::Hash;

use fj_math::Point;

use crate::shape::{Handle, LocalForm, Shape};
use crate::shape::{Handle, Shape};

use super::VertexBuilder;

Expand Down Expand Up @@ -31,19 +31,12 @@ use super::VertexBuilder;
/// between distinct vertices can be configured using
/// [`Shape::with_minimum_distance`].
#[derive(Clone, Debug, Eq, Ord, PartialOrd)]
pub struct Vertex<const D: usize> {
pub struct Vertex {
/// The point that defines the location of the vertex
pub point: LocalForm<Point<D>, Point<3>>,
pub point: Handle<Point<3>>,
}

impl Vertex<3> {
/// Construct a new instance of `Vertex`
pub fn new(point: Handle<Point<3>>) -> Self {
Self {
point: LocalForm::new(point.get(), point),
}
}

impl Vertex {
/// Build a vertex using the [`VertexBuilder`] API
pub fn builder(shape: &mut Shape) -> VertexBuilder {
VertexBuilder::new(shape)
Expand All @@ -54,28 +47,18 @@ impl Vertex<3> {
/// This is a convenience method that saves the caller from dealing with the
/// [`Handle`].
pub fn point(&self) -> Point<3> {
self.point.canonical().get()
}

/// Add a local form of the point to the vertex
pub fn with_local_form<const D: usize>(
self,
local: impl Into<Point<D>>,
) -> Vertex<D> {
Vertex {
point: LocalForm::new(local.into(), self.point.canonical()),
}
self.point.get()
}
}

impl<const D: usize> PartialEq for Vertex<D> {
impl PartialEq for Vertex {
fn eq(&self, other: &Self) -> bool {
self.point == other.point
self.point() == other.point()
}
}

impl<const D: usize> Hash for Vertex<D> {
impl Hash for Vertex {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.point.hash(state);
self.point().hash(state);
}
}