Merge pull request #4304 from Silvertree2010/fix/h2808-shape-polygon-and-shape-star-sil

Accept whole-number floats and reject invalid counts for polygon, star and spiral

crates/engine/src/cmd/create.rs

@@ -339,11 +339,22 @@ fn ellipse(s: &mut Session, p: &Value) -> Result<Value> {
     add_art_turned(s, "Ellipse", path_kind(live.to_path(), Some(live)), f64_or(p, "rotation", 0.0))
 }
 
+/// The whole-number count `p` gives for `key` (`5` or `5.0`), or `default` when it is left out or
+/// null. A fraction, a negative, a non-finite or a non-number is an error rather than the default.
+fn count_or(p: &Value, key: &str, default: u64, cmd: &str) -> Result<u64> {
+    let n = number_or(p, key, default as f64, cmd)?;
+    if n.is_finite() && n >= 0.0 && n.fract() == 0.0 {
+        Ok(n as u64)
+    } else {
+        Err(bad(cmd, format!("`{key}` must be a whole number of at least 0, not {n}")))
+    }
+}
+
 fn polygon(s: &mut Session, p: &Value) -> Result<Value> {
     let c = Point::new(f64_req(p, "cx", "shape.polygon")?, f64_req(p, "cy", "shape.polygon")?);
     let r = f64_req(p, "radius", "shape.polygon")?.abs();
-    check_numbers(p, &["sides", "rotation"], "shape.polygon")?;
-    let sides = p.get("sides").and_then(Value::as_u64).unwrap_or(6).clamp(3, 1000) as u32;
+    check_numbers(p, &["rotation"], "shape.polygon")?;
+    let sides = count_or(p, "sides", 6, "shape.polygon")?.clamp(3, 1000) as u32;
     let rot = f64_or(p, "rotation", 0.0);
     let xf = Affine::translate(c.to_vec2()) * vectorcraft_geom::rotate_deg(rot);
     let live = LiveShape::Polygon { radius: r, sides, xf, radii: vec![], kinds: vec![], radii_relative: false };
@@ -353,9 +364,9 @@ fn polygon(s: &mut Session, p: &Value) -> Result<Value> {
 fn star(s: &mut Session, p: &Value) -> Result<Value> {
     let c = Point::new(f64_req(p, "cx", "shape.star")?, f64_req(p, "cy", "shape.star")?);
     let r1 = f64_req(p, "radius1", "shape.star")?.abs();
-    check_numbers(p, &["radius2", "points", "rotation"], "shape.star")?;
+    check_numbers(p, &["radius2", "rotation"], "shape.star")?;
     let r2 = f64_or(p, "radius2", r1 / 2.0).abs();
-    let n = p.get("points").and_then(Value::as_u64).unwrap_or(5).clamp(2, 1000) as u32;
+    let n = count_or(p, "points", 5, "shape.star")?.clamp(2, 1000) as u32;
     let rot = f64_or(p, "rotation", 0.0);
     let turn = vectorcraft_geom::rotate_deg(rot);
     // A quarter or whole turn (a Constrain Angle of 90°, say) turns the upright star exactly.
@@ -500,12 +511,12 @@ fn line(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn spiral(s: &mut Session, p: &Value) -> Result<Value> {
     let c = Point::new(f64_req(p, "cx", "shape.spiral")?, f64_req(p, "cy", "shape.spiral")?);
-    check_numbers(p, &["segments"], "shape.spiral")?;
+    let segments = count_or(p, "segments", 10, "shape.spiral")?.clamp(2, 1000) as u32;
     let path = shapes::spiral(
         c,
         f64_req(p, "radius", "shape.spiral")?,
         number_or(p, "decay", 80.0, "shape.spiral")?,
-        p.get("segments").and_then(Value::as_u64).unwrap_or(10).clamp(2, 1000) as u32,
+        segments,
         switch_or(p, "clockwise", true, "shape.spiral")?,
     );
     add_art(s, "Spiral", path_kind(path, None), None)

crates/engine/src/tests/draw2.rs

@@ -2812,3 +2812,33 @@ fn pencil_with_live_preview_off_draws_the_filled_path_on_release() {
     assert!(n.path_data().is_some() && !n.appearance.fill_paint().is_none(), "{:?}", n.appearance);
     assert_eq!(s.doc().unwrap().history.undo.len(), undo + 1, "one undo step");
 }
+
+fn shape_anchors(cmd: &str, params: &serde_json::Value) -> usize {
+    let mut s = session();
+    let r = s.execute(cmd, params).unwrap();
+    let doc = &s.doc().unwrap().doc;
+    doc.node(NodeId(r["id"].as_u64().unwrap())).unwrap().path_data().unwrap().anchors().count()
+}
+
+#[test]
+fn shape_counts_written_as_whole_floats_are_honoured() {
+    let poly = |v: serde_json::Value| shape_anchors("shape.polygon", &json!({"cx": 100, "cy": 100, "radius": 50, "sides": v}));
+    assert_eq!(poly(json!(5.0)), 5);
+    assert_eq!(poly(json!(5)), 5);
+    let star = |v: serde_json::Value| shape_anchors("shape.star", &json!({"cx": 100, "cy": 100, "radius1": 50, "points": v}));
+    assert_eq!(star(json!(7.0)), star(json!(7)));
+    assert_ne!(star(json!(7.0)), star(json!(5)));
+    let spiral = |v: serde_json::Value| shape_anchors("shape.spiral", &json!({"cx": 100, "cy": 100, "radius": 50, "segments": v}));
+    assert_eq!(spiral(json!(4.0)), spiral(json!(4)));
+    assert_ne!(spiral(json!(4.0)), spiral(json!(10)));
+}
+
+#[test]
+fn shape_counts_that_are_not_whole_numbers_are_errors() {
+    for bad in [json!(5.5), json!(-3), json!(-3.0), json!("5")] {
+        let mut s = session();
+        assert!(s.execute("shape.polygon", &json!({"cx": 1, "cy": 1, "radius": 5, "sides": bad})).is_err(), "{bad}");
+        assert!(s.execute("shape.star", &json!({"cx": 1, "cy": 1, "radius1": 5, "points": bad})).is_err(), "{bad}");
+        assert!(s.execute("shape.spiral", &json!({"cx": 1, "cy": 1, "radius": 5, "segments": bad})).is_err(), "{bad}");
+    }
+}