Skip to content

Commit

Permalink
Merge pull request #1661 from hannobraun/unify
Browse files Browse the repository at this point in the history 
Simplify connecting cycle to edges
  • Loading branch information
@hannobraun
hannobraun authored Mar 8, 2023
2 parents c0f77c9 + 3198922 commit 61106dc
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 119 deletions.
8 changes: 2 additions & 6 deletions crates/fj-kernel/src/algorithms/sweep/face.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl Sweep for Handle<Face> {
bottom_face.surface().clone().translate(path, objects);

let mut top_face = PartialFace::new(objects);
top_face.surface = Some(top_surface.clone());
top_face.surface = Some(top_surface);
top_face.color = Some(self.color());

for (i, cycle) in bottom_face.all_cycles().cloned().enumerate() {
Expand Down Expand Up @@ -89,11 +89,7 @@ impl Sweep for Handle<Face> {
top_edges.push(Partial::from(top_edge));
}

top_cycle.write().connect_to_closed_edges(
top_edges,
&top_surface.geometry(),
objects,
);
top_cycle.write().connect_to_edges(top_edges, objects);

for (bottom, top) in original_edges
.into_iter()
Expand Down
15 changes: 6 additions & 9 deletions crates/fj-kernel/src/builder/cycle.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use fj_math::Point;
use itertools::Itertools;

use crate::{
geometry::surface::SurfaceGeometry,
objects::{HalfEdge, Objects},
partial::{Partial, PartialCycle},
services::Service,
Expand Down Expand Up @@ -42,10 +41,9 @@ pub trait CycleBuilder {
/// equivalents of this cycle, form a cycle themselves.
///
/// Returns the local equivalents of the provided half-edges.
fn connect_to_closed_edges<O>(
fn connect_to_edges<O>(
&mut self,
edges: O,
surface: &SurfaceGeometry,
objects: &mut Service<Objects>,
) -> O::SameSize<Partial<HalfEdge>>
where
Expand Down Expand Up @@ -88,19 +86,18 @@ impl CycleBuilder for PartialCycle {
half_edges
}

fn connect_to_closed_edges<O>(
fn connect_to_edges<O>(
&mut self,
edges: O,
surface: &SurfaceGeometry,
objects: &mut Service<Objects>,
) -> O::SameSize<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>,
{
edges.map_with_prev(|other, prev| {
let mut this = self.add_half_edge(objects);
this.write().update_from_other_edge(&other, &prev, surface);
this
edges.map_with_prev(|_, prev| {
let mut edge = self.add_half_edge(objects);
edge.write().start_vertex = prev.read().start_vertex.clone();
edge
})
}
}
104 changes: 1 addition & 103 deletions crates/fj-kernel/src/builder/edge.rs
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
use fj_interop::ext::ArrayExt;
use fj_math::{Point, Scalar};

use crate::{
geometry::{
curve::{Curve, GlobalPath},
surface::SurfaceGeometry,
},
objects::HalfEdge,
partial::{Partial, PartialHalfEdge},
};
use crate::{geometry::curve::Curve, partial::PartialHalfEdge};

/// Builder API for [`PartialHalfEdge`]
pub trait HalfEdgeBuilder {
Expand Down Expand Up @@ -37,22 +30,6 @@ pub trait HalfEdgeBuilder {
start: Point<2>,
end: Point<2>,
) -> Curve;

/// Update this edge from another
///
/// Infers as much information about this edge from the other, under the
/// assumption that the other edge is on a different surface.
///
/// This method is quite fragile. It might panic, or even silently fail,
/// under various circumstances. As long as you're only dealing with lines
/// and planes, you should be fine. Otherwise, please read the code of this
/// method carefully, to make sure you don't run into trouble.
fn update_from_other_edge(
&mut self,
other: &Partial<HalfEdge>,
other_prev: &Partial<HalfEdge>,
surface: &SurfaceGeometry,
);
}

impl HalfEdgeBuilder for PartialHalfEdge {
Expand Down Expand Up @@ -130,83 +107,4 @@ impl HalfEdgeBuilder for PartialHalfEdge {

path
}

fn update_from_other_edge(
&mut self,
other: &Partial<HalfEdge>,
other_prev: &Partial<HalfEdge>,
surface: &SurfaceGeometry,
) {
self.curve = other.read().curve.as_ref().and_then(|path| {
// We have information about the other edge's surface available. We
// need to use that to interpret what the other edge's curve path
// means for our curve path.
match surface.u {
GlobalPath::Circle(_) => {
// The other surface is curved. We're entering some dodgy
// territory here, as only some edge cases can be
// represented using our current curve/surface
// representation.
match path {
Curve::Line(_) => {
// We're dealing with a line on a rounded surface.
//
// Based on the current uses of this method, we can
// make some assumptions:
//
// 1. The line is parallel to the u-axis of the
// other surface.
// 2. The surface that *our* edge is in is a plane
// that is parallel to the the plane of the
// circle that defines the curvature of the other
// surface.
//
// These assumptions are necessary preconditions for
// the following code to work. But unfortunately, I
// see no way to check those preconditions here, as
// neither the other line nor our surface is
// necessarily defined yet.
//
// Handling this case anyway feels like a grave sin,
// but I don't know what else to do. If you tracked
// some extremely subtle and annoying bug back to
// this code, I apologize.
//
// I hope that I'll come up with a better curve/
// surface representation before this becomes a
// problem.
None
}
_ => {
// The other edge is a line segment in a curved
// surface. No idea how to deal with this.
todo!(
"Can't connect edge to circle on curved \
surface"
)
}
}
}
GlobalPath::Line(_) => {
// The other edge is defined on a plane.
match path {
Curve::Line(_) => {
// The other edge is a line segment on a plane. That
// means our edge must be a line segment too.
None
}
_ => {
// The other edge is a circle or arc on a plane. I'm
// actually not sure what that means for our edge.
// We might be able to represent it somehow, but
// let's leave that as an exercise for later.
todo!("Can't connect edge to circle on plane")
}
}
}
}
});

self.start_vertex = other_prev.read().start_vertex.clone();
}
}
2 changes: 1 addition & 1 deletion crates/fj-kernel/src/builder/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub trait ObjectArgument<T>: IntoIterator<Item = T> {

/// Create a return value by mapping the implementing type
///
/// Provides access to the (circular) next item.
/// Provides access to the (circular) previous item.
fn map_with_prev<F, R>(self, f: F) -> Self::SameSize<R>
where
F: FnMut(T, T) -> R,
Expand Down

0 comments on commit 61106dc

Please sign in to comment.