CADmium-Co / ISOtope

A standalone Iterative Solver for 2D sketch constraints
Other
22 stars 6 forks source link

Bug: Containment with Arcs is Incorrect #32

Open dwmunster opened 5 months ago

dwmunster commented 5 months ago

Overview

Currently, faces are tested for containment within one another by converting to polygons and using geo to test containment of those polygons. However, all Segments are converted to polygons by adding a (straight) line from the start point to the end point. This can differ significantly from the true area if the Segment is of type Segment::Arc.

Steps to Reproduce

#[cfg(test)]
mod tests {
    use crate::decompose::ring::Ring;
    use crate::decompose::segment::Segment;
    use crate::primitives::arc::Arc;
    use crate::primitives::circle::Circle;
    use crate::primitives::point2::Point2;
    use geo::Contains;
    use std::cell::RefCell;
    use std::f64::consts::PI;
    use std::rc::Rc;

    #[test]
    fn test_arcs_contains_circle() {
        let origin = Rc::new(RefCell::new(Point2::new(0.0, 0.0)));
        let circle = Circle::new(origin.clone(), 0.99);
        let ring = Ring::Circle(circle);

        let angles = [0., PI / 2., PI, 3. * PI / 2.];
        let segments = angles
            .iter()
            .zip(angles.iter().cycle().skip(1))
            .map(|(start, end)| Segment::Arc(Arc::new(origin.clone(), 1.0, false, *start, *end)))
            .collect();
        let arcs = Ring::Segments(segments);
        assert!(!ring.as_polygon().contains(&arcs.as_polygon()));
        assert!(arcs.as_polygon().contains(&ring.as_polygon()));
    }
}

This test constructs a circle of radius 1 by joining 4 quarter-circle Arc segments and compares it against a circle of radius 0.99.

Expected Outcome

Test passes.

Actual Outcome

The assertion that arcs.as_polygon().contains(&ring.as_polygon()) fails because the former is rendered as a diamond (with vertices at [(-1, 0), (0, -1), (1, 0), (0, 1)]) and the latter as a 36-gon of radius 0.99.

dwmunster commented 5 months ago

I do have some WIP on replacing geo for our more simple case that would resolve this, but I'm not sure when I will have a chance to finish that work. Thus, I wanted to at least capture it here.