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

Remove position fields of SurfaceVertex/PartialSurfaceVertex #1640

Merged
merged 34 commits into from
Mar 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
d8df9b3
Inline trait method into only call site
hannobraun Mar 2, 2023
f56b85a
Move automatic inference out of builder method
hannobraun Mar 2, 2023
1d42838
Remove redundant code
hannobraun Mar 2, 2023
34f5769
Remove unnecessary references
hannobraun Mar 2, 2023
32495c8
Simplify trait method argument
hannobraun Mar 2, 2023
8917c8a
Prepare code for follow-on change
hannobraun Mar 2, 2023
0b6e9c4
Prepare code for follow-on change
hannobraun Mar 2, 2023
5011a3a
Prepare code for follow-on change
hannobraun Mar 2, 2023
4f161b4
Be more explicit about arguments of builder method
hannobraun Mar 2, 2023
9b97696
Make variable name more explicit
hannobraun Mar 2, 2023
ee95933
Prepare code for follow-on change
hannobraun Mar 2, 2023
381bbca
Simplify builder method arguments
hannobraun Mar 2, 2023
67786cf
Simplify argument names
hannobraun Mar 2, 2023
3732d10
Remove redundant code
hannobraun Mar 2, 2023
c48b65d
Remove redundant write to surface position
hannobraun Mar 2, 2023
13205f2
Remove useless references
hannobraun Mar 2, 2023
5f7cd2c
Prepare code for follow-on change
hannobraun Mar 2, 2023
8d9239d
Prepare code for follow-on change
hannobraun Mar 2, 2023
a98508e
Simplify trait method argument
hannobraun Mar 2, 2023
a46b77a
Update order of arguments
hannobraun Mar 2, 2023
9240011
Be more explicit about arguments of builder method
hannobraun Mar 2, 2023
3a79243
Remove unnecessary `unwrap`s
hannobraun Mar 2, 2023
fe2aa59
Make name of binding more explicit
hannobraun Mar 2, 2023
3b7b298
Make name of binding more explicit
hannobraun Mar 2, 2023
aad3d35
Remove unnecessary `expect`s
hannobraun Mar 2, 2023
b8dbd62
Compute `PartialHalfEdge`'s start position
hannobraun Mar 1, 2023
1e0d7c1
Remove unused builder method
hannobraun Mar 2, 2023
77049bb
Update test name
hannobraun Mar 2, 2023
2a06d47
Base validation check for vertex position on curve
hannobraun Mar 2, 2023
30d09d6
Use test values less prone to accuracy errors
hannobraun Mar 2, 2023
9a23681
Remove validation check
hannobraun Mar 2, 2023
d7c26c4
Remove `position` fields of surface vertex structs
hannobraun Mar 2, 2023
46ffa6d
Simplify test code
hannobraun Mar 2, 2023
68d6c23
Remove unused code
hannobraun Mar 2, 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
36 changes: 17 additions & 19 deletions crates/fj-kernel/src/algorithms/sweep/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,41 +94,39 @@ impl Sweep for (Handle<HalfEdge>, &Handle<SurfaceVertex>, &Surface, Color) {
edge_down.write(),
]
.zip_ext(boundaries)
.zip_ext(surface_points)
.zip_ext(global_vertices)
.map(
|(((mut half_edge, boundary), surface_point), global_vertex)| {
for (a, b) in
half_edge.boundary.each_mut_ext().zip_ext(boundary)
{
*a = Some(b);
}

// Writing to the start vertices is enough. Neighboring half-
// edges share surface vertices, so writing the start vertex of
// each half-edge writes to all vertices.
let mut vertex = half_edge.start_vertex.write();
vertex.position = Some(surface_point);
vertex.global_form = Partial::from(global_vertex);
},
);
.map(|((mut half_edge, boundary), global_vertex)| {
for (a, b) in half_edge.boundary.each_mut_ext().zip_ext(boundary) {
*a = Some(b);
}

// Writing to the start vertices is enough. Neighboring half-
// edges share surface vertices, so writing the start vertex of
// each half-edge writes to all vertices.
let mut vertex = half_edge.start_vertex.write();
vertex.global_form = Partial::from(global_vertex);
});

// With the vertices set, we can now update the curves.
//
// Those are all line segments. For the bottom and top curve, because
// even if the original edge was a circle, it's still going to be a line
// when projected into the new surface. For the side edges, because
// we're sweeping along a straight path.
for (mut edge, next) in [
for ((mut half_edge, start), (next_half_edge, end)) in [
edge_bottom.clone(),
edge_up.clone(),
edge_top.clone(),
edge_down.clone(),
]
.zip_ext(surface_points)
.into_iter()
.circular_tuple_windows()
{
edge.write().update_as_line_segment(next.clone());
half_edge.write().update_as_line_segment(start, end);
half_edge
.write()
.infer_global_form(next_half_edge.read().start_vertex.clone());
}

// Finally, we can make sure that all edges refer to the correct global
Expand Down
17 changes: 0 additions & 17 deletions crates/fj-kernel/src/algorithms/sweep/face.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,23 +94,6 @@ impl Sweep for Handle<Face> {
.write()
.connect_to_closed_edges(top_edges, &top_surface.geometry());

for half_edge in &mut top_cycle.write().half_edges {
let mut half_edge = half_edge.write();

let mut start_vertex = half_edge.start_vertex.write();
let global_point = start_vertex.global_form.read().position;

if start_vertex.position.is_none() {
if let Some(global_point) = global_point {
start_vertex.position = Some(
top_surface
.geometry()
.project_global_point(global_point),
);
}
}
}

for (bottom, top) in original_edges
.into_iter()
.zip(top_cycle.write().half_edges.iter_mut())
Expand Down
6 changes: 1 addition & 5 deletions crates/fj-kernel/src/algorithms/transform/vertex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,12 @@ impl TransformObject for SurfaceVertex {
objects: &mut Service<Objects>,
cache: &mut TransformCache,
) -> Self {
// Don't need to transform position, as that is defined in surface
// coordinates and thus transforming the surface takes care of it.
let position = self.position();

let global_form = self
.global_form()
.clone()
.transform_with_cache(transform, objects, cache);

Self::new(position, global_form)
Self::new(global_form)
}
}

Expand Down
10 changes: 5 additions & 5 deletions crates/fj-kernel/src/algorithms/triangulate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,11 +196,11 @@ mod tests {
// is not part of the polygon. The higher-level triangulation will
// filter that out, but it will result in missing triangles.

let a = [0.1, 0.0];
let b = [0.2, 0.9];
let c = [0.2, 1.0];
let d = [0.1, 0.1];
let e = [0.0, 1.0];
let a = [1., 0.];
let b = [2., 8.];
let c = [2., 9.];
let d = [1., 1.];
let e = [0., 9.];

let surface = services.objects.surfaces.xy_plane();
let mut face = PartialFace {
Expand Down
50 changes: 13 additions & 37 deletions crates/fj-kernel/src/builder/cycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,6 @@ pub trait CycleBuilder {
/// meaning its front and back vertices are the same.
fn add_half_edge(&mut self) -> Partial<HalfEdge>;

/// Add a new half-edge that starts at the provided point
///
/// Opens the cycle between the last and first edge, updates the last edge
/// to go the provided point, and adds a new half-edge from the provided
/// point the the first edge.
///
/// If the cycle doesn't have any edges yet, the new edge connects to
/// itself, starting and ending at the provided point.
fn add_half_edge_from_point_to_start(
&mut self,
point: impl Into<Point<2>>,
) -> Partial<HalfEdge>;

/// Update cycle as a polygon from the provided points
fn update_as_polygon_from_points<O, P>(
&mut self,
Expand All @@ -44,11 +31,6 @@ pub trait CycleBuilder {
O: ObjectArgument<P>,
P: Into<Point<2>>;

/// Update cycle as a polygon
///
/// Will update each half-edge in the cycle to be a line segment.
fn update_as_polygon(&mut self);

/// Connect the cycles to the provided half-edges
///
/// Assumes that the provided half-edges, once translated into local
Expand Down Expand Up @@ -109,15 +91,6 @@ impl CycleBuilder for PartialCycle {
new_half_edge
}

fn add_half_edge_from_point_to_start(
&mut self,
point: impl Into<Point<2>>,
) -> Partial<HalfEdge> {
let mut half_edge = self.add_half_edge();
half_edge.write().start_vertex.write().position = Some(point.into());
half_edge
}

fn update_as_polygon_from_points<O, P>(
&mut self,
points: O,
Expand All @@ -126,18 +99,21 @@ impl CycleBuilder for PartialCycle {
O: ObjectArgument<P>,
P: Into<Point<2>>,
{
let half_edges =
points.map(|point| self.add_half_edge_from_point_to_start(point));
self.update_as_polygon();
half_edges
}

fn update_as_polygon(&mut self) {
for (mut half_edge, next) in
self.half_edges.iter().cloned().circular_tuple_windows()
let mut start_positions = Vec::new();
let half_edges = points.map(|point| {
start_positions.push(point.into());
self.add_half_edge()
});

for ((start, end), half_edge) in start_positions
.into_iter()
.circular_tuple_windows()
.zip(&mut self.half_edges)
{
half_edge.write().update_as_line_segment(next.clone());
half_edge.write().update_as_line_segment(start, end);
}

half_edges
}

fn connect_to_closed_edges<O>(
Expand Down
62 changes: 12 additions & 50 deletions crates/fj-kernel/src/builder/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ pub trait HalfEdgeBuilder {
/// Panics if the given angle is not within the range (-2pi, 2pi) radians.
fn update_as_arc(
&mut self,
start: Point<2>,
end: Point<2>,
angle_rad: impl Into<Scalar>,
next_half_edge: Partial<HalfEdge>,
);

/// Update partial half-edge to be a line segment
fn update_as_line_segment(
&mut self,
next_half_edge: Partial<HalfEdge>,
start: Point<2>,
end: Point<2>,
) -> Curve;

/// Infer the global form of the half-edge
Expand Down Expand Up @@ -80,9 +82,6 @@ impl HalfEdgeBuilder for PartialHalfEdge {
let [a_curve, b_curve] =
[Scalar::ZERO, Scalar::TAU].map(|coord| Point::from([coord]));

self.start_vertex.write().position =
Some(path.point_from_path_coords(a_curve));

for (point_boundary, point_curve) in
self.boundary.each_mut_ext().zip_ext([a_curve, b_curve])
{
Expand All @@ -96,20 +95,14 @@ impl HalfEdgeBuilder for PartialHalfEdge {

fn update_as_arc(
&mut self,
start: Point<2>,
end: Point<2>,
angle_rad: impl Into<Scalar>,
mut next_half_edge: Partial<HalfEdge>,
) {
let angle_rad = angle_rad.into();
if angle_rad <= -Scalar::TAU || angle_rad >= Scalar::TAU {
panic!("arc angle must be in the range (-2pi, 2pi) radians");
}
let [start, end] = [
&self.start_position(),
&next_half_edge.read().start_position(),
]
.map(|position| {
position.expect("Can't infer arc without surface position")
});

let arc = fj_math::Arc::from_endpoints_and_angle(start, end, angle_rad);

Expand All @@ -119,35 +112,20 @@ impl HalfEdgeBuilder for PartialHalfEdge {
let [a_curve, b_curve] =
[arc.start_angle, arc.end_angle].map(|coord| Point::from([coord]));

for ((point_boundary, surface_vertex), point_curve) in self
.boundary
.each_mut_ext()
.zip_ext([
&mut self.start_vertex,
&mut next_half_edge.write().start_vertex,
])
.zip_ext([a_curve, b_curve])
for (point_boundary, point_curve) in
self.boundary.each_mut_ext().zip_ext([a_curve, b_curve])
{
*point_boundary = Some(point_curve);
surface_vertex.write().position =
Some(path.point_from_path_coords(point_curve));
}

self.infer_global_form(next_half_edge.read().start_vertex.clone());
}

fn update_as_line_segment(
&mut self,
next_half_edge: Partial<HalfEdge>,
start: Point<2>,
end: Point<2>,
) -> Curve {
let boundary = self.boundary;
let points_surface = [
&self.start_position(),
&next_half_edge.read().start_position(),
]
.map(|position| {
position.expect("Can't infer line segment without surface position")
});
let points_surface = [start, end];

let path = if let [Some(start), Some(end)] = boundary {
let points = [start, end].zip_ext(points_surface);
Expand All @@ -169,8 +147,6 @@ impl HalfEdgeBuilder for PartialHalfEdge {
path
};

self.infer_global_form(next_half_edge.read().start_vertex.clone());

path
}

Expand Down Expand Up @@ -202,21 +178,7 @@ impl HalfEdgeBuilder for PartialHalfEdge {
{
let position_curve = boundary_point
.expect("Can't infer surface position without curve position");

let position_surface = surface_vertex.read().position;

// Infer surface position, if not available.
let position_surface = match position_surface {
Some(position_surface) => position_surface,
None => {
let position_surface =
path.point_from_path_coords(position_curve);

surface_vertex.write().position = Some(position_surface);

position_surface
}
};
let position_surface = path.point_from_path_coords(position_curve);

// Infer global position, if not available.
let position_global =
Expand Down
17 changes: 2 additions & 15 deletions crates/fj-kernel/src/objects/full/vertex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,13 @@ use crate::storage::Handle;
/// A vertex, defined in surface (2D) coordinates
#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
pub struct SurfaceVertex {
position: Point<2>,
global_form: Handle<GlobalVertex>,
}

impl SurfaceVertex {
/// Construct a new instance of `SurfaceVertex`
pub fn new(
position: impl Into<Point<2>>,
global_form: Handle<GlobalVertex>,
) -> Self {
let position = position.into();
Self {
position,
global_form,
}
}

/// Access the position of the vertex on the surface
pub fn position(&self) -> Point<2> {
self.position
pub fn new(global_form: Handle<GlobalVertex>) -> Self {
Self { global_form }
}

/// Access the global form of the vertex
Expand Down
15 changes: 14 additions & 1 deletion crates/fj-kernel/src/partial/objects/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,20 @@ pub struct PartialHalfEdge {
impl PartialHalfEdge {
/// Compute the surface position where the half-edge starts
pub fn start_position(&self) -> Option<Point<2>> {
self.start_vertex.read().position
// Computing the surface position from the curve position is fine.
// `HalfEdge` "owns" its start position. There is no competing code that
// could compute the surface position from slightly different data.

let [start, _] = self.boundary;
start.and_then(|start| {
let curve = self.curve?;

if let MaybeCurve::Defined(curve) = curve {
return Some(curve.point_from_path_coords(start));
}

None
})
}
}

Expand Down
Loading