From 25c5c05b4025873da574c8f8a814a7bdc4830ecd Mon Sep 17 00:00:00 2001 From: Philippe Leduc Date: Thu, 9 Jul 2026 18:21:03 +0200 Subject: [PATCH] Various fixes and hardening: - engine: build() fails on a scheduled step with an unwired required input (declare_input gains an `optional` flag; dt_in marked optional) - editor: enforce validate_connection + can_connect (reject same-node and input fan-in); push active mode to the engine after build/rebuild - autosave: flush-check the stream before rename; keep the autosave file when a recovery load fails - format: keep V2 at version 2 for pre-beta; re-stamp example .piper files - style: remove all production ternaries (shared Step::resolve_dt helper) --- .gitignore | 1 + app/autosave.cc | 12 +- app/canvas_adapter.cc | 31 +++ app/include/piper/app/main_window.h | 6 + app/main_window.cc | 188 +++++++++++++----- core/include/piper/serialize_v2.h | 2 +- core/src/builtin_nodes.cc | 9 +- core/src/serialize_v2.cc | 8 +- engine/include/piper/engine/step.h | 22 +- engine/src/engine.cc | 20 ++ engine/src/step/low_pass.h | 6 +- engine/src/step/pid.h | 6 +- engine/src/step/sin_wave.h | 6 +- examples/am_radio/am_radio.piper | 2 +- examples/filter_demo/filter_demo.piper | 2 +- .../motor_control_dual_jacobian.piper | 2 +- examples/pid_demo/pid_demo.piper | 2 +- tests/engine/build_smoke-t.cc | 38 ++++ 18 files changed, 281 insertions(+), 82 deletions(-) diff --git a/.gitignore b/.gitignore index ff915a3..95e7d66 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,4 @@ imgui.ini CMakeSettings.json .cache/ compile_commands.json +/TODO.md diff --git a/app/autosave.cc b/app/autosave.cc index c0e26e4..e53c302 100644 --- a/app/autosave.cc +++ b/app/autosave.cc @@ -43,10 +43,20 @@ namespace piper::studio std::string const json = piper::v2::serialize(doc.graph, doc.pipeline_name); { std::ofstream f(tmp); - if (not f.is_open() or not (f << json)) + if (not f.is_open()) { return; } + f << json; + // close() flushes; a failed flush (e.g. disk full) sets + // failbit. Checking after close catches a truncated write + // before it renames over the last good autosave. + f.close(); + if (not f) + { + std::filesystem::remove(tmp, ec); + return; + } } std::filesystem::rename(tmp, path, ec); if (ec) diff --git a/app/canvas_adapter.cc b/app/canvas_adapter.cc index 946354e..48b362c 100644 --- a/app/canvas_adapter.cc +++ b/app/canvas_adapter.cc @@ -156,6 +156,37 @@ namespace piper::studio { return canvas::Connect::TypeMismatch; } + + PinRef const ra = pin_id_to_ref(a.id); + PinRef const rb = pin_id_to_ref(b.id); + if (not ra.attr.empty() and ra.node == rb.node) + { + return canvas::Connect::SameNode; + } + + // An input pin may hold only one incoming link. Reject a second + // source so fan-in can't silently commit (the engine would then + // last-writer-win). + canvas::Pin const* input = nullptr; + if (a.kind == canvas::PinKind::Input) + { + input = &a; + } + else if (b.kind == canvas::PinKind::Input) + { + input = &b; + } + if (input != nullptr) + { + PinRef const in_ref = pin_id_to_ref(input->id); + for (auto const& l : graph_.links()) + { + if (l.to == in_ref) + { + return canvas::Connect::AlreadyConnected; + } + } + } return canvas::Connect::Allow; } diff --git a/app/include/piper/app/main_window.h b/app/include/piper/app/main_window.h index 2cce006..c231939 100644 --- a/app/include/piper/app/main_window.h +++ b/app/include/piper/app/main_window.h @@ -126,6 +126,12 @@ namespace piper::studio // O(N + L) sweep; called once per draw(). void recompute_lints(Document& doc); + // Required (non-optional) input pins with no incoming link, as + // {node, attr}. Feeds both the lint and the Run gate -- running a + // graph with an unwired required input would throw at tick. + std::vector> + unwired_required_inputs(Document const& doc) const; + void align_selection(Document& doc, AlignMode mode); void distribute_selection(Document& doc, bool horizontal); diff --git a/app/main_window.cc b/app/main_window.cc index b76ab51..4ef8f7d 100644 --- a/app/main_window.cc +++ b/app/main_window.cc @@ -31,6 +31,7 @@ #include "piper/builtin_nodes.h" #include "piper/canvas/event.h" #include "piper/commands.h" +#include "piper/connect.h" #include "piper/serialize_v2.h" namespace piper::studio @@ -283,7 +284,11 @@ namespace piper::studio } std::error_code ec; std::string const abs = std::filesystem::absolute(path, ec).string(); - std::string const key = ec ? path : abs; + std::string key = abs; + if (ec) + { + key = path; + } recent_files_.erase( std::remove(recent_files_.begin(), recent_files_.end(), key), recent_files_.end()); @@ -1312,9 +1317,11 @@ namespace piper::studio // modes panel. if (not doc.graph.mode_profiles().empty()) { - char const* preview = doc.active_mode_profile.empty() - ? "(none)" - : doc.active_mode_profile.c_str(); + char const* preview = "(none)"; + if (not doc.active_mode_profile.empty()) + { + preview = doc.active_mode_profile.c_str(); + } ImGui::TextUnformatted("Profile"); ImGui::SameLine(); ImGui::SetNextItemWidth(-FLT_MIN); @@ -1458,7 +1465,11 @@ namespace piper::studio auto const it = doc.probe_history.find(n.id); ProbeView pv; pv.name = &n.name; - pv.hist = (it == doc.probe_history.end()) ? &empty : &it->second; + pv.hist = ∅ + if (it != doc.probe_history.end()) + { + pv.hist = &it->second; + } pv.color = probe_palette[probes.size() % palette_size]; probes.push_back(pv); } @@ -1466,7 +1477,11 @@ namespace piper::studio for (auto const& p : probes) { - float const last = p.hist->empty() ? 0.0f : p.hist->back(); + float last = 0.0f; + if (not p.hist->empty()) + { + last = p.hist->back(); + } ImGui::PushStyleColor(ImGuiCol_Text, p.color); ImGui::Text("%s = %.4g", p.name->c_str(), last); ImGui::PopStyleColor(); @@ -1583,6 +1598,8 @@ namespace piper::studio doc.engine.reset(); return; } + // Run the mode selected in the UI; build() defaults to base mode. + doc.engine->set_mode(doc.active_mode_profile); doc.engine_built_revision = doc.command_stack.revision(); doc.engine_running = true; // Fresh capture buffer on start: avoids a visual discontinuity @@ -1627,6 +1644,7 @@ namespace piper::studio doc.engine_running = false; return; } + doc.engine->set_mode(doc.active_mode_profile); doc.engine_built_revision = doc.command_stack.revision(); doc.probe_latest.clear(); doc.probe_history.clear(); @@ -1699,6 +1717,53 @@ namespace piper::studio stage_play_next_advance_ = now + std::chrono::milliseconds(2000); } + std::vector> + MainWindow::unwired_required_inputs(Document const& doc) const + { + std::set> connected; + for (auto const& l : doc.graph.links()) + { + connected.insert({ l.to.node, l.to.attr }); + connected.insert({ l.from.node, l.from.attr }); + } + + std::vector> missing; + for (auto const& n : doc.graph.nodes()) + { + // Optional inputs set is_optional in the spec and the step + // falls back when unwired (e.g. dt_in on sin_wave/low_pass/pid). + NodeType const* spec = registry_.find(n.type); + for (auto const& a : n.attrs) + { + if (a.role != AttributeSpec::Role::Input) + { + continue; + } + bool optional = false; + if (spec != nullptr) + { + for (auto const& s : spec->attributes) + { + if (s.name == a.name and s.is_optional) + { + optional = true; + break; + } + } + } + if (optional) + { + continue; + } + if (connected.count({ n.id, a.name }) == 0) + { + missing.push_back({ n.id, a.name }); + } + } + } + return missing; + } + void MainWindow::recompute_lints(Document& doc) { doc.lint_diagnostics.clear(); @@ -1760,44 +1825,6 @@ namespace piper::studio doc.lint_diagnostics.push_back(d); } - // Skip optional inputs: their spec sets is_optional and the - // step is expected to fall back when unwired (e.g. dt_in - // on sin_wave / low_pass / pid). - NodeType const* spec = registry_.find(n.type); - for (auto const& a : n.attrs) - { - if (a.role != AttributeSpec::Role::Input) - { - continue; - } - bool optional = false; - if (spec != nullptr) - { - for (auto const& s : spec->attributes) - { - if (s.name == a.name and s.is_optional) - { - optional = true; - break; - } - } - } - if (optional) - { - continue; - } - if (connected.count({ n.id, a.name }) == 0) - { - Diagnostic d; - d.kind = Diagnostic::Kind::SchemaError; - d.message = "input '" + n.name + "." + a.name - + "' has no source"; - d.node_id = n.id; - d.attr_name = a.name; - doc.lint_diagnostics.push_back(d); - } - } - // Only flag missing per-node labels for nodes whose type // advertises mode labels (preset3 etc.). Ordinary nodes // don't dispatch on labels, so "no entry" means the engine @@ -1815,6 +1842,21 @@ namespace piper::studio doc.lint_diagnostics.push_back(d); } } + + for (auto const& [nid, attr] : unwired_required_inputs(doc)) + { + std::string nname; + if (Node const* n = doc.graph.find_node(nid); n != nullptr) + { + nname = n->name; + } + Diagnostic d; + d.kind = Diagnostic::Kind::SchemaError; + d.message = "input '" + nname + "." + attr + "' has no source"; + d.node_id = nid; + d.attr_name = attr; + doc.lint_diagnostics.push_back(d); + } } void MainWindow::add_node_at(Document& doc, @@ -2322,8 +2364,13 @@ namespace piper::studio { if (d->dirty) { ++dirty_count; } } + char const* plural = "s"; + if (dirty_count == 1) + { + plural = ""; + } ImGui::Text("%d document%s with unsaved changes:", - dirty_count, (dirty_count == 1) ? "" : "s"); + dirty_count, plural); for (auto const& d : documents_) { if (not d->dirty) { continue; } @@ -2538,6 +2585,10 @@ namespace piper::studio adoc.active_mode_profile.clear(); adoc.adapter.set_active_mode_profile(adoc.active_mode_profile); adoc.adapter.rebuild(); + if (adoc.engine != nullptr) + { + adoc.engine->set_mode(adoc.active_mode_profile); + } } for (auto const& mp : adoc.graph.mode_profiles()) { @@ -2548,6 +2599,10 @@ namespace piper::studio adoc.adapter.set_active_mode_profile(adoc.active_mode_profile); adoc.adapter.rebuild(); adoc.lint_dirty = true; + if (adoc.engine != nullptr) + { + adoc.engine->set_mode(adoc.active_mode_profile); + } } } ImGui::EndCombo(); @@ -3307,20 +3362,27 @@ namespace piper::studio } if (not to_open.empty()) { + std::string const fname = + std::filesystem::path(to_open).filename().string(); if (load_file(to_open)) { - push_toast(ToastLevel::Info, - "Recovered " + std::filesystem::path(to_open) - .filename().string()); + push_toast(ToastLevel::Info, "Recovered " + fname); + // Only drop the autosave once its contents are safely + // loaded -- a failed load must keep the sole copy. + std::error_code ec; + std::filesystem::remove(to_open, ec); + autosave_pending_.erase( + std::remove(autosave_pending_.begin(), autosave_pending_.end(), to_open), + autosave_pending_.end()); + if (autosave_pending_.empty()) + { + ImGui::CloseCurrentPopup(); + } } - std::error_code ec; - std::filesystem::remove(to_open, ec); - autosave_pending_.erase( - std::remove(autosave_pending_.begin(), autosave_pending_.end(), to_open), - autosave_pending_.end()); - if (autosave_pending_.empty()) + else { - ImGui::CloseCurrentPopup(); + push_toast(ToastLevel::Error, + "Could not recover " + fname + " -- autosave kept"); } } if (not to_discard.empty()) @@ -3898,6 +3960,22 @@ namespace piper::studio { break; } + // Authoritative domain check for node<->node links + // (same-node, kind/type mismatch, fan-in). Label + // endpoints resolve type/direction at build via the + // label cluster, so skip the node-only validator there. + bool const involves_label = + doc.graph.find_label(from.node) != nullptr + or doc.graph.find_label(to.node) != nullptr; + if (not involves_label) + { + piper::TypeCheck const tc; + if (piper::validate_connection(doc.graph, from, to, tc) + != piper::Connect::Allow) + { + break; + } + } // Either endpoint may be a label (wildcard pin); the // declared data_type comes from whichever side is a // real Node attribute. diff --git a/core/include/piper/serialize_v2.h b/core/include/piper/serialize_v2.h index 679a779..1945207 100644 --- a/core/include/piper/serialize_v2.h +++ b/core/include/piper/serialize_v2.h @@ -13,7 +13,7 @@ namespace piper::v2 { - constexpr int format_version = 3; + constexpr int format_version = 2; constexpr int min_supported_version = 2; // ---- Graph bundle (one or many pipelines) ---- diff --git a/core/src/builtin_nodes.cc b/core/src/builtin_nodes.cc index 59b2136..318529d 100644 --- a/core/src/builtin_nodes.cc +++ b/core/src/builtin_nodes.cc @@ -129,8 +129,13 @@ namespace piper + " source. Use Engine::input<" + data_type_string() + ">(name). min/max bound the slider in the editor's " "Live panel; the engine itself does no clamping."; - char const* const min_default = std::is_floating_point_v ? "-1.0" : "-100"; - char const* const max_default = std::is_floating_point_v ? "1.0" : "100"; + char const* min_default = "-100"; + char const* max_default = "100"; + if constexpr (std::is_floating_point_v) + { + min_default = "-1.0"; + max_default = "1.0"; + } nt.attributes = { { "name", "string", AttributeSpec::Role::Member, "" }, { "min", data_type_string(), AttributeSpec::Role::Member, min_default }, diff --git a/core/src/serialize_v2.cc b/core/src/serialize_v2.cc index 6b80673..f330f60 100644 --- a/core/src/serialize_v2.cc +++ b/core/src/serialize_v2.cc @@ -931,11 +931,9 @@ namespace piper::v2 } int version = doc.value("version", 0); - // Loader accepts versions in [min_supported_version, - // format_version]. v2 -> v3: labels were promoted from - // label_in/label_out node entries to first-class Label - // entities; the migration in parse_pipeline_body picks up - // label_in/label_out node entries from v2 saves regardless. + // parse_pipeline_body picks up legacy label_in/label_out node + // entries and promotes them to first-class Labels regardless of + // the version integer. if (version < min_supported_version or version > format_version) { throw std::runtime_error( diff --git a/engine/include/piper/engine/step.h b/engine/include/piper/engine/step.h index 12145c4..8013deb 100644 --- a/engine/include/piper/engine/step.h +++ b/engine/include/piper/engine/step.h @@ -90,6 +90,10 @@ namespace piper::engine struct InputSlot { bool (*matches)(std::any const&){nullptr}; + // Optional inputs may be left unwired; the step falls back (it + // guards reads with has_input). build() only errors on unwired + // required inputs. + bool optional{false}; }; // Per-step runtime block. Step::io points at this; Engine owns it. @@ -148,6 +152,19 @@ namespace piper::engine return io_->inputs.count(std::string(name)) != 0; } + // Read the optional "dt_in" pin as a double, falling back to the + // step's member timestep when unwired. Shared by the time- + // stepped steps (sin_wave, low_pass, pid). + template + double resolve_dt(double dt_member) const + { + if (has_input("dt_in")) + { + return static_cast(input("dt_in")); + } + return dt_member; + } + // Read or write a published output. Throws if the name is not a // declared output or if T does not match the published type. template @@ -187,13 +204,14 @@ namespace piper::engine // published type at link wire time via std::any_cast on the // producer's ref_any. template - void declare_input(std::string_view name) + void declare_input(std::string_view name, bool optional = false) { io_->input_slots[std::string(name)] = InputSlot{ [](std::any const& a) { return std::any_cast>(&a) != nullptr; - } + }, + optional }; } diff --git a/engine/src/engine.cc b/engine/src/engine.cc index c9c559b..9e200db 100644 --- a/engine/src/engine.cc +++ b/engine/src/engine.cc @@ -352,6 +352,26 @@ namespace piper::engine block.active_stage_indices.assign(active.begin(), active.end()); std::sort(block.active_stage_indices.begin(), block.active_stage_indices.end()); + + // A scheduled step reading a declared, non-optional input + // that no link wired throws std::out_of_range on first read + // -- fail the build so no host (studio or embedder) ticks + // into a crash. + if (not active.empty()) + { + for (auto const& [in_name, slot] : block.input_slots) + { + if (slot.optional or block.inputs.count(in_name) != 0) + { + continue; + } + result.diagnostics.push_back( + make_build_diagnostic(BuildDiagnostic::Kind::UnresolvedInput, + "required input '" + in_name + "' has no wired source", + node.id, in_name)); + has_error = true; + } + } } // ---- Per-stage topo sort (Kahn) ---- diff --git a/engine/src/step/low_pass.h b/engine/src/step/low_pass.h index 5ee5909..6ba5cc6 100644 --- a/engine/src/step/low_pass.h +++ b/engine/src/step/low_pass.h @@ -17,7 +17,7 @@ namespace piper::engine::step void declare_io() override { declare_input("in"); - declare_input("dt_in"); + declare_input("dt_in", /*optional=*/true); declare_output("out", out_); cutoff_ = std::stod(member("cutoff")); dt_member_ = std::stod(member("dt")); @@ -26,9 +26,7 @@ namespace piper::engine::step void compute(Stage) override { T const in = input("in"); - double const dt = has_input("dt_in") - ? static_cast(input("dt_in")) - : dt_member_; + double const dt = resolve_dt(dt_member_); double const tau = 1.0 / (2.0 * std::numbers::pi_v * cutoff_); double const alpha = dt / (tau + dt); double const next = static_cast(out_) diff --git a/engine/src/step/pid.h b/engine/src/step/pid.h index 1dbea15..6f81cbe 100644 --- a/engine/src/step/pid.h +++ b/engine/src/step/pid.h @@ -47,7 +47,7 @@ namespace piper::engine::step declare_input("kp"); declare_input("ki"); declare_input("kd"); - declare_input("dt_in"); + declare_input("dt_in", /*optional=*/true); declare_output("out", out_); dt_member_ = std::stod(member("dt")); out_min_ = std::stod(member("out_min")); @@ -63,9 +63,7 @@ namespace piper::engine::step T const ki = input("ki"); T const kd = input("kd"); - double const dt = has_input("dt_in") - ? static_cast(input("dt_in")) - : dt_member_; + double const dt = resolve_dt(dt_member_); T const e = static_cast(sp - m); diff --git a/engine/src/step/sin_wave.h b/engine/src/step/sin_wave.h index d0206a8..343cd02 100644 --- a/engine/src/step/sin_wave.h +++ b/engine/src/step/sin_wave.h @@ -21,7 +21,7 @@ namespace piper::engine::step void declare_io() override { - declare_input("dt_in"); + declare_input("dt_in", /*optional=*/true); declare_output("out", out_); frequency_ = std::stod(member("frequency")); amplitude_ = std::stod(member("amplitude")); @@ -31,9 +31,7 @@ namespace piper::engine::step void compute(Stage) override { - double const dt = has_input("dt_in") - ? static_cast(input("dt_in")) - : dt_member_; + double const dt = resolve_dt(dt_member_); double const angle = 2.0 * std::numbers::pi_v * frequency_ * t_ + phase_; out_ = static_cast(amplitude_ * std::sin(angle)); t_ += dt; diff --git a/examples/am_radio/am_radio.piper b/examples/am_radio/am_radio.piper index 7f263ec..ef1bbf3 100644 --- a/examples/am_radio/am_radio.piper +++ b/examples/am_radio/am_radio.piper @@ -687,5 +687,5 @@ ] } ], - "version": 3 + "version": 2 } \ No newline at end of file diff --git a/examples/filter_demo/filter_demo.piper b/examples/filter_demo/filter_demo.piper index a8e35ce..0d413cb 100644 --- a/examples/filter_demo/filter_demo.piper +++ b/examples/filter_demo/filter_demo.piper @@ -317,5 +317,5 @@ ] } ], - "version": 3 + "version": 2 } diff --git a/examples/motor_control_dual_jacobian/motor_control_dual_jacobian.piper b/examples/motor_control_dual_jacobian/motor_control_dual_jacobian.piper index 10c852f..aebc243 100644 --- a/examples/motor_control_dual_jacobian/motor_control_dual_jacobian.piper +++ b/examples/motor_control_dual_jacobian/motor_control_dual_jacobian.piper @@ -330,5 +330,5 @@ ] } ], - "version": 3 + "version": 2 } \ No newline at end of file diff --git a/examples/pid_demo/pid_demo.piper b/examples/pid_demo/pid_demo.piper index 5e6f994..8d1986f 100644 --- a/examples/pid_demo/pid_demo.piper +++ b/examples/pid_demo/pid_demo.piper @@ -550,5 +550,5 @@ ] } ], - "version": 3 + "version": 2 } \ No newline at end of file diff --git a/tests/engine/build_smoke-t.cc b/tests/engine/build_smoke-t.cc index 2632028..eeab874 100644 --- a/tests/engine/build_smoke-t.cc +++ b/tests/engine/build_smoke-t.cc @@ -63,6 +63,44 @@ TEST(EngineBuild, BuildSucceedsForLinearGraph) EXPECT_EQ(e.stages().size(), 1u); } +TEST(EngineBuild, UnwiredRequiredInputFailsBuild) +{ + NodeRegistry nr; + piper::register_builtin_nodes(nr); + + Graph g; + g.add_stage(piper::Stage{ "control", 0xFFFFFFFFu }); + + auto const* lp = nr.find("low_pass"); + auto const* pr = nr.find("external_output"); + auto lp_id = g.add_node(*lp, "filter", "control", Point{ 0.0f, 0.0f }); + auto pr_id = g.add_node(*pr, "probe", "control", Point{ 1.0f, 0.0f }); + + // Wire only the output; low_pass "in" (required) is left unwired. + // "dt_in" is also unwired but optional, so it must not be flagged. + g.add_link(PinRef{ lp_id, "out" }, PinRef{ pr_id, "in" }, "float"); + + StepRegistry sr; + piper::engine::register_builtin_steps(sr); + + Engine e; + auto res = e.build(g, sr); + + EXPECT_FALSE(res.ok); + bool flagged_in = false; + bool flagged_dt_in = false; + for (auto const& d : res.diagnostics) + { + if (d.kind == BuildDiagnostic::Kind::UnresolvedInput) + { + if (d.attr_name == "in") { flagged_in = true; } + if (d.attr_name == "dt_in") { flagged_dt_in = true; } + } + } + EXPECT_TRUE(flagged_in) << "required 'in' should fail the build"; + EXPECT_FALSE(flagged_dt_in) << "optional 'dt_in' must not fail the build"; +} + TEST(EngineName, DefaultsEmptyAndSurvivesBuild) { Graph g = piper_engine_test::make_linear_chain();