diff --git a/CMakeLists.txt b/CMakeLists.txt index fdcb6784..8ab98c56 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/src/DasherCore/DasherViewSquare.cpp b/src/DasherCore/DasherViewSquare.cpp index 418b791b..32bd0ede 100644 --- a/src/DasherCore/DasherViewSquare.cpp +++ b/src/DasherCore/DasherViewSquare.cpp @@ -439,9 +439,26 @@ 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& pts, double dXMul) const { + std::vector& 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(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(sq(r) - sq(cy - y2)) * dXMul; + myint x2(static_cast(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 @@ -449,8 +466,8 @@ void CDasherViewSquare::CircleTo(myint cy, myint r, myint y1, myint x1, myint y3 // 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: diff --git a/src/DasherCore/DasherViewSquare.h b/src/DasherCore/DasherViewSquare.h index 2da7f7de..a740360a 100644 --- a/src/DasherCore/DasherViewSquare.h +++ b/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& 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, diff --git a/tests/test_circle_geometry.cpp b/tests/test_circle_geometry.cpp new file mode 100644 index 00000000..ec011292 --- /dev/null +++ b/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 + +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); +} diff --git a/tests/test_circle_view_internal.cpp b/tests/test_circle_view_internal.cpp new file mode 100644 index 00000000..988c6ddf --- /dev/null +++ b/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 + +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 polyline_points{0}; + std::atomic polygon_points{0}; + + std::pair 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 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(&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 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 + } +}