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 various fixes and small updates in builder API #1572

Merged
merged 6 commits into from
Feb 8, 2023
Merged

Make various fixes and small updates in builder API #1572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3cac27e
Fix path inference when updating edge from another
hannobraun Feb 3, 2023
08d399c
Update doc comment
hannobraun Feb 3, 2023
27b57b6
Update variable name
hannobraun Feb 3, 2023
25fb27c
Remove unused code
hannobraun Feb 7, 2023
eb8411a
Store radius of undefined circle
hannobraun Feb 7, 2023
419921a
Move methods from `FaceBuilder` to `CycleBuilder`
hannobraun Feb 8, 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
68 changes: 68 additions & 0 deletions crates/fj-kernel/src/builder/cycle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::VecDeque;

use fj_interop::ext::ArrayExt;
use fj_math::Point;

Expand Down Expand Up @@ -76,6 +78,33 @@ pub trait CycleBuilder {
&mut self,
points: [impl Into<Point<3>>; 3],
) -> [Partial<HalfEdge>; 3];

/// Connect the cycle to the provided half-edges
///
/// Assumes that the provided half-edges, once translated into local
/// equivalents of this cycle, will not form a cycle themselves.
///
/// Returns the local equivalents of the provided half-edges and, as the
/// last entry, an additional half-edge that closes the cycle.
fn connect_to_open_edges<O>(
&mut self,
edges: O,
) -> O::SizePlusOne<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>;

/// Connect the cycles to the provided half-edges
///
/// Assumes that the provided half-edges, once translated into local
/// equivalents of this cycle, form a cycle themselves.
///
/// Returns the local equivalents of the provided half-edges.
fn connect_to_closed_edges<O>(
&mut self,
edges: O,
) -> O::SameSize<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>;
}

impl CycleBuilder for PartialCycle {
Expand Down Expand Up @@ -197,4 +226,43 @@ impl CycleBuilder for PartialCycle {

half_edges
}

fn connect_to_open_edges<O>(
&mut self,
edges: O,
) -> O::SizePlusOne<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>,
{
// We need to create the additional half-edge last, but at the same time
// need to provide it to the `map_plus_one` method first. Really no
// choice but to create them all in one go, as we do here.
let mut half_edges = VecDeque::new();
for _ in 0..edges.num_objects() {
half_edges.push_back(self.add_half_edge());
}
let additional_half_edge = self.add_half_edge();

edges.map_plus_one(additional_half_edge, |other| {
let mut this = half_edges.pop_front().expect(
"Pushed correct number of half-edges; should be able to pop",
);
this.write().update_from_other_edge(&other);
this
})
}

fn connect_to_closed_edges<O>(
&mut self,
edges: O,
) -> O::SameSize<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>,
{
edges.map(|other| {
let mut this = self.add_half_edge();
this.write().update_from_other_edge(&other);
this
})
}
}
115 changes: 108 additions & 7 deletions crates/fj-kernel/src/builder/edge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use fj_interop::ext::ArrayExt;
use fj_math::{Point, Scalar};

use crate::{
geometry::path::{GlobalPath, SurfacePath},
objects::{GlobalEdge, HalfEdge, Surface},
partial::{MaybeSurfacePath, Partial, PartialGlobalEdge, PartialHalfEdge},
};
Expand Down Expand Up @@ -51,6 +52,11 @@ pub trait HalfEdgeBuilder {
///
/// 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>);
}

Expand Down Expand Up @@ -196,13 +202,108 @@ impl HalfEdgeBuilder for PartialHalfEdge {
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()
.map(MaybeSurfacePath::to_undefined);
self.curve.write().path =
other.read().curve.read().path.as_ref().and_then(|path| {
match other.read().curve.read().surface.read().geometry {
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"
)
}
}
}
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"
)
}
}
}
}
}
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
}
}
});

for (this, other) in self
.vertices
Expand Down
76 changes: 5 additions & 71 deletions crates/fj-kernel/src/builder/face.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,41 +4,14 @@ use fj_interop::ext::ArrayExt;

use crate::{
geometry::path::SurfacePath,
objects::{Cycle, HalfEdge, Surface},
objects::{Cycle, Surface},
partial::{MaybeSurfacePath, Partial, PartialCycle, PartialFace},
};

use super::{CycleBuilder, HalfEdgeBuilder, ObjectArgument, SurfaceBuilder};
use super::SurfaceBuilder;

/// Builder API for [`PartialFace`]
pub trait FaceBuilder {
/// Connect the face to other faces at the provided half-edges
///
/// Assumes that the provided half-edges, once translated into local
/// equivalents of this face, will not form a cycle.
///
/// Returns the local equivalents of the provided half-edges and, as the
/// last entry, an additional half-edge that closes the cycle.
fn connect_to_open_edges<O>(
&mut self,
edges: O,
) -> O::SizePlusOne<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>;

/// Connect the face to other faces at the provided half-edges
///
/// Assumes that the provided half-edges, once translated into local
/// equivalents of this face, form a cycle.
///
/// Returns the local equivalents of the provided half-edges.
fn connect_to_closed_edges<O>(
&mut self,
edges: O,
) -> O::SameSize<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>;

/// Add an interior cycle
fn add_interior(&mut self) -> Partial<Cycle>;

Expand All @@ -59,45 +32,6 @@ pub trait FaceBuilder {
}

impl FaceBuilder for PartialFace {
fn connect_to_open_edges<O>(
&mut self,
edges: O,
) -> O::SizePlusOne<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>,
{
// We need to create the additional half-edge last, but at the same time
// need to provide it to the `map_plus_one` method first. Really no
// choice but to create them all in one go, as we do here.
let mut half_edges = VecDeque::new();
for _ in 0..edges.num_objects() {
half_edges.push_back(self.exterior.write().add_half_edge());
}
let additional_half_edge = self.exterior.write().add_half_edge();

edges.map_plus_one(additional_half_edge, |other| {
let mut this = half_edges.pop_front().expect(
"Pushed correct number of half-edges; should be able to pop",
);
this.write().update_from_other_edge(&other);
this
})
}

fn connect_to_closed_edges<O>(
&mut self,
edges: O,
) -> O::SameSize<Partial<HalfEdge>>
where
O: ObjectArgument<Partial<HalfEdge>>,
{
edges.map(|other| {
let mut this = self.exterior.write().add_half_edge();
this.write().update_from_other_edge(&other);
this
})
}

fn add_interior(&mut self) -> Partial<Cycle> {
let cycle = Partial::from_partial(PartialCycle {
surface: self.exterior.read().surface.clone(),
Expand Down Expand Up @@ -125,7 +59,7 @@ impl FaceBuilder for PartialFace {
})
.collect::<VecDeque<_>>();

let (first_three_vertices, surface) = {
let (significant_vertices, surface) = {
let first_three_vertices = array::from_fn(|_| {
vertices
.pop_front()
Expand Down Expand Up @@ -153,7 +87,7 @@ impl FaceBuilder for PartialFace {
});

for (mut surface_vertex, point) in
first_three_vertices.into_iter().chain(rest_of_vertices)
significant_vertices.into_iter().chain(rest_of_vertices)
{
surface_vertex.write().position = Some(point);
}
Expand All @@ -173,7 +107,7 @@ impl FaceBuilder for PartialFace {
MaybeSurfacePath::Defined(_) => {
// Path is already defined. Nothing to infer.
}
MaybeSurfacePath::UndefinedCircle => todo!(
MaybeSurfacePath::UndefinedCircle { .. } => todo!(
"Inferring undefined circles is not supported yet"
),
MaybeSurfacePath::UndefinedLine => {
Expand Down
23 changes: 6 additions & 17 deletions crates/fj-kernel/src/partial/objects/curve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use fj_math::Scalar;

use crate::{
geometry::path::SurfacePath,
objects::{Curve, GlobalCurve, Objects, Surface},
Expand Down Expand Up @@ -55,28 +57,15 @@ pub enum MaybeSurfacePath {
Defined(SurfacePath),

/// The surface path is undefined, but we know it is a circle
UndefinedCircle,
UndefinedCircle {
/// The radius of the undefined circle
radius: Scalar,
},

/// The surface path is undefined, but we know it is a line
UndefinedLine,
}

impl MaybeSurfacePath {
/// Convert into an undefined variant
///
/// If `self` is defined, it is converted into the applicable undefined
/// variant. If it is undefined, a copy is returned.
pub fn to_undefined(&self) -> Self {
match self {
Self::Defined(path) => match path {
SurfacePath::Circle(_) => Self::UndefinedCircle,
SurfacePath::Line(_) => Self::UndefinedLine,
},
undefined => undefined.clone(),
}
}
}

impl From<SurfacePath> for MaybeSurfacePath {
fn from(path: SurfacePath) -> Self {
Self::Defined(path)
Expand Down