Merge pull request #1532 from yztink/fix/ellipse-block-transform

fix(ellipse): accurately transform ellipse and elliptical arcs in blocks and standard transforms

src/entities/ellipse.rs

@@ -30,13 +30,28 @@ fn to_render(ell: &Ellipse) -> RenderEntity {
         .map(crate::entities::curve::snap_from)
         .unwrap_or_default();
 
+    let is_full = ell.is_full()
+        || (ell.end_parameter - ell.start_parameter).abs() >= std::f64::consts::TAU - 1e-9;
+    let (start_param, end_param) = if is_full {
+        (0.0, std::f64::consts::TAU)
+    } else {
+        let s = ell.start_parameter.rem_euclid(std::f64::consts::TAU);
+        let raw = ell.end_parameter - ell.start_parameter;
+        let sweep = if raw <= 0.0 {
+            raw.rem_euclid(std::f64::consts::TAU)
+        } else {
+            raw
+        };
+        (s, s + sweep)
+    };
+
     let tangent = TangentGeom::PlanarEllipse {
         center: [ell.center.x, ell.center.y, ell.center.z],
         major_axis: [ell.major_axis.x, ell.major_axis.y, ell.major_axis.z],
         normal: [ell.normal.x, ell.normal.y, ell.normal.z],
         minor_axis_ratio: ell.minor_axis_ratio,
-        start_param: ell.start_parameter,
+        start_param,
-        end_param: ell.end_parameter,
+        end_param,
     };
 
     // The points come from the entity's own kernel curve and angular policy.
@@ -339,20 +354,58 @@ fn apply_grip(ell: &mut Ellipse, grip_id: usize, apply: GripApply) {
 }
 
 fn apply_transform(ell: &mut Ellipse, t: &EntityTransform) {
-    crate::scene::view::transform::apply_standard_entity_transform(ell, t, |entity, p1, p2| {
+    use codec::types::{Transform, Vector3};
-        crate::scene::view::transform::reflect_xy_point(
+    let transform = match t {
-            &mut entity.center.x,
+        EntityTransform::Translate(d) => {
-            &mut entity.center.y,
+            ell.center = ell.center + Vector3::new(d.x, d.y, d.z);
-            p1,
+            return;
-            p2,
+        }
-        );
+        EntityTransform::Rotate { center, axis, angle_rad } => {
-        crate::scene::view::transform::reflect_xy_point(
+            let axis_norm = Vector3::new(axis.x, axis.y, axis.z);
-            &mut entity.major_axis.x,
+            let axis_len = axis_norm.length();
-            &mut entity.major_axis.y,
+            let axis_v = if axis_len > 1e-12 { axis_norm / axis_len } else { Vector3::UNIT_Z };
-            p1,
+            Transform::from_translation(Vector3::new(-center.x, -center.y, -center.z))
-            p2,
+                .then(&Transform::from_rotation(axis_v, *angle_rad))
-        );
+                .then(&Transform::from_translation(Vector3::new(center.x, center.y, center.z)))
-    });
+        }
+        EntityTransform::Scale { center, factor } => {
+            let s = *factor;
+            Transform::from_scaling_with_origin(
+                Vector3::new(s, s, s),
+                Vector3::new(center.x, center.y, center.z),
+            )
+        }
+        EntityTransform::Mirror { p1, p2, working_normal } => {
+            crate::scene::view::transform::reflection_about_working_line(
+                *p1,
+                *p2,
+                *working_normal,
+            )
+        }
+        EntityTransform::Affine(tr) => tr.clone(),
+    };
+
+    if let Some((c, m, n, ratio, s, e)) = crate::scene::view::transform::transform_ellipse_geometry(
+        ell.center,
+        ell.major_axis,
+        ell.normal,
+        ell.minor_axis_ratio,
+        ell.start_parameter,
+        ell.end_parameter,
+        &transform,
+    ) {
+        ell.center = c;
+        ell.major_axis = m;
+        ell.normal = n;
+        ell.minor_axis_ratio = ratio;
+        ell.start_parameter = s;
+        let is_full = (e - s - std::f64::consts::TAU).abs() < 1e-9;
+        ell.end_parameter = if is_full {
+            std::f64::consts::TAU
+        } else {
+            e.rem_euclid(std::f64::consts::TAU)
+        };
+    }
 }
 
 impl RenderConvertible for Ellipse {
@@ -487,4 +540,50 @@ mod grip_tests {
             "minor still 10 — no ballooning"
         );
     }
+
+    #[test]
+    fn test_apply_transform_negative_scale_and_rotation() {
+        let mut e = ell(10.0, 0.5);
+        e.start_parameter = 0.0;
+        e.end_parameter = std::f64::consts::FRAC_PI_2;
+
+        let rot = EntityTransform::Rotate {
+            center: DVec3::ZERO,
+            axis: DVec3::Z,
+            angle_rad: -std::f64::consts::FRAC_PI_4,
+        };
+        apply_transform(&mut e, &rot);
+
+        let scale = EntityTransform::Scale {
+            center: DVec3::ZERO,
+            factor: -1.0,
+        };
+        apply_transform(&mut e, &scale);
+
+        assert!((xy_len(&e) - 10.0).abs() < 1e-9);
+        assert!((e.minor_axis_ratio - 0.5).abs() < 1e-9);
+        assert!((e.normal.z - 1.0).abs() < 1e-9);
+        // Start parameter was 0, rotated by -45 then scaled by -1 (180 rotation) -> net effect
+        assert!(e.start_parameter >= 0.0 && e.start_parameter < std::f64::consts::TAU);
+    }
+
+    #[test]
+    fn test_apply_transform_mirror() {
+        let mut e = ell(10.0, 0.5);
+        e.start_parameter = 0.1;
+        e.end_parameter = 1.5;
+
+        // Mirror across Y axis (p1=(0,0,0), p2=(0,1,0))
+        let mirror = EntityTransform::Mirror {
+            p1: DVec3::ZERO,
+            p2: DVec3::Y,
+            working_normal: DVec3::Z,
+        };
+        apply_transform(&mut e, &mirror);
+
+        // Major axis should be reflected to (-10, 0, 0)
+        assert!((e.major_axis.x - (-10.0)).abs() < 1e-9);
+        // Normal should flip to -Z to preserve counter-clockwise traversal
+        assert!(e.normal.z < 0.0);
+    }
 }

src/scene/cache/block_cache.rs

@@ -3000,21 +3000,22 @@ fn transform_tangent(
             start_param,
             end_param,
         } => {
-            let c = t.apply(Vector3::new(center[0], center[1], center[2]));
+            let (c, m, n, ratio, s, e) = crate::scene::view::transform::transform_ellipse_geometry(
-            let m = t.apply_rotation(Vector3::new(major_axis[0], major_axis[1], major_axis[2]));
+                Vector3::new(center[0], center[1], center[2]),
-            let n = t.apply_rotation(Vector3::new(normal[0], normal[1], normal[2]));
+                Vector3::new(major_axis[0], major_axis[1], major_axis[2]),
-            let n_len = n.length();
+                Vector3::new(normal[0], normal[1], normal[2]),
-            if n_len <= 1.0e-12 {
+                *minor_axis_ratio,
-                return None;
+                *start_param,
-            }
+                *end_param,
-            let n = n / n_len;
+                t,
+            )?;
             Some(TangentGeom::PlanarEllipse {
                 center: [c.x, c.y, c.z],
                 major_axis: [m.x, m.y, m.z],
                 normal: [n.x, n.y, n.z],
-                minor_axis_ratio: *minor_axis_ratio,
+                minor_axis_ratio: ratio,
-                start_param: *start_param,
+                start_param: s,
-                end_param: *end_param,
+                end_param: e,
             })
         }
     }
@@ -3269,4 +3270,60 @@ mod compact_nested_tests {
         assert_eq!(wires.len(), 1);
         assert!(wires[0].render_instance.is_none());
     }
+
+    #[test]
+    fn test_transform_tangent_ellipse_arc_negative_scale_and_rotation() {
+        let ellipse_geom = TangentGeom::PlanarEllipse {
+            center: [10.0, 20.0, 0.0],
+            major_axis: [100.0, 0.0, 0.0],
+            normal: [0.0, 0.0, 1.0],
+            minor_axis_ratio: 0.5,
+            start_param: std::f64::consts::FRAC_PI_4,
+            end_param: 3.0 * std::f64::consts::FRAC_PI_4,
+        };
+
+        // Negative scale (-1, -1, 1), negative rotation -45 deg, distant translation (7000, 5000, 0)
+        let t = Transform::from_scaling(Vector3::new(-1.0, -1.0, 1.0))
+            .then(&Transform::from_rotation(Vector3::UNIT_Z, -std::f64::consts::FRAC_PI_4))
+            .then(&Transform::from_translation(Vector3::new(7000.0, 5000.0, 0.0)));
+
+        let transformed = transform_tangent(&ellipse_geom, &t).expect("transform_tangent should succeed");
+        let TangentGeom::PlanarEllipse {
+            center: c,
+            major_axis: m,
+            normal: n,
+            minor_axis_ratio: ratio,
+            start_param: s,
+            end_param: _e,
+        } = transformed else {
+            panic!("Expected PlanarEllipse");
+        };
+
+        // Center must be transformed
+        let expected_c = t.apply(Vector3::new(10.0, 20.0, 0.0));
+        assert!((c[0] - expected_c.x).abs() < 1e-9);
+        assert!((c[1] - expected_c.y).abs() < 1e-9);
+
+        // Major axis length must be 100, ratio 0.5
+        let m_vec = Vector3::new(m[0], m[1], m[2]);
+        assert!((m_vec.length() - 100.0).abs() < 1e-9);
+        assert!((ratio - 0.5).abs() < 1e-9);
+        assert!((n[2] - 1.0).abs() < 1e-9);
+
+        // Start and end points on the curve must match mathematical transformation of original start/end
+        let orig_start_pt = Vector3::new(10.0, 20.0, 0.0)
+            + Vector3::new(100.0, 0.0, 0.0) * (std::f64::consts::FRAC_PI_4).cos()
+            + Vector3::new(0.0, 50.0, 0.0) * (std::f64::consts::FRAC_PI_4).sin();
+        let expected_start_pt = t.apply(orig_start_pt);
+
+        let new_v = Vector3::new(n[0], n[1], n[2]).cross(&m_vec) * ratio;
+        let actual_start_pt = Vector3::new(c[0], c[1], c[2])
+            + m_vec * s.cos()
+            + new_v * s.sin();
+
+        assert!(
+            (actual_start_pt - expected_start_pt).length() < 1e-9,
+            "Start point mismatch: expected {expected_start_pt:?}, got {actual_start_pt:?}"
+        );
+    }
 }

src/scene/view/transform.rs

@@ -120,6 +120,93 @@ pub fn reflect_xy_point(x: &mut f64, y: &mut f64, p1: DVec3, p2: DVec3) {
     *y = p1.y as f64 + my;
 }
 
+/// Fully transforms ellipse / elliptical arc geometry under any 3D affine transform
+/// (including negative scales, reflections, negative rotations, non-uniform scales, and translation).
+///
+/// Returns `(new_center, new_major_axis, new_normal, new_minor_axis_ratio, new_start_param, new_end_param)`
+/// where `new_major_axis` is guaranteed to be orthogonal to `new_normal`, its length is >= semi-minor axis length,
+/// and `new_minor_axis_ratio <= 1.0`.
+pub fn transform_ellipse_geometry(
+    center: Vector3,
+    major_axis: Vector3,
+    normal: Vector3,
+    minor_axis_ratio: f64,
+    start_param: f64,
+    end_param: f64,
+    transform: &Transform,
+) -> Option<(Vector3, Vector3, Vector3, f64, f64, f64)> {
+    let norm_len = normal.length();
+    if norm_len <= 1e-12 || !norm_len.is_finite() {
+        return None;
+    }
+    let norm = normal / norm_len;
+    let m = major_axis;
+    let m_len = m.length();
+    if m_len <= 1e-12 || !m_len.is_finite() {
+        return None;
+    }
+    let ratio = minor_axis_ratio.clamp(1e-12, 1.0);
+    // V = (N x M) * ratio
+    let v = norm.cross(&m) * ratio;
+
+    let c_prime = transform.apply(center);
+    let m0 = transform.apply_rotation(m);
+    let v0 = transform.apply_rotation(v);
+
+    let m0_len_sq = m0.length_squared();
+    let v0_len_sq = v0.length_squared();
+    if m0_len_sq <= 1e-24 || v0_len_sq <= 1e-24 || !m0_len_sq.is_finite() || !v0_len_sq.is_finite() {
+        return None;
+    }
+
+    // Extremum angle theta to find the principal axes of the transformed ellipse
+    let dot = m0.dot(&v0);
+    let diff = m0_len_sq - v0_len_sq;
+    let theta = 0.5 * (2.0 * dot).atan2(diff);
+
+    let (sin_t, cos_t) = theta.sin_cos();
+    let m_new = m0 * cos_t + v0 * sin_t;
+    let v_new = m0 * (-sin_t) + v0 * cos_t;
+
+    let a = m_new.length();
+    let b = v_new.length();
+    if a <= 1e-12 || !a.is_finite() {
+        return None;
+    }
+
+    let cross = m_new.cross(&v_new);
+    let cross_len = cross.length();
+    if cross_len <= 1e-12 || !cross_len.is_finite() {
+        return None;
+    }
+    let n_new = cross / cross_len;
+    let new_ratio = (b / a).clamp(1e-12, 1.0);
+
+    // Sweep and parameters
+    use std::f64::consts::TAU;
+    let raw = end_param - start_param;
+    let is_full = (raw - TAU).abs() < 1e-9
+        || (raw.abs() >= TAU - 1e-9)
+        || (raw.rem_euclid(TAU).abs() < 1e-9 && raw.abs() > 1e-9);
+
+    let (new_start, new_end) = if is_full {
+        (0.0, TAU)
+    } else {
+        let sweep = if raw <= 0.0 {
+            let s = raw.rem_euclid(TAU);
+            if s <= 1e-9 { TAU } else { s }
+        } else if raw > TAU {
+            TAU
+        } else {
+            raw
+        };
+        let s = (start_param - theta).rem_euclid(TAU);
+        (s, s + sweep)
+    };
+
+    Some((c_prime, m_new, n_new, new_ratio, new_start, new_end))
+}
+
 /// DXF arbitrary-axis algorithm — returns the OCS X and Y basis vectors in WCS
 /// for a given entity normal vector.
 ///
@@ -271,3 +358,154 @@ mod ocs_axes_142 {
         assert!(p.0.abs()+p.1.abs()+p.2.abs() > 1.0, "point collapsed: {:?}", p);
     }
 }
+
+#[cfg(test)]
+mod ellipse_transform_tests {
+    use super::*;
+    use std::f64::consts::{FRAC_PI_2, FRAC_PI_4};
+
+    fn sample_point(center: Vector3, major: Vector3, normal: Vector3, ratio: f64, t: f64) -> Vector3 {
+        let u = major;
+        let v = normal.normalize().cross(&major) * ratio;
+        center + u * t.cos() + v * t.sin()
+    }
+
+    #[test]
+    fn negative_rotation_negative_scale_distant_origin() {
+        // Original ellipse arc at distant center (7000, 5000, 0)
+        let center = Vector3::new(7000.0, 5000.0, 0.0);
+        let major = Vector3::new(10.0, 0.0, 0.0);
+        let normal = Vector3::new(0.0, 0.0, 1.0);
+        let ratio = 0.5;
+        let start_param = FRAC_PI_4;
+        let end_param = 3.0 * FRAC_PI_4;
+
+        // Transform: non-uniform negative scale (-1.0, -1.0, 1.0), rotation -45 deg, translation (100, 200, 0)
+        let rot_rad = -FRAC_PI_4;
+        let transform = Transform::from_scaling(Vector3::new(-1.0, -1.0, 1.0))
+            .then(&Transform::from_rotation(Vector3::UNIT_Z, rot_rad))
+            .then(&Transform::from_translation(Vector3::new(100.0, 200.0, 0.0)));
+
+        let (c, m, n, r, s, e) = transform_ellipse_geometry(
+            center,
+            major,
+            normal,
+            ratio,
+            start_param,
+            end_param,
+            &transform,
+        )
+        .expect("transformation should succeed");
+
+        assert!(r <= 1.0 && r > 0.0, "ratio must be in (0, 1], got {r}");
+        assert!(m.length() >= m.length() * r);
+
+        // Verify that start, end, and intermediate points match exactly
+        for i in 0..=10 {
+            let frac = i as f64 / 10.0;
+            let orig_t = start_param + frac * (end_param - start_param);
+            let orig_pt = sample_point(center, major, normal, ratio, orig_t);
+            let expected_pt = transform.apply(orig_pt);
+
+            let new_t = s + frac * (e - s);
+            let actual_pt = sample_point(c, m, n, r, new_t);
+
+            let diff = (expected_pt - actual_pt).length();
+            assert!(
+                diff < 1e-9,
+                "Point at frac {frac} differs by {diff}: expected {expected_pt:?}, got {actual_pt:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn axis_swapping_under_non_uniform_negative_scale() {
+        // When scale along Y is much larger, the minor axis becomes the major axis
+        let center = Vector3::new(0.0, 0.0, 0.0);
+        let major = Vector3::new(10.0, 0.0, 0.0);
+        let normal = Vector3::new(0.0, 0.0, 1.0);
+        let ratio = 0.5; // initial minor length is 5.0
+        let start_param = 0.1;
+        let end_param = 2.0;
+
+        // Scale X by -1, Scale Y by -3: transformed major length is 10, transformed minor length is 15.
+        // So axes must swap!
+        let transform = Transform::from_scaling(Vector3::new(-1.0, -3.0, 1.0))
+            .then(&Transform::from_rotation(Vector3::UNIT_Z, -FRAC_PI_2));
+
+        let (c, m, n, r, s, e) = transform_ellipse_geometry(
+            center,
+            major,
+            normal,
+            ratio,
+            start_param,
+            end_param,
+            &transform,
+        )
+        .expect("transformation should succeed");
+
+        assert!(r <= 1.0, "ratio must be <= 1.0 after axis swap, got {r}");
+        assert!((r - (10.0 / 15.0)).abs() < 1e-9);
+
+        // Verify points match
+        for i in 0..=10 {
+            let frac = i as f64 / 10.0;
+            let orig_t = start_param + frac * (end_param - start_param);
+            let orig_pt = sample_point(center, major, normal, ratio, orig_t);
+            let expected_pt = transform.apply(orig_pt);
+
+            let new_t = s + frac * (e - s);
+            let actual_pt = sample_point(c, m, n, r, new_t);
+
+            let diff = (expected_pt - actual_pt).length();
+            assert!(
+                diff < 1e-9,
+                "Point at frac {frac} differs by {diff}: expected {expected_pt:?}, got {actual_pt:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn reflection_handedness_flip_arc_preservation() {
+        // Reflection across Y axis (scale X by -1, Y by 1)
+        let center = Vector3::new(50.0, 60.0, 0.0);
+        let major = Vector3::new(8.0, 0.0, 0.0);
+        let normal = Vector3::new(0.0, 0.0, 1.0);
+        let ratio = 0.75;
+        let start_param = 0.2;
+        let end_param = 1.8;
+
+        let transform = Transform::from_scaling(Vector3::new(-1.0, 1.0, 1.0))
+            .then(&Transform::from_rotation(Vector3::UNIT_Z, -0.6));
+
+        let (c, m, n, r, s, e) = transform_ellipse_geometry(
+            center,
+            major,
+            normal,
+            ratio,
+            start_param,
+            end_param,
+            &transform,
+        )
+        .expect("transformation should succeed");
+
+        // Normal must be inverted in Z due to reflection
+        assert!(n.z < 0.0, "normal Z should be flipped, got {}", n.z);
+
+        for i in 0..=10 {
+            let frac = i as f64 / 10.0;
+            let orig_t = start_param + frac * (end_param - start_param);
+            let orig_pt = sample_point(center, major, normal, ratio, orig_t);
+            let expected_pt = transform.apply(orig_pt);
+
+            let new_t = s + frac * (e - s);
+            let actual_pt = sample_point(c, m, n, r, new_t);
+
+            let diff = (expected_pt - actual_pt).length();
+            assert!(
+                diff < 1e-9,
+                "Point at frac {frac} differs by {diff}: expected {expected_pt:?}, got {actual_pt:?}"
+            );
+        }
+    }
+}