fix(view): circle shape SIGABRT + CircleTo stack overflow

CMakeLists.txt

@@ -269,6 +269,9 @@ if(BUILD_CAPI)
         dasher_add_test(dasher_capi_buffer_lifetime_tests test_capi_buffer_lifetime.cpp)
         dasher_add_test(dasher_lm_correctness_tests test_lm_correctness.cpp)
         dasher_add_test(dasher_view_geometry_tests test_view_geometry.cpp)
+        dasher_add_test(dasher_circle_geometry_tests test_circle_geometry.cpp)
+        # Internal: links DasherCore directly to drive CircleTo/IsSpaceAroundNode.
+        dasher_add_test_internal(dasher_circle_view_internal_tests test_circle_view_internal.cpp)
         dasher_add_test(dasher_input_filter_tests test_input_filters.cpp)
         dasher_add_test(dasher_xml_error_path_tests test_xml_error_paths.cpp)
 

src/DasherCore/DasherViewSquare.cpp

@@ -439,18 +439,35 @@ void CDasherViewSquare::Circle(myint Range, myint y1, myint y2, const ColorPalet
 }
 
 void CDasherViewSquare::CircleTo(myint cy, myint r, myint y1, myint x1, myint y3, myint x3, point dest,
-                                 std::vector<point>& pts, double dXMul) const {
+                                 std::vector<point>& pts, double dXMul, int depth) const {
+    // Termination guards:
+    //  - depth <= 0: hard cap on recursion so the stack can never be exhausted.
+    //    Approximating the remaining arc with a straight line is always fine at
+    //    the subdivision granularity reached by then.
+    //  - y1 >= y3: no forward progress. This happens once integer truncation
+    //    stalls (y1 and y3 within 1 of each other); subdividing further cannot
+    //    move the midpoint, so stop.
+    if (depth <= 0 || y1 >= y3) {
+        pts.push_back(dest);
+        return;
+    }
     myint y2((y1 + y3) / 2);
-    myint x2(static_cast<myint>(sqrt((sq(r) - sq(cy - y2)) * dXMul)));
+    // Clamp before sqrt: degenerate arcs passed in via DasherSpaceArc can have
+    // endpoints outside the circle (|cy - y| > r), making sq(r)-sq(cy-y2)
+    // negative. sqrt of a negative yields NaN, which used to poison `mid`,
+    // defeat the convergence test below, and recurse without bound (stack
+    // overflow / SIGSEGV).
+    const double underSqrt = static_cast<double>(sq(r) - sq(cy - y2)) * dXMul;
+    myint x2(static_cast<myint>(sqrt(underSqrt > 0.0 ? underSqrt : 0.0)));
     point mid;                           // where midpoint of circle/arc should be
     Dasher2Screen(x2, y2, mid.x, mid.y); //(except "midpoint" measured along y axis)
     int lmx = (pts.back().x + dest.x) / 2, lmy = (pts.back().y + dest.y) / 2; // midpoint of straight line
     if (abs(dest.y - pts.back().y) < 2 || abs(mid.x - lmx) + abs(mid.y - lmy) < 2) {
         // okay, use straight line
         pts.push_back(dest);
     } else {
-        CircleTo(cy, r, y1, x1, y2, x2, mid, pts, dXMul);
-        CircleTo(cy, r, y2, x2, y3, x3, dest, pts, dXMul);
+        CircleTo(cy, r, y1, x1, y2, x2, mid, pts, dXMul, depth - 1);
+        CircleTo(cy, r, y2, x2, y3, x3, dest, pts, dXMul, depth - 1);
     }
 }
 #undef sq
@@ -523,8 +540,20 @@ bool CDasherViewSquare::IsSpaceAroundNode(myint y1, myint y2) const {
     if ((maxX < visibleRegion.maxX) || (y1 > visibleRegion.minY) || (y2 < visibleRegion.maxY))
         return true; // space around sq => space around anything smaller!
 
-    // in theory, even if the crosshair is off-screen (!), anything spanning y1-y2 should cover it...
-    DASHER_ASSERT(CoversCrosshair(y2 - y1, y1, y2));
+    // The invariant "any shape spanning y1-y2 covers the crosshair" only holds
+    // for rectangular shapes. Curved/angled shapes (triangles, quadrics,
+    // circles/ellipses) can span the full visible height yet legitimately miss
+    // the crosshair, so CoversCrosshair can validly return false for them and
+    // the assertion must not fire. (Circle mode used to SIGABRT here.)
+    switch (m_pSettingsStore->GetLongParameter(LP_SHAPE_TYPE)) {
+    case Options::DISJOINT_RECTANGLE:
+    case Options::OVERLAPPING_RECTANGLE:
+    case Options::CUBE:
+        DASHER_ASSERT(CoversCrosshair(y2 - y1, y1, y2));
+        break;
+    default:
+        break;
+    }
 
     switch (m_pSettingsStore->GetLongParameter(LP_SHAPE_TYPE)) {
     case Options::DISJOINT_RECTANGLE:

src/DasherCore/DasherViewSquare.h

@@ -104,8 +104,18 @@ class Dasher::CDasherViewSquare : public CDasherView {
     /// dest - point (x2,y2) in screen coords
     /// pts - vector into which to store points; on entry, last element should already be screen-coords of (x1,y1)
     /// dXMul - multiply x coords (in dasher space) by this (i.e. aspect ratio), for ovals
+    /// depth - remaining subdivision budget. CircleTo subdivides the arc
+    ///   recursively; this bounds the recursion so a degenerate arc (NaN
+    ///   midpoint, integer-rounding stall) cannot exhaust the stack. At the
+    ///   budget the remaining arc is approximated by a straight line, which is
+    ///   visually fine at the granularity reached.
     void CircleTo(myint cy, myint r, myint y1, myint x1, myint y3, myint x3, point dest, std::vector<point>& pts,
-                  double dXMul) const;
+                  double dXMul, int depth = kMaxCircleSubdivisionDepth) const;
+    /// Maximum recursion depth for CircleTo(). 2^20 segments is far more than any
+    /// real arc needs (a screen-sized circle resolves to pixel accuracy in ~13
+    /// levels), yet keeps the worst-case call count bounded if convergence ever
+    /// fails. Stack usage at this depth is negligible.
+    static constexpr int kMaxCircleSubdivisionDepth = 20;
     void Circle(myint Range, myint y1, myint y2, const ColorPalette::Color& fillColor,
                 const ColorPalette::Color& outlineColor, int lineWidth) const;
     void Quadric(myint Range, myint lowY, myint highY, const ColorPalette::Color& fillColor,

tests/test_circle_geometry.cpp

@@ -0,0 +1,126 @@
+// test_circle_geometry.cpp
+//
+// REGRESSION TESTS for the circle node-shape (LP_SHAPE_TYPE == CIRCLE, 5).
+//
+// Two crashes historically affected the circular/compass Dasher layout, both
+// in CDasherViewSquare (src/DasherCore/DasherViewSquare.cpp):
+//
+//   Bug 1 (SIGABRT): IsSpaceAroundNode() asserts CoversCrosshair(...) for any
+//     node that spans the visible region. That invariant only holds for
+//     rectangular shapes — a circle/ellipse can span the full height yet
+//     legitimately miss the crosshair, so the assert is unsound for circles
+//     (and triangles/quadrics).
+//
+//   Bug 2 (SIGSEGV / stack overflow): CircleTo() recursively subdivides an arc
+//     with no recursion-depth bound. Degenerate endpoints (integer-rounding
+//     stall, or sqrt of a negative -> NaN) prevent the convergence test from
+//     ever firing and it recurses until the stack is exhausted.
+//
+// These tests drive normal rendering with LP_SHAPE_TYPE=CIRCLE. If either bug
+// is present, the test *process* is killed (abort/segv), so simply reaching
+// the assertions is the proof of the fix. We additionally check that circle
+// geometry actually emits polygon (line-segment) draw commands.
+
+#include "test_common.h"
+
+#include <cmath>
+
+namespace {
+
+// LP_SHAPE_TYPE key (Options::CIRCLE == 5). Resolved lazily inside each test,
+// NOT at static init — dasher_find_parameter_key needs the engine registered,
+// and doctest's context isn't available until main() runs.
+static int shape_type_key() {
+    const int k = dasher_find_parameter_key("LP_SHAPE_TYPE");
+    REQUIRE(k > 0);
+    return k;
+}
+
+// Counts opcode-2 (Line) segments emitted across `frames` frames. The C-API
+// screen renders Polygon() (used by the circle shape) as a fan of opcode-2
+// line segments, so this is a proxy for "circle shapes were actually drawn".
+int count_line_segments(dasher_ctx* ctx, int frames) {
+    int lines = 0;
+    for (int i = 0; i < frames; ++i) {
+        int* cmds = nullptr;
+        int cc = 0;
+        char** strs = nullptr;
+        int sc = 0;
+        dasher_frame(ctx, 1000 + i * 16, &cmds, &cc, &strs, &sc);
+        for (int j = 0; j + 5 < cc; j += 6) {
+            if (cmds[j] == 2) ++lines;
+        }
+    }
+    return lines;
+}
+
+} // namespace
+
+// ---------------------------------------------------------------------------
+// Bug 1 + Bug 2: rendering a single frame in circle mode must not abort/crash.
+// ---------------------------------------------------------------------------
+
+TEST_CASE("circle/first frame does not crash") {
+    ScopedContext ctx(800, 600);
+    dasher_set_long_parameter(ctx, shape_type_key(), 5); // Options::CIRCLE
+
+    // A single frame calls NewRender -> IsSpaceAroundNode (Bug 1) and, for
+    // nodes drawn as circles, Circle -> CircleTo (Bug 2).
+    int* cmds = nullptr;
+    int cc = 0;
+    char** strs = nullptr;
+    int sc = 0;
+    dasher_frame(ctx, 1000, &cmds, &cc, &strs, &sc);
+
+    // The frame must have produced *some* drawing.
+    CHECK(cc > 0);
+
+    dasher_set_long_parameter(ctx, shape_type_key(), 1); // restore default
+}
+
+// ---------------------------------------------------------------------------
+// Bug 2: deep recursion. Drive the engine forward so nodes grow small, which
+// makes the per-node circles shrink and stresses CircleTo's subdivision.
+// ---------------------------------------------------------------------------
+
+TEST_CASE("circle/many forward frames do not overflow the stack") {
+    ScopedContext ctx(800, 600);
+    dasher_set_long_parameter(ctx, shape_type_key(), 5); // Options::CIRCLE
+
+    dasher_set_speed_percent(ctx, 250);
+    dasher_mouse_move(ctx, 720.0f, 300.0f);
+    dasher_mouse_down(ctx);
+
+    // Plenty of frames at a high bit-rate to force deep node trees / small
+    // circles. If CircleTo recurses without bound this segfaults.
+    const int lines = count_line_segments(ctx, 60);
+
+    dasher_mouse_up(ctx);
+
+    CHECK(lines > 0); // circle geometry actually drew something
+
+    dasher_set_long_parameter(ctx, shape_type_key(), 1); // restore default
+}
+
+// ---------------------------------------------------------------------------
+// The setting must be switchable at runtime without leaving the engine in a
+// bad state (covers the parameter-change -> ComputeScaleFactor path).
+// ---------------------------------------------------------------------------
+
+TEST_CASE("circle/switching into and out of circle mode is stable") {
+    ScopedContext ctx(800, 600);
+
+    dasher_set_long_parameter(ctx, shape_type_key(), 5);
+    run_frames(ctx, 5);
+    CHECK(dasher_get_long_parameter(ctx, shape_type_key()) == 5);
+
+    dasher_set_long_parameter(ctx, shape_type_key(), 1);
+    run_frames(ctx, 5);
+    CHECK(dasher_get_long_parameter(ctx, shape_type_key()) == 1);
+
+    // back to circle, then back to default, all while ticking frames
+    dasher_set_long_parameter(ctx, shape_type_key(), 5);
+    run_frames(ctx, 5);
+    dasher_set_long_parameter(ctx, shape_type_key(), 1);
+    run_frames(ctx, 5);
+}

tests/test_circle_view_internal.cpp

@@ -0,0 +1,145 @@
+// test_circle_view_internal.cpp
+//
+// INTERNAL unit tests for the circle node-shape fixes in CDasherViewSquare,
+// exercising the buggy code paths directly (no engine/CAPI involvement) so the
+// reproduction is deterministic.
+//
+//   Bug 2 (deterministic here): DasherSpaceArc() -> CircleTo() used to recurse
+//   without bound when an arc endpoint lay outside the circle (|y - cy| > r),
+//   because sqrt(negative) -> NaN poisoned the convergence test. This drove the
+//   game-mode brachistochrone (GameModule.cpp:235) into a stack overflow. The
+//   degenerate args below are constructed so |cy - y_mid| > r for the recursive
+//   midpoint, which is exactly the poisoning condition.
+//
+//   Bug 1 (smoke): IsSpaceAroundNode() used to DASHER_ASSERT(CoversCrosshair)
+//   for every shape. That invariant is false for non-rectangular shapes, so in
+//   circle mode the assert is a latent SIGABRT. We can't easily build a node
+//   that both spans the visible region AND misses the crosshair on a default
+//   screen, but we can at least confirm circle-mode IsSpaceAroundNode runs
+//   without aborting across a sweep of bounds (the assert site is reached for
+//   the root-sized span).
+//
+// Built via dasher_add_test_internal (links DasherCore directly).
+
+#include "test_common.h"
+
+#include "DasherCore/ColorPalette.h"
+#include "DasherCore/DasherScreen.h"
+#include "DasherCore/DasherViewSquare.h"
+#include "DasherCore/Parameters.h"
+#include "DasherCore/SettingsStore.h"
+
+#include <atomic>
+
+using namespace Dasher;
+
+namespace {
+
+// Minimal CDasherScreen: records how many points it was asked to draw and
+// otherwise does nothing. CircleTo's output lands in Polyline/Polygon.
+class StubScreen : public CDasherScreen {
+  public:
+    StubScreen(screenint w, screenint h) : CDasherScreen(w, h) {}
+
+    std::atomic<int> polyline_points{0};
+    std::atomic<int> polygon_points{0};
+
+    std::pair<screenint, screenint> TextSize(Label*, unsigned int) override { return {1, 1}; }
+    void DrawString(Label*, screenint, screenint, unsigned int, const ColorPalette::Color&) override {}
+    void DrawRectangle(screenint, screenint, screenint, screenint, const ColorPalette::Color&,
+                       const ColorPalette::Color&, int) override {}
+    void DrawCircle(screenint, screenint, screenint, const ColorPalette::Color&, const ColorPalette::Color&,
+                    int) override {}
+    void Polyline(point* pts, int n, int, const ColorPalette::Color&) override {
+        if (pts && n > 0) polyline_points += n;
+    }
+    void Polygon(point* pts, int n, const ColorPalette::Color&, const ColorPalette::Color&, int) override {
+        if (pts && n > 0) polygon_points += n;
+    }
+    void Display() override {}
+    bool IsPointVisible(screenint, screenint) override { return true; }
+};
+
+struct ViewFixture {
+    CSettingsStore store;
+    StubScreen screen{800, 600};
+    std::unique_ptr<CDasherViewSquare> view;
+
+    explicit ViewFixture(Options::ScreenOrientations orient = Options::LeftToRight) {
+        // Populate the default parameter table (LP_SHAPE_TYPE, BP_NONLINEAR_Y,
+        // LP_GEOMETRY, ...). CSettingsStore::LoadPersistent is protected, but
+        // AddParameters is public and takes the same defaults table.
+        store.AddParameters(Settings::parameter_defaults);
+        store.SetLongParameter(LP_SHAPE_TYPE, Options::CIRCLE);
+        view = std::make_unique<CDasherViewSquare>(&store, &screen, orient);
+    }
+};
+
+} // namespace
+
+// ---------------------------------------------------------------------------
+// Bug 2: a degenerate arc (endpoint outside the circle) must terminate.
+// Without the fix this is UB (NaN -> integer cast) and, depending on the
+// platform/orientation, recurses until the stack overflows (SIGSEGV).
+// ---------------------------------------------------------------------------
+
+TEST_CASE("circle/DasherSpaceArc degenerate arc does not recurse forever") {
+    for (auto orient : {Options::LeftToRight, Options::TopToBottom}) {
+        ViewFixture f(orient);
+
+        // cy = ORIGIN_Y, r small. Endpoint y2 is far outside [cy-r, cy+r], so
+        // the recursive midpoint satisfies |cy - y_mid| > r => sqrt(negative)
+        // => NaN. r is chosen <= ORIGIN_X so DasherSpaceArc skips its
+        // bisect-at-apex branch and feeds the degenerate endpoints to CircleTo.
+        const myint cy = CDasherModel::ORIGIN_Y;
+        const myint r = 100; // well inside ORIGIN_X (2048)
+        const myint y1 = CDasherModel::ORIGIN_Y;
+        const myint y2 = CDasherModel::ORIGIN_Y + 100000; // far outside the circle
+
+        const ColorPalette::Color guideColor{255, 255, 255, 255};
+
+        // If CircleTo recurses without bound this never returns.
+        f.view->DasherSpaceArc(cy, r, CDasherModel::ORIGIN_X, y1, 0, y2, guideColor, 1);
+
+        // It must actually have produced geometry.
+        CHECK(f.screen.polyline_points.load() > 0);
+    }
+}
+
+// A second degenerate configuration: arc spanning across cy with r > ORIGIN_X
+// (exercises DasherSpaceArc's apex-bisection branch too).
+TEST_CASE("circle/DasherSpaceArc large-radius cross-centre arc terminates") {
+    ViewFixture f;
+
+    const myint cy = CDasherModel::ORIGIN_Y - 5000;
+    const myint r = CDasherModel::ORIGIN_X + 1000; // > ORIGIN_X => apex branch taken
+    // y1 and y2 on opposite sides of cy => ((y1<cy)^(y2<cy)) is true.
+    const myint y1 = cy - 40000; // outside circle: |y1 - cy| = 40000 > r
+    const myint y2 = cy + 40000;
+
+    const ColorPalette::Color guideColor{255, 0, 0, 255};
+
+    f.view->DasherSpaceArc(cy, r, CDasherModel::ORIGIN_X, y1, 0, y2, guideColor, 2);
+    CHECK(f.screen.polyline_points.load() > 0);
+}
+
+// ---------------------------------------------------------------------------
+// Bug 1 smoke: circle-mode IsSpaceAroundNode must not abort (debug builds).
+// A root-sized span reaches the (now guarded) assert site.
+// ---------------------------------------------------------------------------
+
+TEST_CASE("circle/IsSpaceAroundNode runs without aborting") {
+    ViewFixture f;
+
+    // Root-sized node: spans the whole visible region, so the early-return in
+    // IsSpaceAroundNode is NOT taken and the (formerly unconditional, now
+    // shape-guarded) assert site is reached.
+    (void)f.view->IsSpaceAroundNode(0, CDasherModel::MAX_Y); // must return, not abort
+
+    // A range of non-trivial spans should also evaluate cleanly.
+    for (myint span : {CDasherModel::MAX_Y, CDasherModel::MAX_Y / 2, CDasherModel::MAX_Y / 4}) {
+        const myint y1 = CDasherModel::ORIGIN_Y - span / 2;
+        const myint y2 = CDasherModel::ORIGIN_Y + span / 2;
+        (void)f.view->IsSpaceAroundNode(y1, y2); // must return, not abort
+    }
+}