Skip to content

Commit

Permalink
Simplify builder method
Browse files Browse the repository at this point in the history
  • Loading branch information
@hannobraun
hannobraun committed Feb 17, 2023
1 parent 0817dd3 commit a7365e6
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 96 deletions.
2 changes: 1 addition & 1 deletion crates/fj-kernel/src/builder/cycle.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ impl CycleBuilder for PartialCycle {
{
edges.map(|other| {
let mut this = self.add_half_edge();
this.write().update_from_other_edge(&other, &Some(*surface));
this.write().update_from_other_edge(&other, surface);
this
})
}
Expand Down
165 changes: 70 additions & 95 deletions crates/fj-kernel/src/builder/edge.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub trait HalfEdgeBuilder {
fn update_from_other_edge(
&mut self,
other: &Partial<HalfEdge>,
surface: &Option<SurfaceGeometry>,
surface: &SurfaceGeometry,
);
}

Expand Down Expand Up @@ -222,111 +222,86 @@ impl HalfEdgeBuilder for PartialHalfEdge {
fn update_from_other_edge(
&mut self,
other: &Partial<HalfEdge>,
surface: &Option<SurfaceGeometry>,
surface: &SurfaceGeometry,
) {
let global_curve = other.read().curve.read().global_form.clone();
self.curve.write().global_form = global_curve.clone();
self.global_form.write().curve = global_curve;

self.curve.write().path =
other.read().curve.read().path.as_ref().and_then(|path| {
match surface {
Some(surface) => {
// 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(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 {
MaybeSurfacePath::Defined(
SurfacePath::Line(_),
)
| MaybeSurfacePath::UndefinedLine => {
// 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.
Some(
MaybeSurfacePath::UndefinedCircle {
radius: circle.radius(),
},
)
}
_ => {
// 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"
)
}
}
// 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(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 {
MaybeSurfacePath::Defined(SurfacePath::Line(_))
| MaybeSurfacePath::UndefinedLine => {
// 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.
Some(MaybeSurfacePath::UndefinedCircle {
radius: circle.radius(),
})
}
GlobalPath::Line(_) => {
// The other edge is defined on a plane.
match path {
MaybeSurfacePath::Defined(
SurfacePath::Line(_),
)
| MaybeSurfacePath::UndefinedLine => {
// The other edge is a line segment on
// a plane. That means our edge must be
// a line segment too.
Some(MaybeSurfacePath::UndefinedLine)
}
_ => {
// 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"
)
}
}
_ => {
// 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"
)
}
}
}
None => {
// We know nothing about the surface the other edge is
// on. This means we can't infer anything about our
// curve from the other curve.
None
GlobalPath::Line(_) => {
// The other edge is defined on a plane.
match path {
MaybeSurfacePath::Defined(SurfacePath::Line(_))
| MaybeSurfacePath::UndefinedLine => {
// The other edge is a line segment on a plane.
// That means our edge must be a line segment
// too.
Some(MaybeSurfacePath::UndefinedLine)
}
_ => {
// 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")
}
}
}
}
});
Expand Down

0 comments on commit a7365e6

Please sign in to comment.