From fe986dae1f7cf6311eb344d9ebd732a85e5727b3 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 16:45:12 +0200 Subject: [PATCH 01/15] docs: component registry unification design spec (#51) --- ...6-component-registry-unification-design.md | 298 ++++++++++++++++++ 1 file changed, 298 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-06-component-registry-unification-design.md diff --git a/docs/superpowers/specs/2026-08-06-component-registry-unification-design.md b/docs/superpowers/specs/2026-08-06-component-registry-unification-design.md new file mode 100644 index 0000000..73d9f84 --- /dev/null +++ b/docs/superpowers/specs/2026-08-06-component-registry-unification-design.md @@ -0,0 +1,298 @@ +# Component Registration Unification + App Decomposition + +**Issue:** https://github.com/striderZA/Tiny-RF-Simulator/issues/51 + +**Problem:** Adding a new RF component (the project's headline extensibility claim) requires +coordinated edits across ~10 files because six parallel hardcoded type-dispatch tables drift. +`app/src/app.cpp` is a 1320-line god-object, and the PFB widget lockstep-vector lifecycle has +already caused one use-after-free (issue #37) and one missing-`markDirty()` bug (Equalizer). + +## Goal + +Adding a component should touch **one registry row + one engine module** (+ unavoidable per-kind +view code in `node_graph`). All type dispatch — canvas menu, add, duplicate, save/load, inspector +drawing — flows through a single `ComponentTypeRegistry` table in `app/`. + +## Scope + +Three workstreams, staged as separate implementation phases: + +1. **Unified registration object + `type_name()` virtual** — extend `ComponentTypeRegistry` into + the single table; add `type_name()` to `IComponentEngine`; rewire save/load/duplicate/menu/ + inspector dispatch through it. Fixes the Equalizer `markDirty()` bug. +2. **`PFBViewManager` extraction** — move the four lockstep PFB widget vectors and their six + rebuild sites into one owning class. +3. **`ProjectSerializer` extraction** — move `saveProject()`/`loadProject()`/`newProject()` JSON + logic out of `RfSimulatorApp`. + +Out of scope: removing the test-only accessors (`testGraphEngine()`, `testComponents()`, ...) — +they are a smell but removing them is a separate cleanup that would churn `test_project_file.cpp` +with no behavior win. `NodeKind` enum + `themeColor` + `drawSchematicSymbol` stay in `node_graph` +(view-layer: a new schematic symbol is inherently per-kind drawing code). The library-authoring +form's type combo is driven from the registry instead of a hardcoded `type_names[]` array. + +## Backward compatibility constraints (non-negotiable) + +- `.rfsim` project files keep using today's type strings: `SignalGenerator`, `Amplifier`, + `Splitter`, `Mixer`, `Attenuator`, `Combiner`, `Equalizer`, `ADC`, `PFBChannelizer`, `CoaxCable`, + `IdealFilter`. `saveProject` writes them; `loadProject` must accept **both** legacy and canonical + (`amplifier`, ...) names. Old project files load unchanged; new files are indistinguishable from + old ones for the type field. +- Library JSON (`component_data/library/**/*.json`, `rf-sim-libraries/**`) keeps lowercase type + strings: `amplifier`, `attenuator`, `splitter`, `filter`, `mixer`, `equalizer`, `combiner`, `adc`. +- Canvas menu labels stay byte-identical: `Add Generator`, `Add Amplifier`, `Add Splitter`, + `Add Combiner`, `Add Coax Cable`, `Add Equalizer`, `Add Mixer`, `Add RF ADC`, + `Add PFB Channelizer`, `Add Ideal Filter`, `Add Attenuator`. UI tests in + `test_engine/ui_tests.cpp` click these strings. + +--- + +## Phase 1 — Unified registration object + `type_name()` virtual + +### 1.1 `IComponentEngine` gains `type_name()` + +`common/component_interface.h`: + +```cpp +virtual std::string_view type_name() const = 0; +``` + +Pure virtual (no default) — a new engine **must** self-identify or it won't compile. Returns the +canonical lowercase key, e.g. `"amplifier"`. Implemented in all 11 engine headers (inline, e.g. +`std::string_view type_name() const override { return "amplifier"; }`) plus the two test engines in +`tests/test_component_registry.cpp`. + +### 1.2 `ComponentTypeDescriptor` becomes the single table + +`app/include/component_type_registry.h` — extend the existing struct: + +```cpp +struct ComponentTypeDescriptor { + std::string type; // canonical key, e.g. "amplifier" + std::string project_type; // .rfsim name, e.g. "Amplifier" + std::string display_name; // e.g. "Amplifier" + std::string menu_label; // e.g. "Add Amplifier" + std::string label_prefix; // graph label prefix, e.g. "Amplifier " (trailing space) + NodeKind kind; // NodeKind::Amplifier (node_graph include) + bool authorable = false; // appears in New Component form combo + bool supports_sparam_file = false; + std::vector fields; + std::function create; + std::function draw_inspector; +}; +``` + +Notes: +- `create` replaces the old params-taking `factory`. Parameters are applied by the caller via + `deserialize()` — params application now lives in exactly one place per engine (its + `deserialize()`), not duplicated between registry factories and `loadProject` branches. +- `draw_inspector` is populated at app startup by one registration function in + `inspector_panel.cpp`, e.g. `void registerInspectorDrawers(ComponentTypeRegistry ®istry);` + called from the `RfSimulatorApp` constructor after the registry is built; it assigns + `draw_inspector` on each descriptor. The lambda does the `static_cast` + call, e.g. + `[](InspectorPanel &p, IComponentEngine &e) { p.drawAmplifierProperties(static_cast(e), e.id()); }`. + `InspectorPanel::draw()` only invokes `desc->draw_inspector`. The existing `drawXProperties` + methods become public (single-file churn, no behavior change). +- `ComponentTypeRegistry` keeps `find(std::string_view)` (canonical `type` lookup, existing) and + gains `findByProjectType(std::string_view)` (accepts canonical + legacy `.rfsim` names); + `all()` unchanged. +- Registry now holds **11** descriptors: the current 8 (`amplifier`, `attenuator`, `splitter`, + `filter`→`IdealFilter`, `mixer`, `equalizer`, `combiner`, `adc`) plus `generator`, `coax`, + `pfb`. `authorable` is true only for the current 8 (generator/coax/pfb stay out of the library + authoring form). + +### 1.3 `saveProject` / `loadProject` / `duplicateComponent` + +- `saveProject`: replace `s_type_names` static map with + `registry.find(comp->type_name())->project_type`. +- `loadProject`: replace the 11-branch chain with + `auto *d = registry.findByProjectType(type); if (!d) { LOG_WARN(...); ... } auto *comp = d->create(m_components, m_graph_engine, m_next_component_id++); comp->deserialize(params);`. + PFB restore calls `m_pfb_views.addFor(...)` (see Phase 2). +- `duplicateComponent`: replace the 11-way `dynamic_cast` chain + `dup` lambda with + `auto *d = registry.find(src->type_name()); auto *copy = d->create(...); copy->deserialize(src->serialize());` + plus the existing position-offset and part-number copy. PFB duplicate calls `m_pfb_views.addFor(...)`. +- `ComponentLibrary::instantiate` (`app/src/component_library.cpp`): `create()` + + `deserialize(def.parameters)`; the `def.type == "amplifier"` S-param special case stays. + +### 1.4 Canvas menu + add path + +`node_graph/include/node_graph_widget.h`: + +```cpp +struct AddableComponent { + std::string menu_label; + std::function on_add; +}; +void setAddableComponents(std::vector addable); +``` + +Remove the 11 `onAdd*` callbacks. `handleContextMenu()` iterates `m_addable_components`, rendering +one `ImGui::MenuItem(menu_label)` per entry. App constructor builds the list from +`ComponentTypeRegistry::instance().all()` and one shared `addComponent(const ComponentTypeDescriptor *, ImVec2)` +method that: `create()`s, sets node position, calls `markDirty()` **unconditionally** (fixes +Equalizer bug), and routes PFB through `m_pfb_views.addFor(...)`. + +### 1.5 Inspector dispatch + +`inspector_panel.cpp`: +- `findSelected()`: `engine = m_components->find(selected_id)` then + `auto *d = ComponentTypeRegistry::instance().find(engine->type_name()); return {d, engine};` + — the 11-way `dynamic_cast` chain dies. `Hit` becomes + `{ const ComponentTypeDescriptor *desc; IComponentEngine *engine; }`; the private `ComponentType` + enum is deleted. +- `labelForHit()`: derive from `desc->display_name` (+ `id`), PFB special case kept. Fixes a + latent drift bug: today's `labelForHit` switch has no `CoaxCable`/`Equalizer` case, so selecting + those components shows an empty panel title (they fall through to `default`). +- `draw()`: after the group/PFB-selector handling, dispatch via + `if (hit.desc && hit.desc->draw_inspector) hit.desc->draw_inspector(*this, *hit.engine);` — the + type switch dies. + +### 1.6 Node graph kind mapping becomes data-driven + +`node_graph` keeps `NodeKind`, `themeColor`, `drawSchematicSymbol` (per-kind view code stays +there). But `nodeKindFromLabel`'s 11-branch chain is replaced by a widget-held prefix map: + +```cpp +void NodeGraphWidget::registerNodeKind(std::string label_prefix, NodeKind kind); +``` + +`drawNodes()` looks up `node.label` prefix → `NodeKind` → `themeColor(kind)`. The app constructor +feeds `registerNodeKind(d->label_prefix, d->kind)` for each descriptor. `nodeKindFromLabel` is +deleted from `node_graph_engine.h`; the `[node_graph][appearance]` tests that asserted its exact +prefix list are removed (replaced by a registry-completeness test asserting every descriptor's +`label_prefix` maps to its `kind`). + +### 1.7 New Component form combo + +`app/src/app.cpp::drawComponentFormModal()`: replace the hardcoded `type_names[]` array with the +registry's `authorable` descriptors. + +--- + +## Phase 2 — PFBViewManager extraction + +New `app/include/pfb_view_manager.h` + `app/src/pfb_view_manager.cpp`: + +```cpp +class PFBViewManager { + public: + void addFor(PFBChannelizerEngine &engine, SessionState &state); // IQ + grid widgets + void rebuild(const ComponentRegistry &components, SessionState &state); // sync all PFBs + void clear(); + void draw(); // draw_ui loop body + void saveVisibility(SessionState &state) const; // destructor path + std::vector &iqVisibility() { return m_show_iq_pfbs; } // InspectorPanel hooks + std::vector &gridVisibility() { return m_show_pfb_grids; } + + private: + std::vector> m_iq_widgets; + std::vector m_show_iq_pfbs; + std::vector> m_pfb_grid_widgets; + std::vector m_show_pfb_grids; +}; +``` + +- `RfSimulatorApp` members `m_iq_widgets` / `m_show_iq_pfbs` / `m_pfb_grid_widgets` / + `m_show_pfb_grids` are deleted; one `PFBViewManager m_pfb_views;` member replaces them. +- The six rebuild sites (`onAddPFB`, `onRemoveNode`, `duplicateComponent`, `loadProject`, + `newProject`, destructor) collapse into `addFor` / `rebuild` / `clear` / `saveVisibility` calls. +- `draw_ui()`'s IQ-plot and channelizer-grid loops become `m_pfb_views.draw();`. +- `InspectorPanel::setPFBWindowVisibility(&m_show_iq_pfbs, &m_show_pfb_grids)` call site becomes + `setPFBWindowVisibility(&m_pfb_views.iqVisibility(), &m_pfb_views.gridVisibility())`. +- **Member declaration order**: `PFBViewManager` is declared after `m_components` in `app.h` + (reverse-declaration destruction ⇒ manager/widgets destroyed before the engines they reference). + This is strictly safer than today's order, though not load-bearing: `IQPlotWidget::~IQPlotWidget` + only frees `m_ifft` and `PFBChannelizerWidget` has no custom destructor, so no destructor + dereferences a dead engine either way. + +--- + +## Phase 3 — ProjectSerializer extraction + +New `app/include/project_serializer.h` + `app/src/project_serializer.cpp`: + +```cpp +class ProjectSerializer { + public: + ProjectSerializer(ComponentRegistry &components, NodeGraphEngine &graph, + NodeGraphWidget &graph_widget, PFBViewManager &pfb_views, + SessionState &state, int &next_component_id); + void save(const std::string &path); + bool load(const std::string &path); // returns false on parse/unknown-type errors (logged) + void reset(); // newProject: links, components, probes, counters, PFBs + private: + // all the JSON/link/pin/position/group logic currently in app.cpp +}; +``` + +- `RfSimulatorApp::saveProject` / `loadProject` / `newProject` become thin wrappers that delegate + JSON + graph/component work to `m_serializer` and keep only app-level concerns: file dialogs, + `m_dirty` flag, `refreshExtensions()`, `m_current_project_path`, window-state persistence. +- `loadProject`'s `LOG_WARN` unknown-type path, position restore, part-number restore, link + restore (component-index+port pairs), group restore, probe restore, and counter resets all move + with the logic. + +--- + +## Files changed (summary) + +**Phase 1** +- `common/component_interface.h` — `type_name()` pure virtual +- 11 engine headers (`signal_generator`, `amplifier`, `splitter`, `mixer`, `adc`, + `pfb_channelizer`, `coax`, `equalizer`, `ideal_filter`, `attenuator`, `combiner`) — inline + `type_name()` override +- `app/include/component_type_registry.h` / `app/src/component_type_registry.cpp` — descriptor + fields, 11 rows, new lookups, `create` +- `app/include/component_library.h` / `app/src/component_library.cpp` — `create`+`deserialize` +- `app/include/app.h` / `app/src/app.cpp` — `addComponent`, menu list build, `drawComponentFormModal` +- `app/include/inspector_panel.h` / `app/src/inspector_panel.cpp` — registry-driven dispatch, + draw registration +- `node_graph/include/node_graph_engine.h` — delete `nodeKindFromLabel` +- `node_graph/include/node_graph_widget.h` / `node_graph/src/node_graph_widget.cpp` — + `setAddableComponents`, `registerNodeKind`, menu loop +- `tests/test_component_registry.cpp` — test engines gain `type_name()` +- `tests/test_node_graph_engine.cpp` — drop `nodeKindFromLabel` cases + +**Phase 2** +- `app/include/pfb_view_manager.h` / `app/src/pfb_view_manager.cpp` — new +- `app/include/app.h` / `app/src/app.cpp` — member swap, 6 call sites +- `app/CMakeLists.txt` — add new sources + +**Phase 3** +- `app/include/project_serializer.h` / `app/src/project_serializer.cpp` — new +- `app/include/app.h` / `app/src/app.cpp` — thin wrappers +- `app/CMakeLists.txt` — add new sources + +**Tests + docs** +- New standalone test executable `tests/test_component_dispatch.cpp` (per `tests/AGENTS.md` MinGW + ceiling: standalone `add_executable`, not a new file in the `tests` target) +- `app/AGENTS.md`, `common/AGENTS.md` — DOX pass (new owners, `type_name()` contract) +- `openwiki/testing/guidance.md` — test file table update + +## Testing + +- **Registry completeness** (new standalone exe): all 11 canonical types present; every descriptor + has non-empty `type`, `project_type`, `menu_label`, `label_prefix`, `kind`, `create`, + `draw_inspector`; every `create()` returns an engine whose `type_name()` matches the row; every + descriptor's `label_prefix` maps to its `kind`. +- **Round-trip** (new standalone exe, ImGui fixture like `test_project_file.cpp`): for each of the + 11 types — add via `addComponent`, serialize, reload, assert type/params survive. PFB reload + recreates IQ/grid widgets. +- **Equalizer dirty-flag regression** (new standalone exe): `addComponent(equalizer)` → + `app.isDirty() == true` (was the bug). +- **Backward compat**: hand-written `.rfsim` containing legacy `SignalGenerator`/`ADC`/ + `IdealFilter` strings loads; `component_data/library` scan still validates (`filter` ↔ + `IdealFilter` mapping intact). +- Existing `build/bin/tests.exe`, `test_issue37_pfb_input_removal`, `test_extensions`, + `test_component_authoring`, `test_signal_domain`, `ui_tests` all pass unchanged (menu labels and + `.rfsim` strings identical). + +## Risks + +- **`draw_inspector` access**: `InspectorPanel::drawXProperties` are private today. Making them + public or registering via a friend/free-function seam is the main interface churn — contained in + one file, no behavior change. +- **Registry growth**: `component_type_registry.cpp` grows to 11 rows (~+250 lines) but stays a + flat declarative table — the point of the refactor. +- **Ordering**: `PFBViewManager` member placement must keep widget-before-engine destruction + order; verified by existing `test_issue37_pfb_input_removal` (no ASan hits). From ac97d433b67d256fc79e3a7591d2693dfe0e229d Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 17:04:53 +0200 Subject: [PATCH 02/15] docs: component registry unification implementation plan (#51) --- ...26-08-06-component-registry-unification.md | 1833 +++++++++++++++++ 1 file changed, 1833 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-06-component-registry-unification.md diff --git a/docs/superpowers/plans/2026-08-06-component-registry-unification.md b/docs/superpowers/plans/2026-08-06-component-registry-unification.md new file mode 100644 index 0000000..7cb0450 --- /dev/null +++ b/docs/superpowers/plans/2026-08-06-component-registry-unification.md @@ -0,0 +1,1833 @@ +# Component Registration Unification + App Decomposition — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make adding a new RF component touch one registry row + one engine module instead of ~10 files, by unifying all six parallel type-dispatch tables into the extended `ComponentTypeRegistry`, then extracting `PFBViewManager` and `ProjectSerializer` from the `RfSimulatorApp` god-object. + +**Architecture:** Extend the existing `app/` `ComponentTypeRegistry` into the single table (canonical type, `.rfsim` project name, display name, menu label, label prefix, `NodeKind`, `create()` factory, inspector draw callback). Engines self-identify via a new pure-virtual `type_name()` on `IComponentEngine`. All dispatch (canvas menu, add, duplicate, save/load, inspector) consumes the table. Then two extractions: `PFBViewManager` (owns the four lockstep PFB widget vectors) and `ProjectSerializer` (owns save/load/new JSON logic). + +**Tech Stack:** C++20, CMake ≥ 3.20, Ninja, MinGW-w64 g++ (Windows) / GCC / Clang, nlohmann/json, ImGui/ImNodes/ImPlot, Catch2 v3.4.0. + +## Global Constraints + +- `.rfsim` project files keep today's type strings verbatim: `SignalGenerator`, `Amplifier`, `Splitter`, `Mixer`, `Attenuator`, `Combiner`, `Equalizer`, `ADC`, `PFBChannelizer`, `CoaxCable`, `IdealFilter`. `saveProject` writes them; `loadProject` accepts legacy AND canonical (`amplifier`, ...) names. +- Library JSON (`component_data/library/**`, `rf-sim-libraries/**`) keeps lowercase type strings: `amplifier`, `attenuator`, `splitter`, `filter`, `mixer`, `equalizer`, `combiner`, `adc`. +- Canvas menu labels stay byte-identical: `Add Generator`, `Add Amplifier`, `Add Splitter`, `Add Combiner`, `Add Coax Cable`, `Add Equalizer`, `Add Mixer`, `Add RF ADC`, `Add PFB Channelizer`, `Add Ideal Filter`, `Add Attenuator`. UI tests in `test_engine/ui_tests.cpp` click these exact strings. +- Library-form parameter keys must keep working after `instantiate()` moves to `create()`+`deserialize()` — the key-parity fixes in Task 2b are mandatory, not optional. +- Only widget files may `#include ` / `` / `` (per CONTRIBUTING). `app/src/app.cpp` and `inspector_panel.cpp` are widget-layer files and already include them. +- `uint64_t` requires explicit `#include `. +- MinGW test-registration ceiling (~217 TEST_CASEs in the `tests` binary): any NEW test cases go into a NEW standalone executable (`tests/test_component_dispatch.cpp`). Do not add TEST_CASEs to `test_main.cpp` or any file already in the `tests` target. REMOVING cases from `test_node_graph_engine.cpp` is fine. +- Format: run `scripts/format.sh` after each task; CI enforces clang-format 18. +- Commit per task with an imperative subject; verify `cmake --build build && ctest --test-dir build` before committing. + +--- + +## Phase 1 — Unified registry + `type_name()` + +### Task 1: `type_name()` pure virtual on IComponentEngine + +**Files:** +- Modify: `common/component_interface.h` +- Modify: 11 engine headers (`signal_generator`, `amplifier`, `splitter`, `mixer`, `adc`, `pfb_channelizer`, `coax`, `equalizer`, `ideal_filter`, `attenuator`, `combiner` — each `include/*_engine.h`) +- Modify: `tests/test_component_registry.cpp` (test engines `TestEngineA`/`TestEngineB`) + +**Interfaces:** +- Produces: `virtual std::string_view type_name() const = 0;` on `IComponentEngine`; every engine returns its canonical lowercase key. + +- [ ] **Step 1: Add the pure virtual to the interface** + +`common/component_interface.h` — add `#include ` and the method (after the `inputPinId(int)` block, before `serialize()`): + +```cpp +#pragma once + +#include "signal_node.h" +#include +#include +#include + +class IComponentEngine { + public: + virtual ~IComponentEngine() = default; + virtual int id() const = 0; + virtual int graphNodeId() const = 0; + virtual int outputPinId() const = 0; + virtual std::string hoverSummary() const = 0; + virtual SignalNode &node() = 0; + virtual const SignalNode &node() const = 0; + virtual void update(double dt) = 0; + + virtual int inputPinId() const { return -1; } + + // Multi-pin accessors (default: forward to inputPinId() for port 0) + virtual int inputPinId(int port) const { return port == 0 ? inputPinId() : -1; } + virtual int outputPinId(int port) const { return port == 0 ? outputPinId() : -1; } + + // Pin count (default: 1/1 for legacy single-pin engines) + virtual int numInputPins() const { return 1; } + virtual int numOutputPins() const { return 1; } + + // Canonical type key (e.g. "amplifier"). Single source of truth for the + // component-type dispatch tables (save/load, duplicate, inspector, menu). + virtual std::string_view type_name() const = 0; + + // Serialization — default no-op + virtual nlohmann::json serialize() const { return nlohmann::json::object(); } + virtual void deserialize(const nlohmann::json &) {} +}; +``` + +- [ ] **Step 2: Implement in all 11 engines** — add this inline override to each engine header (put it next to the other `int id() const override`-style one-liners): + +| Header | exact line | +|---|---| +| `signal_generator/include/signal_generator_engine.h` | `std::string_view type_name() const override { return "generator"; }` | +| `amplifier/include/amplifier_engine.h` | `std::string_view type_name() const override { return "amplifier"; }` | +| `splitter/include/splitter_engine.h` | `std::string_view type_name() const override { return "splitter"; }` | +| `mixer/include/mixer_engine.h` | `std::string_view type_name() const override { return "mixer"; }` | +| `adc/include/adc_engine.h` | `std::string_view type_name() const override { return "adc"; }` | +| `pfb_channelizer/include/pfb_channelizer_engine.h` | `std::string_view type_name() const override { return "pfb"; }` | +| `coax/include/coax_cable_engine.h` | `std::string_view type_name() const override { return "coax"; }` | +| `equalizer/include/equalizer_engine.h` | `std::string_view type_name() const override { return "equalizer"; }` | +| `ideal_filter/include/ideal_filter_engine.h` | `std::string_view type_name() const override { return "filter"; }` | +| `attenuator/include/attenuator_engine.h` | `std::string_view type_name() const override { return "attenuator"; }` | +| `combiner/include/combiner_engine.h` | `std::string_view type_name() const override { return "combiner"; }` | + +Each header already includes `component_interface.h` (which now provides ``), so no extra include is needed. + +- [ ] **Step 3: Implement in the two test engines** + +`tests/test_component_registry.cpp` — in `TestEngineA` add `std::string_view type_name() const override { return "test_a"; }`; in `TestEngineB` add `std::string_view type_name() const override { return "test_b"; }`. + +- [ ] **Step 4: Build** + +Run: `cmake --build build` +Expected: compiles clean (the pure virtual forces every engine to implement it — the build is the test). + +- [ ] **Step 5: Run existing tests** + +Run: `build/bin/tests.exe` (Windows) or `build/bin/tests` (Linux/macOS); also `build/bin/test_issue37_pfb_input_removal.exe` (or without `.exe`). +Expected: all pass (no behavior change). + +- [ ] **Step 6: Commit** + +```bash +git add common/component_interface.h \ + signal_generator/include/signal_generator_engine.h \ + amplifier/include/amplifier_engine.h \ + splitter/include/splitter_engine.h \ + mixer/include/mixer_engine.h \ + adc/include/adc_engine.h \ + pfb_channelizer/include/pfb_channelizer_engine.h \ + coax/include/coax_cable_engine.h \ + equalizer/include/equalizer_engine.h \ + ideal_filter/include/ideal_filter_engine.h \ + attenuator/include/attenuator_engine.h \ + combiner/include/combiner_engine.h \ + tests/test_component_registry.cpp +git commit -m "refactor: add type_name() virtual to IComponentEngine" +``` + +--- + +### Task 2a: Extend descriptor struct + populate 11 registry rows (additive) + +**Files:** +- Modify: `app/include/component_type_registry.h` +- Modify: `app/src/component_type_registry.cpp` +- Modify: `tests/test_component_authoring.cpp` + +**Interfaces:** +- Consumes: `NodeKind` from `node_graph/include/node_graph_engine.h` (app already depends on node_graph). +- Produces: `ComponentTypeDescriptor` with `type`, `project_type`, `display_name`, `menu_label`, `label_prefix`, `kind`, `authorable`, `supports_sparam_file`, `fields`, `create`, `draw_inspector` — AND the old `factory` field kept (additive, so this task compiles and commits alone; removed in Task 2b). `ComponentTypeRegistry` with `find(std::string_view)`, `findByProjectType(std::string_view)`, `all()` returning non-const pointers. + +- [ ] **Step 1: Rewrite the header** + +`app/include/component_type_registry.h` — full file: + +```cpp +#pragma once + +#include "node_graph_engine.h" +#include +#include +#include +#include +#include +#include + +class ComponentRegistry; +class NodeGraphEngine; +class IComponentEngine; +class InspectorPanel; + +enum class FieldKind { Number, String, Enum, FilePath, Bool }; + +struct ParameterField { + std::string key; // JSON key under "parameters", e.g. "gain_dB" + std::string label; // UI label, e.g. "Gain" + std::string unit; // e.g. "dB", "Hz"; empty if none + FieldKind kind = FieldKind::Number; + bool required = false; + double min = -std::numeric_limits::infinity(); // Number only + double max = std::numeric_limits::infinity(); // Number only + std::vector enum_values; // Enum only + nlohmann::json default_value; // optional + std::string help; // optional tooltip +}; + +struct ComponentTypeDescriptor { + std::string type; // canonical key, e.g. "amplifier" + std::string project_type; // .rfsim save/load name, e.g. "Amplifier" + std::string display_name; // e.g. "Amplifier" + std::string menu_label; // canvas menu item, e.g. "Add Amplifier" + std::string label_prefix; // graph label prefix, e.g. "Amplifier" + NodeKind kind = NodeKind::Unknown; + bool authorable = false; // appears in New Component form combo + bool supports_sparam_file = false; + std::vector fields; + + // Create a default engine of this type (no params). Callers apply params + // via engine->deserialize(). Replaces the old params-taking `factory`. + std::function create; + // Inspector property draw. Receives the panel so PFB's multi-instance + // selector and dirty-flag state stay reachable. + std::function draw_inspector; + + // Legacy params-taking factory; still used by ComponentLibrary until + // Task 2b migrates instantiate to create()+deserialize(). + std::function + factory; +}; + +class ComponentTypeRegistry { + public: + static ComponentTypeRegistry &instance(); + + const ComponentTypeDescriptor *find(std::string_view type) const; + const ComponentTypeDescriptor *findByProjectType(std::string_view name) const; + std::vector all(); + + private: + ComponentTypeRegistry(); + std::vector m_descriptors; +}; +``` + +- [ ] **Step 2: Rewrite the registry implementation** + +`app/src/component_type_registry.cpp` — full file. Rows are registered in this order so the New-Component form combo keeps today's order (`amplifier, attenuator, splitter, filter, mixer, equalizer, combiner, adc`). The 8 authorable rows carry BOTH `factory` (old, unchanged semantics) and `create`; the 3 new rows (generator/coax/pfb) carry only `create` (they are not library-authorable, so no factory needed): + +```cpp +// app/src/component_type_registry.cpp +#include "component_type_registry.h" + +#include "adc_engine.h" +#include "amplifier_engine.h" +#include "attenuator_engine.h" +#include "coax_cable_engine.h" +#include "combiner_engine.h" +#include "component_registry.h" +#include "equalizer_engine.h" +#include "ideal_filter_engine.h" +#include "mixer_engine.h" +#include "pfb_channelizer_engine.h" +#include "signal_generator_engine.h" +#include "splitter_engine.h" + +ComponentTypeRegistry &ComponentTypeRegistry::instance() { + static ComponentTypeRegistry reg; + return reg; +} + +const ComponentTypeDescriptor *ComponentTypeRegistry::find(std::string_view type) const { + for (const auto &d : m_descriptors) + if (d.type == type) + return &d; + return nullptr; +} + +const ComponentTypeDescriptor *ComponentTypeRegistry::findByProjectType( + std::string_view name) const { + for (const auto &d : m_descriptors) + if (d.type == name || d.project_type == name) + return &d; + return nullptr; +} + +std::vector ComponentTypeRegistry::all() { + std::vector result; + result.reserve(m_descriptors.size()); + for (auto &d : m_descriptors) + result.push_back(&d); + return result; +} + +ComponentTypeRegistry::ComponentTypeRegistry() { + ComponentTypeDescriptor amp; + amp.type = "amplifier"; + amp.project_type = "Amplifier"; + amp.display_name = "Amplifier"; + amp.menu_label = "Add Amplifier"; + amp.label_prefix = "Amplifier"; + amp.kind = NodeKind::Amplifier; + amp.authorable = true; + amp.supports_sparam_file = true; + amp.fields = { + {"gain_dB", "Gain", "dB", FieldKind::Number, true, -50.0, 100.0, {}, {}, ""}, + {"nf_dB", "Noise Figure", "dB", FieldKind::Number, false, 0.0, 30.0, {}, {}, ""}, + {"oip2_dBm", "OIP2", "dBm", FieldKind::Number, false, -20.0, 100.0, {}, {}, ""}, + {"oip3_dBm", "OIP3", "dBm", FieldKind::Number, false, -20.0, 100.0, {}, {}, ""}, + {"p1db_dBm", "P1dB", "dBm", FieldKind::Number, false, -20.0, 100.0, {}, {}, ""}, + }; + amp.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + // Legacy factory: applied library params directly. Removed in Task 2b. + amp.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("gain_dB")) + e.setGain_dB(parameters["gain_dB"].get()); + if (parameters.contains("nf_dB")) + e.setNF_dB(parameters["nf_dB"].get()); + if (parameters.contains("oip2_dBm")) + e.setOIP2_dBm(parameters["oip2_dBm"].get()); + if (parameters.contains("oip3_dBm")) + e.setOIP3_dBm(parameters["oip3_dBm"].get()); + if (parameters.contains("p1db_dBm")) + e.setP1dB_dBm(parameters["p1db_dBm"].get()); + bool has_nonlinear = parameters.contains("oip2_dBm") || parameters.contains("oip3_dBm") || + parameters.contains("p1db_dBm"); + if (has_nonlinear) + e.setEnableNonlinear(true); + return &e; + }; + m_descriptors.push_back(amp); + + ComponentTypeDescriptor att; + att.type = "attenuator"; + att.project_type = "Attenuator"; + att.display_name = "Attenuator"; + att.menu_label = "Add Attenuator"; + att.label_prefix = "Attenuator"; + att.kind = NodeKind::Attenuator; + att.authorable = true; + att.fields = { + {"attenuation_dB", "Attenuation", "dB", FieldKind::Number, true, 0.0, 100.0, {}, {}, ""}, + }; + att.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + att.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("attenuation_dB")) + e.setAttenuation(parameters["attenuation_dB"].get()); + return &e; + }; + m_descriptors.push_back(att); + + ComponentTypeDescriptor spl; + spl.type = "splitter"; + spl.project_type = "Splitter"; + spl.display_name = "Splitter"; + spl.menu_label = "Add Splitter"; + spl.label_prefix = "Splitter"; + spl.kind = NodeKind::Splitter; + spl.authorable = true; + spl.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + spl.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json &) -> IComponentEngine * { + auto &e = registry.add(id, graph); + return &e; + }; + m_descriptors.push_back(spl); + + ComponentTypeDescriptor flt; + flt.type = "filter"; + flt.project_type = "IdealFilter"; + flt.display_name = "IdealFilter"; + flt.menu_label = "Add Ideal Filter"; + flt.label_prefix = "IdealFilter"; + flt.kind = NodeKind::IdealFilter; + flt.authorable = true; + flt.fields = { + {"filter_type", + "Filter Type", + "", + FieldKind::Enum, + true, + 0, + 0, + {"LPF", "HPF", "BPF", "BSF"}, + {}, + ""}, + {"fc_low_Hz", "Low Cutoff", "Hz", FieldKind::Number, false, 0.0, 1e12, {}, {}, ""}, + {"fc_high_Hz", "High Cutoff", "Hz", FieldKind::Number, false, 0.0, 1e12, {}, {}, ""}, + }; + flt.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + flt.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("filter_type")) { + std::string ft = parameters["filter_type"].get(); + if (ft == "LPF") + e.setFilterType(FilterType::LPF); + else if (ft == "HPF") + e.setFilterType(FilterType::HPF); + else if (ft == "BPF") + e.setFilterType(FilterType::BPF); + else if (ft == "BSF") + e.setFilterType(FilterType::BSF); + } + double fc_low = parameters.value("fc_low_Hz", 100e6); + double fc_high = parameters.value("fc_high_Hz", 200e6); + if (parameters.contains("fc_low_Hz") && parameters.contains("fc_high_Hz")) + e.setCutoffs_Hz(fc_low, fc_high); + else if (parameters.contains("fc_low_Hz")) + e.setCutoff_Hz(fc_low); + return &e; + }; + m_descriptors.push_back(flt); + + ComponentTypeDescriptor mix; + mix.type = "mixer"; + mix.project_type = "Mixer"; + mix.display_name = "Mixer"; + mix.menu_label = "Add Mixer"; + mix.label_prefix = "Mixer"; + mix.kind = NodeKind::Mixer; + mix.authorable = true; + mix.fields = { + {"lo_freq_Hz", "LO Frequency", "Hz", FieldKind::Number, true, 0.0, 1e12, {}, {}, ""}, + {"conversion_gain_dB", + "Conversion Gain", + "dB", + FieldKind::Number, + false, + -60.0, + 30.0, + {}, + {}, + ""}, + {"nf_dB", "Noise Figure", "dB", FieldKind::Number, false, 0.0, 30.0, {}, {}, ""}, + }; + mix.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + mix.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("lo_freq_Hz")) + e.setLoFreq_Hz(parameters["lo_freq_Hz"].get()); + if (parameters.contains("conversion_gain_dB")) + e.setConversionGain_dB(parameters["conversion_gain_dB"].get()); + if (parameters.contains("nf_dB")) + e.setNF_dB(parameters["nf_dB"].get()); + return &e; + }; + m_descriptors.push_back(mix); + + ComponentTypeDescriptor eq; + eq.type = "equalizer"; + eq.project_type = "Equalizer"; + eq.display_name = "Equalizer"; + eq.menu_label = "Add Equalizer"; + eq.label_prefix = "Equalizer"; + eq.kind = NodeKind::Equalizer; + eq.authorable = true; + eq.fields = { + {"ref_gain_dB", "Reference Gain", "dB", FieldKind::Number, false, -50.0, 50.0, {}, {}, ""}, + {"ref_freq_Hz", + "Reference Frequency", + "Hz", + FieldKind::Number, + false, + 0.0, + 1e12, + {}, + {}, + ""}, + {"slope_dB_per_decade", + "Slope", + "dB/decade", + FieldKind::Number, + false, + -100.0, + 100.0, + {}, + {}, + ""}, + }; + eq.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + eq.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("ref_gain_dB")) + e.setRefGain_dB(parameters["ref_gain_dB"].get()); + if (parameters.contains("ref_freq_Hz")) + e.setRefFreq_Hz(parameters["ref_freq_Hz"].get()); + if (parameters.contains("slope_dB_per_decade")) + e.setSlope_dBPerDecade(parameters["slope_dB_per_decade"].get()); + return &e; + }; + m_descriptors.push_back(eq); + + ComponentTypeDescriptor comb; + comb.type = "combiner"; + comb.project_type = "Combiner"; + comb.display_name = "Combiner"; + comb.menu_label = "Add Combiner"; + comb.label_prefix = "Combiner"; + comb.kind = NodeKind::Combiner; + comb.authorable = true; + comb.fields = { + {"manual_mode", "Manual Mode", "", FieldKind::Bool, false, 0, 0, {}, false, ""}, + }; + comb.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + comb.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("manual_mode")) + e.setManualMode(parameters["manual_mode"].get()); + return &e; + }; + m_descriptors.push_back(comb); + + ComponentTypeDescriptor adc; + adc.type = "adc"; + adc.project_type = "ADC"; + adc.display_name = "ADC"; + adc.menu_label = "Add RF ADC"; + adc.label_prefix = "ADC"; + adc.kind = NodeKind::Adc; + adc.authorable = true; + adc.fields = { + {"fs_Hz", "Sample Rate", "Hz", FieldKind::Number, true, 0.0, 1e12, {}, {}, ""}, + {"nsd_dBm_per_Hz", + "Noise Spectral Density", + "dBm/Hz", + FieldKind::Number, + false, + -200.0, + 0.0, + {}, + {}, + ""}, + }; + adc.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + adc.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, + const nlohmann::json ¶meters) -> IComponentEngine * { + auto &e = registry.add(id, graph); + if (parameters.contains("fs_Hz")) + e.setFs_Hz(parameters["fs_Hz"].get()); + if (parameters.contains("nsd_dBm_per_Hz")) + e.setNsd_dBm_per_Hz(parameters["nsd_dBm_per_Hz"].get()); + return &e; + }; + m_descriptors.push_back(adc); + + ComponentTypeDescriptor gen; + gen.type = "generator"; + gen.project_type = "SignalGenerator"; + gen.display_name = "Generator"; + gen.menu_label = "Add Generator"; + gen.label_prefix = "Generator"; + gen.kind = NodeKind::Generator; + gen.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + m_descriptors.push_back(gen); + + ComponentTypeDescriptor coax; + coax.type = "coax"; + coax.project_type = "CoaxCable"; + coax.display_name = "Coax Cable"; + coax.menu_label = "Add Coax Cable"; + coax.label_prefix = "Coax Cable"; + coax.kind = NodeKind::CoaxCable; + coax.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + m_descriptors.push_back(coax); + + ComponentTypeDescriptor pfb; + pfb.type = "pfb"; + pfb.project_type = "PFBChannelizer"; + pfb.display_name = "PFB"; + pfb.menu_label = "Add PFB Channelizer"; + pfb.label_prefix = "PFB"; + pfb.kind = NodeKind::PFB; + pfb.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + m_descriptors.push_back(pfb); +} +``` + +- [ ] **Step 3: Update the registry-count test** + +`tests/test_component_authoring.cpp` — change the `"ComponentTypeRegistry covers all 8 existing types"` TEST_CASE to expect 11: + +```cpp +TEST_CASE("ComponentTypeRegistry covers all 11 existing types", "[type_registry]") { + auto all = ComponentTypeRegistry::instance().all(); + std::vector types; + for (auto *d : all) + types.push_back(d->type); + std::sort(types.begin(), types.end()); + std::vector expected = {"adc", "amplifier", "attenuator", "coax", + "combiner", "equalizer", "filter", "generator", + "mixer", "pfb", "splitter"}; + REQUIRE(types == expected); +} +``` + +- [ ] **Step 4: Build** + +Run: `cmake --build build` +Expected: compiles clean (additive change — `factory` still present, `ComponentLibrary` untouched). + +- [ ] **Step 5: Run authoring tests** + +Run: `build/bin/test_component_authoring.exe` (or without `.exe`). +Expected: all pass — 11-type registry test, amplifier descriptor field test, filter enum test, `ComponentLibrary::validate`/`upsert`/instantiate tests, form-model tests. + +- [ ] **Step 6: Commit** + +```bash +git add app/include/component_type_registry.h app/src/component_type_registry.cpp tests/test_component_authoring.cpp +git commit -m "refactor: extend ComponentTypeRegistry to all 11 types" +``` + +--- + +### Task 2b: Switch instantiate to `create()`+`deserialize()`, remove `factory`, fix deserialize key parity + +**Files:** +- Modify: `app/src/component_library.cpp` +- Modify: `amplifier/src/amplifier_engine.cpp` (enable_nonlinear derivation) +- Modify: `adc/src/adc_engine.cpp` (`fs_Hz` key alias) +- Modify: `mixer/src/mixer_engine.cpp` (`conversion_gain_dB` key alias) +- Modify: `attenuator/src/attenuator_engine.cpp` (`attenuation_dB` key alias) +- Modify: `ideal_filter/src/ideal_filter_engine.cpp` (string enum + conditional cutoff logic) + +**Interfaces:** +- Consumes: `ComponentTypeDescriptor::create` from Task 2a. +- Produces: `ComponentLibrary::instantiate()` creates via `create()` then applies params via `deserialize()`. Each affected engine's `deserialize()` accepts BOTH the engine's serialize keys (project files) and the library-form keys. + +- [ ] **Step 1: Rewire `ComponentLibrary::instantiate`** + +`app/src/component_library.cpp` — replace the factory call (around line 164) with: + +```cpp + IComponentEngine *result = descriptor->create(registry, graph, id); + if (!result) + return nullptr; + result->deserialize(def.parameters); +``` + +Everything after (the `def.type == "amplifier"` S-param special case and part-number restore) stays. + +- [ ] **Step 2: Amplifier — enable_nonlinear derivation parity** + +`amplifier/src/amplifier_engine.cpp` — replace the body of `deserialize` with: + +```cpp +void AmplifierEngine::deserialize(const nlohmann::json &j) { + m_gain_dB = j.value("gain_dB", 0.0); + m_nf_dB = j.value("nf_dB", 0.0); + m_nonlinear.setEnabled(j.value("enable_nonlinear", false)); + m_nonlinear.setOIP2_dBm(j.value("oip2_dBm", 50.0)); + m_nonlinear.setOIP3_dBm(j.value("oip3_dBm", 50.0)); + m_nonlinear.setP1dB_dBm(j.value("p1db_dBm", 100.0)); + // Library definitions (schema v1/v2) omit `enable_nonlinear` but include + // OIP/P1dB params. The old registry factory enabled nonlinearity whenever + // any of those were present; project files always serialize the explicit + // key, so only fall back when it is absent. + if (!j.contains("enable_nonlinear") && + (j.contains("oip2_dBm") || j.contains("oip3_dBm") || j.contains("p1db_dBm"))) + m_nonlinear.setEnabled(true); + m_sparam_mode = j.value("sparam_mode", false); + m_sparam_filepath = j.value("sparam_filepath", ""); + m_sparam_fwd_idx = j.value("sparam_fwd_idx", 0); + m_dirty = true; +} +``` + +- [ ] **Step 3: ADC — accept `fs_Hz` (library) alongside `sample_rate_Hz` (project)** + +`adc/src/adc_engine.cpp` — replace the body of `deserialize` with: + +```cpp +void AdcEngine::deserialize(const nlohmann::json &j) { + m_fs_Hz = j.contains("sample_rate_Hz") ? j["sample_rate_Hz"].get() + : j.value("fs_Hz", 1e9); + m_nsd_dBm_per_Hz = j.value("nsd_dBm_per_Hz", -155.0); + m_dirty = true; +} +``` + +- [ ] **Step 4: Mixer — accept `conversion_gain_dB` (library) alongside `conv_gain_dB` (project)** + +`mixer/src/mixer_engine.cpp` — replace the body of `deserialize` with: + +```cpp +void MixerEngine::deserialize(const nlohmann::json &j) { + m_lo_freq_Hz = j.value("lo_freq_Hz", 1e9); + m_conv_gain_dB = j.contains("conv_gain_dB") ? j["conv_gain_dB"].get() + : j.value("conversion_gain_dB", -6.0); + m_nf_dB = j.value("nf_dB", 0.0); + m_dirty = true; +} +``` + +- [ ] **Step 5: Attenuator — accept `attenuation_dB` (library) alongside `atten_dB` (project)** + +`attenuator/src/attenuator_engine.cpp` — replace the body of `deserialize` with: + +```cpp +void AttenuatorEngine::deserialize(const nlohmann::json &j) { + m_atten_dB = j.contains("atten_dB") ? j["atten_dB"].get() + : j.value("attenuation_dB", 0.0); + m_sparam_mode = j.value("sparam_mode", false); + m_sparam_path = j.value("sparam_path", ""); + m_dirty = true; +} +``` + +- [ ] **Step 6: Ideal filter — accept string enum + preserve factory cutoff logic** + +`ideal_filter/src/ideal_filter_engine.cpp` — replace the body of `deserialize` with: + +```cpp +void IdealFilterEngine::deserialize(const nlohmann::json &j) { + if (j.contains("filter_type")) { + if (j["filter_type"].is_string()) { + const std::string ft = j["filter_type"].get(); + if (ft == "LPF") + m_type = FilterType::LPF; + else if (ft == "HPF") + m_type = FilterType::HPF; + else if (ft == "BPF") + m_type = FilterType::BPF; + else if (ft == "BSF") + m_type = FilterType::BSF; + } else { + int ft = j.value("filter_type", 0); + if (ft < 0) + ft = 0; + if (ft > 3) + ft = 3; + m_type = static_cast(ft); + } + } + // Preserve the old registry factory's conditional cutoff semantics: + // both present -> setCutoffs; only fc_low -> setCutoff (mirrors high); + // neither -> keep constructor defaults. + if (j.contains("fc_low_Hz") && j.contains("fc_high_Hz")) { + setCutoffs_Hz(j["fc_low_Hz"].get(), j["fc_high_Hz"].get()); + } else if (j.contains("fc_low_Hz")) { + setCutoff_Hz(j["fc_low_Hz"].get()); + } + m_sparam_mode = j.value("sparam_mode", false); + m_sparam_filepath = j.value("sparam_filepath", ""); + m_sparam_fwd_idx = j.value("sparam_fwd_idx", 0); + m_dirty = true; +} +``` + +- [ ] **Step 7: Remove the now-dead `factory` field from the descriptor** + +`app/include/component_type_registry.h` — delete the `factory` member and its comment. `app/src/component_type_registry.cpp` — delete all 8 `xxx.factory = ...` assignments (the bodies above have already been superseded; do not carry them into `create` — params are applied by the caller through `deserialize()`). + +- [ ] **Step 8: Build** + +Run: `cmake --build build` +Expected: compiles clean. + +- [ ] **Step 9: Run library + authoring tests** + +Run: `build/bin/test_component_authoring.exe` and `build/bin/tests.exe`. +Expected: all pass — `ComponentTypeRegistry` 11-type test, amplifier descriptor field test, filter enum test, `ComponentLibrary::validate`/`upsert`/instantiate tests, form-model tests. + +- [ ] **Step 10: Commit** + +```bash +git add app/src/component_library.cpp app/include/component_type_registry.h app/src/component_type_registry.cpp amplifier/src/amplifier_engine.cpp adc/src/adc_engine.cpp mixer/src/mixer_engine.cpp attenuator/src/attenuator_engine.cpp ideal_filter/src/ideal_filter_engine.cpp +git commit -m "refactor: instantiate via create()+deserialize(), drop factory field" +``` + +--- + +### Task 3: Rewire saveProject / loadProject / duplicateComponent through the registry + +**Files:** +- Modify: `app/src/app.cpp` (saveProject type map ~line 422; loadProject branch chain ~line 586; duplicateComponent dynamic_cast chain ~line 224) + +**Interfaces:** +- Consumes: `IComponentEngine::type_name()`, `ComponentTypeRegistry::find()`/`findByProjectType()`. +- Produces: saveProject writes `project_type` strings byte-identically; loadProject accepts legacy + canonical; duplicateComponent clones via `create()`+`deserialize()`. + +- [ ] **Step 1: Rewire saveProject** + +`app/src/app.cpp` `saveProject` — delete the `s_type_names` static map (lines ~422-441). Inside the component loop, replace: + +```cpp + auto it = s_type_names.find(std::type_index(typeid(*comp))); + cj["type"] = (it != s_type_names.end()) ? it->second : "Unknown"; +``` + +with: + +```cpp + const auto *desc = ComponentTypeRegistry::instance().find(comp->type_name()); + cj["type"] = desc ? desc->project_type : "Unknown"; +``` + +Also remove the now-unused includes at the top of `app.cpp`: `` and `` (verify with a build; `` is used elsewhere in `app.cpp` for the pin map in `saveProject` — keep it if still referenced). + +- [ ] **Step 2: Rewire loadProject** + +`app/src/app.cpp` `loadProject` — replace the entire 11-branch `if (type == ...)` chain (from `IComponentEngine *comp = nullptr;` through the final `} else { LOG_WARN... }`) with: + +```cpp + const auto *desc = ComponentTypeRegistry::instance().findByProjectType(type); + if (!desc) { + LOG_WARN("Unknown component type in project file: %s", type.c_str()); + new_node_ids.push_back(-1); + continue; + } + IComponentEngine *comp = desc->create(m_components, m_graph_engine, m_next_component_id++); + comp->deserialize(params); + if (desc->type == "pfb") { + auto *pfb = static_cast(comp); + // Restore IQ plot + PFB grid widgets for this PFB + m_iq_widgets.push_back(std::make_unique(*pfb)); + m_show_iq_pfbs.push_back(true); + m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); + m_show_pfb_grids.push_back(true); + } + + new_node_ids.push_back(comp ? comp->graphNodeId() : -1); +``` + +> `comp` is never null on this path (create always succeeds); the rest of the loop (position restore, part-number restore) keeps using `comp` unchanged. The PFB block above is replaced by `m_pfb_views.addFor(...)` in Phase 2. + +- [ ] **Step 3: Rewire duplicateComponent** + +`app/src/app.cpp` `duplicateComponent` — delete the `dup` lambda and the entire 11-way `dynamic_cast` chain. Replace from the `// Helper: create a new engine of type T...` comment through the final closing brace of the if-chain with: + +```cpp + // Clone via the registry: create a default engine, then copy params through + // serialize/deserialize. Removes the 11-way dynamic_cast chain. + const auto *desc = ComponentTypeRegistry::instance().find(src->type_name()); + if (!desc) + return; + IComponentEngine *copy = desc->create(m_components, m_graph_engine, m_next_component_id++); + copy->deserialize(src->serialize()); + int new_nid = copy->graphNodeId(); + // Register with imnodes pool and set position + ImNodes::EditorContextSet(m_graph_widget->context()); + ImNodes::SetNodeEditorSpacePos(new_nid, ImVec2(src_pos.x + OFFSET, src_pos.y + OFFSET)); + // Copy library part number + if (!src_part_number.empty()) + m_graph_engine.setNodePartNumber(new_nid, src_part_number); + // PFB also needs IQ plot widget and grid widget (same as onAddPFB) + if (desc->type == "pfb") { + auto *new_pfb = static_cast(copy); + m_iq_widgets.push_back(std::make_unique(*new_pfb)); + m_show_iq_pfbs.push_back(m_state.loadBool( + "WindowState", ("IQPlot_" + std::to_string(new_pfb->id())).c_str(), true)); + m_pfb_grid_widgets.push_back(std::make_unique(*new_pfb)); + m_show_pfb_grids.push_back(m_state.loadBool( + "WindowState", ("PFBGrid_" + std::to_string(new_pfb->id())).c_str(), true)); + } + + markDirty(); +``` + +> The PFB block above is replaced by `m_pfb_views.addFor(...)` in Phase 2. + +- [ ] **Step 4: Build** + +Run: `cmake --build build` +Expected: compiles clean. + +- [ ] **Step 5: Run round-trip tests** + +Run: `build/bin/test_project_file.exe`, `build/bin/tests.exe`, `build/bin/test_component_authoring.exe`. +Expected: all pass (save/load/duplicate rewired without breaking round-trips). + +- [ ] **Step 6: Commit** + +```bash +git add app/src/app.cpp +git commit -m "refactor: save/load/duplicate dispatch through ComponentTypeRegistry" +``` + +--- + +### Task 4: Data-driven canvas menu + unified addComponent (fixes Equalizer bug) + +**Files:** +- Create: `tests/test_component_dispatch.cpp` +- Modify: `tests/CMakeLists.txt` +- Modify: `node_graph/include/node_graph_widget.h` +- Modify: `node_graph/src/node_graph_widget.cpp` +- Modify: `app/include/app.h` +- Modify: `app/src/app.cpp` + +**Interfaces:** +- Consumes: `ComponentTypeRegistry::all()`. +- Produces: `NodeGraphWidget::setAddableComponents(std::vector)` replacing the 11 `onAdd*` callbacks; `RfSimulatorApp::addComponent(const ComponentTypeDescriptor*, ImVec2)`; the canvas context menu iterates the list. `test_component_dispatch.cpp` hosts the Equalizer dirty-flag regression (red→green this task). + +- [ ] **Step 1: Write the failing regression test (old API still present)** + +`tests/test_component_dispatch.cpp` — create the new standalone test file: + +```cpp +#include "app.h" +#include "imgui.h" +#include "imnodes.h" +#include "implot.h" +#include + +struct ImGuiFixture { + ImGuiFixture() { + ImGui::CreateContext(); + ImPlot::CreateContext(); + ImNodes::CreateContext(); + } + ~ImGuiFixture() { + ImNodes::DestroyContext(); + ImPlot::DestroyContext(); + ImGui::DestroyContext(); + } +}; + +TEST_CASE_METHOD(ImGuiFixture, "Adding an Equalizer marks the project dirty (issue #51)", + "[dispatch][regression]") { + RfSimulatorApp app; + REQUIRE(app.isDirty() == false); + // Canvas menu path for Equalizer. Today's onAddEqualizer lambda omits + // markDirty(); the unified addComponent path always marks dirty. + app.testGraphWidget().onAddEqualizer(ImVec2(0, 0)); + REQUIRE(app.isDirty() == true); +} +``` + +Add to `tests/CMakeLists.txt` (after the `test_issue37_pfb_input_removal` block): + +```cmake +add_executable(test_component_dispatch test_component_dispatch.cpp) +target_link_libraries(test_component_dispatch PRIVATE + simulator::app + Catch2::Catch2WithMain +) +target_compile_definitions(test_component_dispatch PRIVATE PROJECT_SOURCE_DIR="${CMAKE_SOURCE_DIR}") +add_test(NAME test_component_dispatch COMMAND test_component_dispatch WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) +``` + +- [ ] **Step 2: Build + run the new test to see it fail** + +Run: `cmake --build build && build/bin/test_component_dispatch.exe` +Expected: FAIL — `app.isDirty()` is still `false` after `onAddEqualizer` (the bug). + +- [ ] **Step 3: Widget header — replace 11 callbacks with one list** + +`node_graph/include/node_graph_widget.h` — delete the 11 `onAdd*` members (`onAddGenerator` ... `onAddCombiner`). Add: + +```cpp + // Data-driven canvas menu: app populates from ComponentTypeRegistry. + struct AddableComponent { + std::string menu_label; + std::function on_add; + }; + void setAddableComponents(std::vector addable) { + m_addable_components = std::move(addable); + } + const std::vector &addableComponents() const { return m_addable_components; } +``` + +In the private section, add: + +```cpp + std::vector m_addable_components; +``` + +- [ ] **Step 4: Widget cpp — iterate the list in handleContextMenu** + +`node_graph/src/node_graph_widget.cpp` `handleContextMenu` — replace the 11 `if (ImGui::MenuItem(...))` blocks inside `if (ImGui::BeginPopup("canvas_context_menu"))` with: + +```cpp + for (const auto &addable : m_addable_components) { + if (ImGui::MenuItem(addable.menu_label.c_str())) { + if (addable.on_add) + addable.on_add(m_context_menu_pos); + } + } +``` + +- [ ] **Step 5: App header — add addComponent** + +`app/include/app.h` — in the private section add: + +```cpp + void addComponent(const ComponentTypeDescriptor *desc, ImVec2 pos); +``` + +- [ ] **Step 6: App cpp — build the menu from the registry + unified add path** + +`app/src/app.cpp` constructor — delete all 11 `m_graph_widget->onAdd* = ...` lambda assignments (Generator through Combiner). Replace them with: + +```cpp + std::vector addable; + for (const auto *desc : ComponentTypeRegistry::instance().all()) { + addable.push_back({desc->menu_label, + [this, desc](ImVec2 pos) { addComponent(desc, pos); }}); + } + m_graph_widget->setAddableComponents(std::move(addable)); +``` + +Add the method at file scope after the constructor: + +```cpp +void RfSimulatorApp::addComponent(const ComponentTypeDescriptor *desc, ImVec2 pos) { + IComponentEngine *comp = desc->create(m_components, m_graph_engine, m_next_component_id++); + ImNodes::EditorContextSet(m_graph_widget->context()); + ImNodes::SetNodeEditorSpacePos(comp->graphNodeId(), pos); + if (desc->type == "pfb") { + auto *pfb = static_cast(comp); + m_iq_widgets.push_back(std::make_unique(*pfb)); + m_show_iq_pfbs.push_back(m_state.loadBool( + "WindowState", ("IQPlot_" + std::to_string(pfb->id())).c_str(), true)); + m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); + m_show_pfb_grids.push_back(m_state.loadBool( + "WindowState", ("PFBGrid_" + std::to_string(pfb->id())).c_str(), true)); + } + markDirty(); // unconditional — fixes the Equalizer missing-markDirty bug +} +``` + +> The PFB block above is replaced by `m_pfb_views.addFor(...)` in Phase 2. + +- [ ] **Step 7: Update the test to the new menu API** + +`tests/test_component_dispatch.cpp` — replace the test body with: + +```cpp +TEST_CASE_METHOD(ImGuiFixture, "Adding an Equalizer marks the project dirty (issue #51)", + "[dispatch][regression]") { + RfSimulatorApp app; + REQUIRE(app.isDirty() == false); + bool clicked = false; + for (const auto &addable : app.testGraphWidget().addableComponents()) { + if (addable.menu_label == "Add Equalizer") { + addable.on_add(ImVec2(0, 0)); + clicked = true; + } + } + REQUIRE(clicked); + REQUIRE(app.isDirty() == true); +} +``` + +- [ ] **Step 8: Build + run tests** + +Run: `cmake --build build && build/bin/test_component_dispatch.exe` +Expected: PASS (Equalizer dirty regression green). Also run `build/bin/tests.exe` and `build/bin/test_ui.exe` (Xvfb on Linux; on Windows run directly) — all pass, including UI tests that click `Add Splitter` / `Add Mixer` / `Add Coax Cable` menu items (labels unchanged). + +- [ ] **Step 9: Commit** + +```bash +git add tests/test_component_dispatch.cpp tests/CMakeLists.txt node_graph/include/node_graph_widget.h node_graph/src/node_graph_widget.cpp app/include/app.h app/src/app.cpp +git commit -m "feat: data-driven canvas menu with unified addComponent path" +``` + +--- + +### Task 5: Inspector dispatch through the registry + +**Files:** +- Modify: `app/include/inspector_panel.h` +- Modify: `app/src/inspector_panel.cpp` +- Modify: `app/src/app.cpp` (call `registerDrawers`) + +**Interfaces:** +- Consumes: `ComponentTypeDescriptor::draw_inspector`, `type_name()`. +- Produces: `Hit { const ComponentTypeDescriptor* desc; IComponentEngine* engine; }` (the private `ComponentType` enum is deleted); `findSelected()` via `registry.find(engine->type_name())`; `draw()` dispatches via `desc->draw_inspector`; `InspectorPanel::registerDrawers(ComponentTypeRegistry&)` assigns the per-type lambdas; `drawXProperties` methods become public. + +- [ ] **Step 1: Header changes** + +`app/include/inspector_panel.h`: +- Add `#include "component_type_registry.h"`. +- Delete the `enum class ComponentType { ... };` block. +- Change `Hit` to: + +```cpp + struct Hit { + const ComponentTypeDescriptor *desc = nullptr; + IComponentEngine *engine = nullptr; + }; +``` + +- Move the 11 `drawXProperties` + `drawPFBProperties` + `drawGroupPanel` declarations from `private:` to `public:`. +- In the public section add: + +```cpp + // Called once at startup; wires ComponentTypeRegistry draw_inspector + // callbacks to this panel's property drawers. + void registerDrawers(ComponentTypeRegistry ®istry); +``` + +- [ ] **Step 2: findSelected + labelForHit** + +`app/src/inspector_panel.cpp` `findSelected` — replace the body after the `engine` null-check with: + +```cpp + return {ComponentTypeRegistry::instance().find(engine->type_name()), engine}; +``` + +`labelForHit` — replace the whole body with: + +```cpp +std::string InspectorPanel::labelForHit(const Hit &hit) const { + if (!hit.desc || !hit.engine) + return ""; + return hit.desc->display_name + " " + std::to_string(hit.engine->id()); +} +``` + +> This also fixes the latent bug where CoaxCable/Equalizer fell through to an empty panel title. + +- [ ] **Step 3: draw() dispatch** + +`inspector_panel.cpp` `draw` — replace the big `switch (hit.type) { ... }` with: + +```cpp + if (hit.desc->type == "pfb") { + // PFB keeps its multi-instance selector combo (needs m_pfb_ptrs). + auto *pfb = static_cast(hit.engine); + for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { + if (m_pfb_ptrs[i] == pfb) { + m_selected_pfb_index = i; + break; + } + } + if (!m_pfb_ptrs.empty()) { + int display_id = (m_selected_pfb_index < static_cast(m_pfb_ptrs.size()) && + m_pfb_ptrs[m_selected_pfb_index]) + ? m_pfb_ptrs[m_selected_pfb_index]->id() + : m_selected_pfb_index; + std::string combo_label = "PFB##selector"; + std::string preview = "PFB " + std::to_string(display_id); + if (ImGui::BeginCombo(combo_label.c_str(), preview.c_str())) { + for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { + if (!m_pfb_ptrs[i]) + continue; + bool selected = (i == m_selected_pfb_index); + std::string item = "PFB " + std::to_string(m_pfb_ptrs[i]->id()); + if (ImGui::Selectable(item.c_str(), &selected)) + m_selected_pfb_index = i; + if (selected) + ImGui::SetItemDefaultFocus(); + } + ImGui::EndCombo(); + } + } + } + + if (hit.desc->draw_inspector) + hit.desc->draw_inspector(*this, *hit.engine); + + if (m_param_edited && onParamChange) + onParamChange(); +``` + +> The PFB block previously ended by calling `drawPFBProperties` inside the switch; that call now happens via the PFB `draw_inspector` lambda in `registerDrawers` (Step 4). + +- [ ] **Step 4: registerDrawers** + +`inspector_panel.cpp` — add this member definition: + +```cpp +void InspectorPanel::registerDrawers(ComponentTypeRegistry ®istry) { + for (auto *d : registry.all()) { + if (d->type == "generator") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawGeneratorProperties(static_cast(e), e.id()); + }; + } else if (d->type == "amplifier") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawAmplifierProperties(static_cast(e), e.id()); + }; + } else if (d->type == "splitter") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawSplitterProperties(static_cast(e), e.id()); + }; + } else if (d->type == "mixer") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawMixerProperties(static_cast(e), e.id()); + }; + } else if (d->type == "adc") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawAdcProperties(static_cast(e), e.id()); + }; + } else if (d->type == "pfb") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawPFBProperties(static_cast(e)); + }; + } else if (d->type == "filter") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawIdealFilterProperties(static_cast(e), e.id()); + }; + } else if (d->type == "coax") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawCoaxCableProperties(static_cast(e), e.id()); + }; + } else if (d->type == "equalizer") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawEqualizerProperties(static_cast(e), e.id()); + }; + } else if (d->type == "attenuator") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawAttenuatorProperties(static_cast(e), e.id()); + }; + } else if (d->type == "combiner") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawCombinerProperties(static_cast(e), e.id()); + }; + } + } +} +``` + +- [ ] **Step 5: Call registerDrawers at startup** + +`app/src/app.cpp` constructor — right after `m_inspector_panel = std::make_unique(...)` add: + +```cpp + m_inspector_panel->registerDrawers(ComponentTypeRegistry::instance()); +``` + +- [ ] **Step 6: Build** + +Run: `cmake --build build` +Expected: compiles clean. + +- [ ] **Step 7: Run tests** + +Run: `build/bin/tests.exe` and `build/bin/test_ui.exe`. +Expected: all pass (UI tests still click the same menu labels; inspector behavior unchanged). + +- [ ] **Step 8: Commit** + +```bash +git add app/include/inspector_panel.h app/src/inspector_panel.cpp app/src/app.cpp +git commit -m "refactor: inspector dispatch through ComponentTypeRegistry" +``` + +--- + +### Task 6: Data-driven node-kind mapping (delete nodeKindFromLabel) + +**Files:** +- Modify: `node_graph/include/node_graph_engine.h` +- Modify: `node_graph/include/node_graph_widget.h` +- Modify: `node_graph/src/node_graph_widget.cpp` +- Modify: `app/src/app.cpp` +- Modify: `tests/test_node_graph_engine.cpp` +- Modify: `tests/test_component_dispatch.cpp` + +**Interfaces:** +- Consumes: descriptor `label_prefix` + `kind` from the registry. +- Produces: `NodeGraphWidget::registerNodeKind(std::string label_prefix, NodeKind kind)` and public `kindForLabel(const std::string&)` replacing `nodeKindFromLabel`. + +- [ ] **Step 1: Delete nodeKindFromLabel from the engine header** + +`node_graph/include/node_graph_engine.h` — delete the `nodeKindFromLabel` inline function (the 11-branch prefix chain). Keep `NodeKind` enum and `themeColor` (still used by the widget). + +- [ ] **Step 2: Widget header — register + lookup** + +`node_graph/include/node_graph_widget.h` — public section add: + +```cpp + void registerNodeKind(std::string label_prefix, NodeKind kind) { + m_kind_prefixes.push_back({std::move(label_prefix), kind}); + } + NodeKind kindForLabel(const std::string &label) const; +``` + +private section add: + +```cpp + std::vector> m_kind_prefixes; +``` + +- [ ] **Step 3: Widget cpp — implement kindForLabel + use it** + +`node_graph/src/node_graph_widget.cpp` — add: + +```cpp +NodeKind NodeGraphWidget::kindForLabel(const std::string &label) const { + for (const auto &[prefix, kind] : m_kind_prefixes) + if (label.rfind(prefix, 0) == 0) + return kind; + return NodeKind::Unknown; +} +``` + +Replace the call site in `drawNodes` (currently `const NodeKind kind = nodeKindFromLabel(node.label);`) with: + +```cpp + const NodeKind kind = kindForLabel(node.label); +``` + +- [ ] **Step 4: App feeds the mapping** + +`app/src/app.cpp` constructor — inside the registry loop that builds the addable list, add: + +```cpp + m_graph_widget->registerNodeKind(desc->label_prefix, desc->kind); +``` + +- [ ] **Step 5: Drop the obsolete nodeKindFromLabel tests** + +`tests/test_node_graph_engine.cpp` — delete the two TEST_CASEs `"nodeKindFromLabel maps known prefixes"` and `"nodeKindFromLabel returns Unknown for unrecognised input"`. KEEP the `themeColor` TEST_CASE. + +- [ ] **Step 6: Add the replacement registry-driven test** + +`tests/test_component_dispatch.cpp` — append: + +```cpp +TEST_CASE_METHOD(ImGuiFixture, "Every registry label_prefix maps to its kind", "[dispatch]") { + RfSimulatorApp app; + for (const auto *d : ComponentTypeRegistry::instance().all()) { + REQUIRE(app.testGraphWidget().kindForLabel(d->label_prefix + " 1") == d->kind); + } + REQUIRE(app.testGraphWidget().kindForLabel("UnknownThing 1") == NodeKind::Unknown); +} +``` + +Add `#include "component_type_registry.h"` to the test file. + +- [ ] **Step 7: Build + run tests** + +Run: `cmake --build build && build/bin/test_component_dispatch.exe`, then `build/bin/tests.exe` and `build/bin/test_ui.exe`. +Expected: all pass. + +- [ ] **Step 8: Commit** + +```bash +git add node_graph/include/node_graph_engine.h node_graph/include/node_graph_widget.h node_graph/src/node_graph_widget.cpp app/src/app.cpp tests/test_node_graph_engine.cpp tests/test_component_dispatch.cpp +git commit -m "refactor: data-driven NodeKind mapping in node graph widget" +``` + +--- + +### Task 7: New Component form combo from the registry + +**Files:** +- Modify: `app/src/app.cpp` (`drawComponentFormModal`) + +**Interfaces:** +- Consumes: `ComponentTypeDescriptor::authorable`. + +- [ ] **Step 1: Replace the hardcoded combo** + +`app/src/app.cpp` `drawComponentFormModal` — inside the `if (!m_component_form_is_edit)` block, replace the hardcoded `type_names[]` + combo with: + +```cpp + std::vector authorable; + for (auto *d : ComponentTypeRegistry::instance().all()) + if (d->authorable) + authorable.push_back(d); + static int type_idx = 0; + for (size_t i = 0; i < authorable.size(); ++i) + if (m_component_form_model->descriptor().type == authorable[i]->type) + type_idx = static_cast(i); + std::vector type_names; + for (auto *d : authorable) + type_names.push_back(d->type.c_str()); + if (ImGui::Combo("Type", &type_idx, type_names.data(), + static_cast(type_names.size()))) + openNewComponentForm(authorable[type_idx]->type); +``` + +- [ ] **Step 2: Build** + +Run: `cmake --build build` +Expected: compiles clean. + +- [ ] **Step 3: Run authoring tests** + +Run: `build/bin/test_component_authoring.exe`. +Expected: all pass (form model unchanged; combo now derived from registry `authorable` rows, same 8 types in the same order). + +- [ ] **Step 4: Commit** + +```bash +git add app/src/app.cpp +git commit -m "refactor: New Component form combo driven by registry authorable types" +``` + +--- + +## Phase 2 — PFBViewManager + +### Task 8: PFBViewManager owns the lockstep PFB widget vectors + +**Files:** +- Create: `app/include/pfb_view_manager.h` +- Create: `app/src/pfb_view_manager.cpp` +- Modify: `app/CMakeLists.txt` +- Modify: `app/include/app.h` +- Modify: `app/src/app.cpp` + +**Interfaces:** +- Consumes: `ComponentRegistry::byType()`, `SessionState`. +- Produces: `PFBViewManager` with `addFor(engine, state)`, `rebuild(components, state)`, `clear()`, `draw()`, `saveVisibility(components, state)`, `iqVisibility()`, `gridVisibility()`. Replaces the app's `m_iq_widgets`/`m_show_iq_pfbs`/`m_pfb_grid_widgets`/`m_show_pfb_grids` members and all six rebuild sites. + +- [ ] **Step 1: Write the header** + +`app/include/pfb_view_manager.h`: + +```cpp +#pragma once + +#include "iq_plot_widget.h" +#include "pfb_channelizer_engine.h" +#include "pfb_channelizer_widget.h" +#include +#include + +class ComponentRegistry; +class SessionState; + +// Owns the per-PFB view widgets and their visibility flags. The app's old +// four lockstep vectors (m_iq_widgets/m_show_iq_pfbs/m_pfb_grid_widgets/ +// m_show_pfb_grids) were rebuilt by hand at six call sites and caused issue +// #37 (use-after-free). All lifecycle now funnels through this class. +class PFBViewManager { + public: + void addFor(PFBChannelizerEngine &engine, SessionState &state); + void rebuild(const ComponentRegistry &components, SessionState &state); + void clear(); + void draw(); + void saveVisibility(const ComponentRegistry &components, SessionState &state) const; + + std::vector &iqVisibility() { return m_show_iq_pfbs; } + std::vector &gridVisibility() { return m_show_pfb_grids; } + + private: + std::vector> m_iq_widgets; + std::vector m_show_iq_pfbs; + std::vector> m_pfb_grid_widgets; + std::vector m_show_pfb_grids; +}; +``` + +- [ ] **Step 2: Write the implementation** + +`app/src/pfb_view_manager.cpp`: + +```cpp +#include "pfb_view_manager.h" +#include "component_registry.h" +#include "session_state.h" + +void PFBViewManager::addFor(PFBChannelizerEngine &engine, SessionState &state) { + m_iq_widgets.push_back(std::make_unique(engine)); + m_show_iq_pfbs.push_back( + state.loadBool("WindowState", ("IQPlot_" + std::to_string(engine.id())).c_str(), true)); + m_pfb_grid_widgets.push_back(std::make_unique(engine)); + m_show_pfb_grids.push_back( + state.loadBool("WindowState", ("PFBGrid_" + std::to_string(engine.id())).c_str(), true)); +} + +void PFBViewManager::rebuild(const ComponentRegistry &components, SessionState &state) { + clear(); + for (auto *pfb : components.byType()) + addFor(*pfb, state); +} + +void PFBViewManager::clear() { + m_iq_widgets.clear(); + m_show_iq_pfbs.clear(); + m_pfb_grid_widgets.clear(); + m_show_pfb_grids.clear(); +} + +void PFBViewManager::draw() { + for (size_t i = 0; i < m_iq_widgets.size(); ++i) { + if (m_show_iq_pfbs[i]) { + std::string label = "IQ Plot - PFB " + std::to_string(i); + bool show = m_show_iq_pfbs[i]; + m_iq_widgets[i]->draw(label.c_str(), &show); + m_show_iq_pfbs[i] = show; + } + } + for (size_t i = 0; i < m_pfb_grid_widgets.size(); ++i) { + if (m_show_pfb_grids[i]) { + std::string label = "Channelizer Grid - PFB " + std::to_string(i); + bool show = m_show_pfb_grids[i]; + m_pfb_grid_widgets[i]->draw(label.c_str(), &show); + m_show_pfb_grids[i] = show; + } + } +} + +void PFBViewManager::saveVisibility(const ComponentRegistry &components, + SessionState &state) const { + auto pfb_vec = components.byType(); + for (size_t i = 0; i < m_show_iq_pfbs.size() && i < pfb_vec.size(); ++i) { + std::string key = "IQPlot_" + std::to_string(pfb_vec[i]->id()); + state.saveBool("WindowState", key.c_str(), m_show_iq_pfbs[i]); + } + for (size_t i = 0; i < m_show_pfb_grids.size() && i < pfb_vec.size(); ++i) { + std::string key = "PFBGrid_" + std::to_string(pfb_vec[i]->id()); + state.saveBool("WindowState", key.c_str(), m_show_pfb_grids[i]); + } +} +``` + +- [ ] **Step 3: CMake** + +`app/CMakeLists.txt` — add `src/pfb_view_manager.cpp` to the `add_library(app STATIC ...)` list. + +- [ ] **Step 4: App header — swap the four vectors for the manager** + +`app/include/app.h` — delete members `m_iq_widgets`, `m_show_iq_pfbs`, `m_pfb_grid_widgets`, `m_show_pfb_grids`. Add `#include "pfb_view_manager.h"` to the include block (alphabetical). Add (declared AFTER `m_components` so the manager is destroyed before the engines its widgets reference): + +```cpp + PFBViewManager m_pfb_views; +``` + +- [ ] **Step 5: App cpp — replace the six rebuild sites** + +1. `addComponent` PFB block → replace the four pushes with: + +```cpp + m_pfb_views.addFor(*static_cast(comp), m_state); +``` + +2. `onRemoveNode` lambda — replace the `pfb_vec` + four clear/push blocks with: + +```cpp + m_pfb_views.rebuild(m_components, m_state); +``` + +3. `duplicateComponent` PFB block → replace the four pushes with: + +```cpp + m_pfb_views.addFor(*static_cast(copy), m_state); +``` + +4. `newProject` — replace the four `clear()` calls with `m_pfb_views.clear();` + +5. `loadProject` PFB block → replace the four pushes with: + +```cpp + m_pfb_views.addFor(*static_cast(comp), m_state); +``` + +6. `~RfSimulatorApp` — replace the two `m_show_iq_pfbs`/`m_show_pfb_grids` save loops with: + +```cpp + m_pfb_views.saveVisibility(m_components, m_state); +``` + +Also replace the `draw_ui()` IQ-plot and channelizer-grid loops (the two `for (size_t i = 0; i < m_iq_widgets.size(); ...)` blocks) with: + +```cpp + m_pfb_views.draw(); +``` + +And the `InspectorPanel::setPFBWindowVisibility` call site (around line 975) becomes: + +```cpp + m_inspector_panel->setPFBWindowVisibility(&m_pfb_views.iqVisibility(), + &m_pfb_views.gridVisibility()); +``` + +- [ ] **Step 6: Build** + +Run: `cmake --build build` +Expected: compiles clean. + +- [ ] **Step 7: Run tests** + +Run: `build/bin/test_issue37_pfb_input_removal.exe`, `build/bin/test_project_file.exe`, `build/bin/test_component_dispatch.exe`, `build/bin/tests.exe`. +Expected: all pass — especially issue #37 (PFB input removal) which exercises the rebuilt widget lifecycle. + +- [ ] **Step 8: Commit** + +```bash +git add app/include/pfb_view_manager.h app/src/pfb_view_manager.cpp app/CMakeLists.txt app/include/app.h app/src/app.cpp +git commit -m "refactor: extract PFBViewManager from RfSimulatorApp" +``` + +--- + +## Phase 3 — ProjectSerializer + +### Task 9: ProjectSerializer owns save/load/new JSON logic + +**Files:** +- Create: `app/include/project_serializer.h` +- Create: `app/src/project_serializer.cpp` +- Modify: `app/CMakeLists.txt` +- Modify: `app/include/app.h` +- Modify: `app/src/app.cpp` + +**Interfaces:** +- Consumes: `ComponentRegistry`, `NodeGraphEngine`, `NodeGraphWidget` (imnodes positions), `PFBViewManager`, `SessionState`, id counter. +- Produces: `ProjectSerializer` with `save(path)`, `load(path) -> bool`, `reset()`; `RfSimulatorApp::saveProject`/`loadProject`/`newProject` become thin wrappers. + +- [ ] **Step 1: Write the header** + +`app/include/project_serializer.h`: + +```cpp +#pragma once + +#include + +class ComponentRegistry; +class NodeGraphEngine; +class NodeGraphWidget; +class PFBViewManager; +class SessionState; + +// Owns the .rfsim JSON save/load/new logic previously inlined in +// RfSimulatorApp (issue #51: 1320-line god-object). +class ProjectSerializer { + public: + ProjectSerializer(ComponentRegistry &components, NodeGraphEngine &graph, + NodeGraphWidget &graph_widget, PFBViewManager &pfb_views, + SessionState &state, int &next_component_id); + + void save(const std::string &path); + bool load(const std::string &path); // false on parse/unknown-type failure (logged) + void reset(); // newProject: links, components, probes, counters, PFBs + + private: + ComponentRegistry &m_components; + NodeGraphEngine &m_graph; + NodeGraphWidget &m_graph_widget; + PFBViewManager &m_pfb_views; + SessionState &m_state; + int &m_next_component_id; +}; +``` + +- [ ] **Step 2: Move save/load/new bodies** + +`app/src/project_serializer.cpp` — move these bodies VERBATIM from `app/src/app.cpp`, with these substitutions: + +- `save(const std::string &path)`: copy the current `RfSimulatorApp::saveProject` body (lines ~413-560). Replace member accesses: `m_graph_engine` → `m_graph`, `m_graph_widget->` → `m_graph_widget.`, `m_components` → `m_components`, `m_next_component_id` → `m_next_component_id`. The registry type lookup from Task 3 stays. DELETE the tail lines `m_current_project_path = path; m_dirty = false;` — those become app-wrapper responsibilities. +- `load(const std::string &path) -> bool`: copy the current `loadProject` body (lines ~565-740). Keep the `if (!in)` / catch / reset-at-start; replace the internal `newProject();` call with `reset();`. Keep the `LOG_WARN` unknown-type path. DELETE the tail `m_current_project_path = path; refreshExtensions(); m_dirty = false;` — app wrapper handles those. Return `true` after `LOG_INFO("Loaded project from %s", path.c_str());` and `false` on the two early-return error paths (open failure, parse failure). +- `reset()`: copy the current `newProject` body minus the app-owned lines. KEEP: `m_graph.removeAllLinks()`, component removal loop, `m_graph.clearProbes()`, `m_pfb_views.clear()`, `m_graph.setNextIds(...)`, `setNextGroupId(...)`, `setNextBoundaryPinId(...)`, `m_next_component_id = 100`, `m_graph_widget.clearPositionCache()`. LEAVE in the app wrapper: `m_spectrum_widget->setProbeLabels({})`, `m_current_project_path.clear()`, `refreshExtensions()`, `m_dirty = false`. + +- [ ] **Step 3: CMake** + +`app/CMakeLists.txt` — add `src/project_serializer.cpp` to `add_library(app STATIC ...)`. + +- [ ] **Step 4: App header** + +`app/include/app.h` — add `#include "project_serializer.h"`; add member (declared AFTER `m_graph_widget` and `m_pfb_views` so it is constructed after them): + +```cpp + std::unique_ptr m_serializer; +``` + +- [ ] **Step 5: App cpp — thin wrappers** + +In the constructor body, right after `m_graph_widget = std::make_unique(m_graph_engine);` (m_pfb_views and m_components already exist as members): + +```cpp + m_serializer = std::make_unique( + m_components, m_graph_engine, *m_graph_widget, m_pfb_views, m_state, m_next_component_id); +``` + +Replace the bodies of `saveProject`, `loadProject`, `newProject` with: + +```cpp +void RfSimulatorApp::saveProject(const std::string &path) { + m_serializer->save(path); + m_current_project_path = path; + m_dirty = false; +} + +void RfSimulatorApp::loadProject(const std::string &path) { + if (!m_serializer->load(path)) + return; + m_current_project_path = path; + refreshExtensions(); + m_dirty = false; +} + +void RfSimulatorApp::newProject() { + m_serializer->reset(); + m_spectrum_widget->setProbeLabels({}); + m_current_project_path.clear(); + refreshExtensions(); + m_dirty = false; +} +``` + +- [ ] **Step 6: Build** + +Run: `cmake --build build` +Expected: compiles clean. + +- [ ] **Step 7: Run round-trip tests** + +Run: `build/bin/test_project_file.exe`, `build/bin/test_component_dispatch.exe`, `build/bin/test_issue37_pfb_input_removal.exe`, `build/bin/tests.exe`. +Expected: all pass (round-trips, groups, positions, probes, counters, PFB lifecycle all preserved). + +- [ ] **Step 8: Commit** + +```bash +git add app/include/project_serializer.h app/src/project_serializer.cpp app/CMakeLists.txt app/include/app.h app/src/app.cpp +git commit -m "refactor: extract ProjectSerializer from RfSimulatorApp" +``` + +--- + +### Task 10: Round-trip + backward-compat tests in the dispatch exe + +**Files:** +- Modify: `tests/test_component_dispatch.cpp` + +**Interfaces:** +- Consumes: everything from Tasks 1-9. + +- [ ] **Step 1: Add all-11-types round-trip + legacy file test** + +Append to `tests/test_component_dispatch.cpp`: + +```cpp +TEST_CASE_METHOD(ImGuiFixture, "All 11 registry types round-trip through project save/load", + "[dispatch]") { + auto path = "test_dispatch_all_types.rfsim"; + std::remove(path); + { + RfSimulatorApp app; + app.newProject(); + for (const auto &addable : app.testGraphWidget().addableComponents()) + addable.on_add(ImVec2(0, 0)); + REQUIRE(app.componentCount() == 11); + app.saveProject(path); + } + { + RfSimulatorApp app; + app.loadProject(path); + REQUIRE(app.componentCount() == 11); + } + std::remove(path); +} + +TEST_CASE_METHOD(ImGuiFixture, "Legacy .rfsim type strings still load (backward compat)", + "[dispatch]") { + auto path = "test_dispatch_legacy.rfsim"; + std::ofstream out(path); + out << R"({ + "version": 1, + "name": "legacy", + "components": [ + {"type": "SignalGenerator", "params": {"tones": [{"freq_Hz": 100e6, "power_dBm": -20.0, "phase_deg": 0.0}]}}, + {"type": "Amplifier", "params": {"gain_dB": 10.0, "nf_dB": 2.0}}, + {"type": "ADC", "params": {"sample_rate_Hz": 1e9}}, + {"type": "IdealFilter", "params": {"filter_type": 1}}, + {"type": "PFBChannelizer", "params": {}}, + {"type": "CoaxCable", "params": {}} + ], + "links": [], + "groups": [] + })"; + out.close(); + RfSimulatorApp app; + app.loadProject(path); + REQUIRE(app.componentCount() == 6); + REQUIRE(app.testComponents().byType().size() == 1); + std::remove(path); +} +``` + +Add includes at the top: `#include "pfb_channelizer_engine.h"`, `#include `, `#include `. + +- [ ] **Step 2: Build + run** + +Run: `cmake --build build && build/bin/test_component_dispatch.exe` +Expected: PASS — all 11 types round-trip; legacy type strings (capitalized project names) load; canonical names also accepted by `findByProjectType`. + +- [ ] **Step 3: Commit** + +```bash +git add tests/test_component_dispatch.cpp +git commit -m "test: all-types round-trip and legacy .rfsim backward compat" +``` + +--- + +### Task 11: DOX pass + docs + final verification + +**Files:** +- Modify: `app/AGENTS.md` +- Modify: `common/AGENTS.md` +- Modify: `openwiki/testing/guidance.md` + +- [ ] **Step 1: Update app/AGENTS.md** + +- Ownership: add `PFBViewManager` (owns per-PFB IQ/grid widget lifecycle, replaces the app's four lockstep vectors) and `ProjectSerializer` (owns `.rfsim` save/load/new JSON logic). Update `ComponentTypeRegistry` ownership text: now the single dispatch table for add/menu/duplicate/save/load/inspector drawing, with `create()` factories and `draw_inspector` callbacks. +- Work Guidance: replace "Add new component serialization in both saveProject() and loadProject()" with "Add a new component = one ComponentTypeRegistry row (type, project_type, menu_label, label_prefix, kind, create, draw_inspector) + a NodeKind/symbol entry in node_graph". +- Keep the issue #37 / rewireInputs contract and PFB visibility note (now on `PFBViewManager::iqVisibility`/`gridVisibility`). + +- [ ] **Step 2: Update common/AGENTS.md** + +- Ownership/Work Guidance: note `IComponentEngine` now requires `type_name()` (pure virtual, canonical lowercase key) — new engines must implement it; update the "New fields on IComponentEngine must keep a default implementation" contract line to exempt `type_name()` (intentionally pure). + +- [ ] **Step 3: Update openwiki/testing/guidance.md** + +- Add `test_component_dispatch.cpp` to the standalone-executables list (alongside `test_issue37_pfb_input_removal`, `test_extensions`, `test_component_authoring`, `test_signal_domain`). + +- [ ] **Step 4: Format + full suite** + +Run: `scripts/format.sh` then `scripts/format.sh --check`; `cmake --build build`; `ctest --test-dir build --output-on-failure`. +Expected: zero failures. + +- [ ] **Step 5: Commit** + +```bash +git add app/AGENTS.md common/AGENTS.md openwiki/testing/guidance.md +git commit -m "docs: update DOX for unified registry, PFBViewManager, ProjectSerializer" +``` + +--- + +## Self-Review + +**Spec coverage:** every item in the approved spec maps to a task — registration object + `type_name()` (T1-T3), canvas menu + Equalizer fix (T4), inspector (T5), node-kind data-driven (T6), form combo (T7), PFBViewManager (T8), ProjectSerializer (T9), round-trip + backward-compat tests (T10), DOX (T11). + +**Placeholders:** none — all new files have full code; all modified bodies show the exact replacement. + +**Type consistency:** `type_name()` canonical keys match registry `type` values exactly (`generator`, `amplifier`, `splitter`, `mixer`, `adc`, `pfb`, `coax`, `equalizer`, `filter`, `attenuator`, `combiner`). `project_type` strings match today's `.rfsim` output. `findByProjectType` accepts both. `create()` returns `IComponentEngine*`; `draw_inspector` is `std::function` — signatures match between T2a, T2b, T3, T5. `kindForLabel` is public (T6) so the test can call it. + +**Known risks flagged for the implementer:** +- Task 2a is additive and commits green; Task 2b removes `factory` and is the compile-atomic change with `instantiate` rewire. +- Task 4's red step uses the OLD `onAddEqualizer` API; Step 7 updates the test to the new menu API in the SAME task, so no red commit lands. +- The deserialize key-parity edits (T2b Steps 2-6) are behavior-preserving only if applied exactly — they keep library JSON loading identical after `instantiate()` switches to `create()`+`deserialize()`. From 8c042222324b975e46f1ef8afbf2e89bd4a97418 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 17:28:36 +0200 Subject: [PATCH 03/15] refactor: add type_name() virtual to IComponentEngine --- adc/include/adc_engine.h | 1 + amplifier/include/amplifier_engine.h | 1 + attenuator/include/attenuator_engine.h | 1 + coax/include/coax_cable_engine.h | 1 + combiner/include/combiner_engine.h | 1 + common/component_interface.h | 5 +++++ equalizer/include/equalizer_engine.h | 1 + ideal_filter/include/ideal_filter_engine.h | 1 + mixer/include/mixer_engine.h | 1 + pfb_channelizer/include/pfb_channelizer_engine.h | 1 + signal_generator/include/signal_generator_engine.h | 1 + splitter/include/splitter_engine.h | 1 + tests/test_component_registry.cpp | 2 ++ 13 files changed, 18 insertions(+) diff --git a/adc/include/adc_engine.h b/adc/include/adc_engine.h index f0d534c..084d545 100644 --- a/adc/include/adc_engine.h +++ b/adc/include/adc_engine.h @@ -12,6 +12,7 @@ class AdcEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "adc"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override; diff --git a/amplifier/include/amplifier_engine.h b/amplifier/include/amplifier_engine.h index e8c6562..761cc4f 100644 --- a/amplifier/include/amplifier_engine.h +++ b/amplifier/include/amplifier_engine.h @@ -12,6 +12,7 @@ class AmplifierEngine : public IComponentEngine { AmplifierEngine(int id, NodeGraphEngine &graph); int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "amplifier"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override; diff --git a/attenuator/include/attenuator_engine.h b/attenuator/include/attenuator_engine.h index 9f83608..677fb8f 100644 --- a/attenuator/include/attenuator_engine.h +++ b/attenuator/include/attenuator_engine.h @@ -14,6 +14,7 @@ class AttenuatorEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "attenuator"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override { return outputPinId(0); } diff --git a/coax/include/coax_cable_engine.h b/coax/include/coax_cable_engine.h index 5d40dba..7339e17 100644 --- a/coax/include/coax_cable_engine.h +++ b/coax/include/coax_cable_engine.h @@ -12,6 +12,7 @@ class CoaxCableEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "coax"; } int inputPinId() const override; int outputPinId() const override; diff --git a/combiner/include/combiner_engine.h b/combiner/include/combiner_engine.h index 1fa69e1..57af50f 100644 --- a/combiner/include/combiner_engine.h +++ b/combiner/include/combiner_engine.h @@ -13,6 +13,7 @@ class CombinerEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "combiner"; } std::string hoverSummary() const override; int inputPinId() const override { return inputPinId(0); } diff --git a/common/component_interface.h b/common/component_interface.h index 0908518..252f1a5 100644 --- a/common/component_interface.h +++ b/common/component_interface.h @@ -3,6 +3,7 @@ #include "signal_node.h" #include #include +#include class IComponentEngine { public: @@ -25,6 +26,10 @@ class IComponentEngine { virtual int numInputPins() const { return 1; } virtual int numOutputPins() const { return 1; } + // Canonical type key (e.g. "amplifier"). Single source of truth for the + // component-type dispatch tables (save/load, duplicate, inspector, menu). + virtual std::string_view type_name() const = 0; + // Serialization — default no-op virtual nlohmann::json serialize() const { return nlohmann::json::object(); } virtual void deserialize(const nlohmann::json &) {} diff --git a/equalizer/include/equalizer_engine.h b/equalizer/include/equalizer_engine.h index ecc3074..5c509c4 100644 --- a/equalizer/include/equalizer_engine.h +++ b/equalizer/include/equalizer_engine.h @@ -13,6 +13,7 @@ class EqualizerEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "equalizer"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override; diff --git a/ideal_filter/include/ideal_filter_engine.h b/ideal_filter/include/ideal_filter_engine.h index acf763d..b881718 100644 --- a/ideal_filter/include/ideal_filter_engine.h +++ b/ideal_filter/include/ideal_filter_engine.h @@ -12,6 +12,7 @@ class IdealFilterEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "filter"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override; diff --git a/mixer/include/mixer_engine.h b/mixer/include/mixer_engine.h index bf13385..7ee0f12 100644 --- a/mixer/include/mixer_engine.h +++ b/mixer/include/mixer_engine.h @@ -11,6 +11,7 @@ class MixerEngine : public IComponentEngine { MixerEngine(int id, NodeGraphEngine &graph); int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "mixer"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override; diff --git a/pfb_channelizer/include/pfb_channelizer_engine.h b/pfb_channelizer/include/pfb_channelizer_engine.h index d4ee66e..7a2881f 100644 --- a/pfb_channelizer/include/pfb_channelizer_engine.h +++ b/pfb_channelizer/include/pfb_channelizer_engine.h @@ -31,6 +31,7 @@ class PFBChannelizerEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "pfb"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId() const override; diff --git a/signal_generator/include/signal_generator_engine.h b/signal_generator/include/signal_generator_engine.h index 2dfda3a..995bf04 100644 --- a/signal_generator/include/signal_generator_engine.h +++ b/signal_generator/include/signal_generator_engine.h @@ -9,6 +9,7 @@ class SignalGeneratorEngine : public IComponentEngine { int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "generator"; } std::string hoverSummary() const override; int outputPinId() const override; diff --git a/splitter/include/splitter_engine.h b/splitter/include/splitter_engine.h index ace891c..82c62a4 100644 --- a/splitter/include/splitter_engine.h +++ b/splitter/include/splitter_engine.h @@ -11,6 +11,7 @@ class SplitterEngine : public IComponentEngine { SplitterEngine(int id, NodeGraphEngine &graph); int id() const override { return m_id; } int graphNodeId() const override { return m_graph_node_id; } + std::string_view type_name() const override { return "splitter"; } std::string hoverSummary() const override; int inputPinId() const override; int outputPinId(int index) const override; diff --git a/tests/test_component_registry.cpp b/tests/test_component_registry.cpp index 544ad8b..42068ee 100644 --- a/tests/test_component_registry.cpp +++ b/tests/test_component_registry.cpp @@ -14,6 +14,7 @@ struct TestEngineA : IComponentEngine { } int id() const override { return m_id; } int graphNodeId() const override { return m_graph_id; } + std::string_view type_name() const override { return "test_a"; } int outputPinId() const override { return 1; } std::string hoverSummary() const override { return "TestA(" + std::to_string(m_id) + ")"; } SignalNode &node() override { return m_node; } @@ -30,6 +31,7 @@ struct TestEngineB : IComponentEngine { } int id() const override { return m_id; } int graphNodeId() const override { return m_graph_id; } + std::string_view type_name() const override { return "test_b"; } int outputPinId() const override { return 2; } int inputPinId() const override { return 10; } std::string hoverSummary() const override { return "TestB(" + std::to_string(m_id) + ")"; } From 1133e15ad0337a84e3243d2c4a66a99839f5dc01 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 17:36:09 +0200 Subject: [PATCH 04/15] refactor: extend ComponentTypeRegistry to all 11 types --- app/include/component_type_registry.h | 32 +++++-- app/src/component_type_registry.cpp | 124 ++++++++++++++++++++++++-- tests/test_component_authoring.cpp | 7 +- 3 files changed, 147 insertions(+), 16 deletions(-) diff --git a/app/include/component_type_registry.h b/app/include/component_type_registry.h index 7c5cdce..945fb49 100644 --- a/app/include/component_type_registry.h +++ b/app/include/component_type_registry.h @@ -1,15 +1,17 @@ -// app/include/component_type_registry.h #pragma once +#include "node_graph_engine.h" #include #include #include #include +#include #include class ComponentRegistry; class NodeGraphEngine; class IComponentEngine; +class InspectorPanel; enum class FieldKind { Number, String, Enum, FilePath, Bool }; @@ -27,10 +29,25 @@ struct ParameterField { }; struct ComponentTypeDescriptor { - std::string type; // "amplifier" - std::string display_name; // "Amplifier" - std::vector fields; + std::string type; // canonical key, e.g. "amplifier" + std::string project_type; // .rfsim save/load name, e.g. "Amplifier" + std::string display_name; // e.g. "Amplifier" + std::string menu_label; // canvas menu item, e.g. "Add Amplifier" + std::string label_prefix; // graph label prefix, e.g. "Amplifier" + NodeKind kind = NodeKind::Unknown; + bool authorable = false; // appears in New Component form combo bool supports_sparam_file = false; + std::vector fields; + + // Create a default engine of this type (no params). Callers apply params + // via engine->deserialize(). Replaces the old params-taking `factory`. + std::function create; + // Inspector property draw. Receives the panel so PFB's multi-instance + // selector and dirty-flag state stay reachable. + std::function draw_inspector; + + // Legacy params-taking factory; still used by ComponentLibrary until + // Task 2b migrates instantiate to create()+deserialize(). std::function factory; @@ -38,10 +55,11 @@ struct ComponentTypeDescriptor { class ComponentTypeRegistry { public: - static const ComponentTypeRegistry &instance(); + static ComponentTypeRegistry &instance(); - const ComponentTypeDescriptor *find(const std::string &type) const; - std::vector all() const; + const ComponentTypeDescriptor *find(std::string_view type) const; + const ComponentTypeDescriptor *findByProjectType(std::string_view name) const; + std::vector all(); private: ComponentTypeRegistry(); diff --git a/app/src/component_type_registry.cpp b/app/src/component_type_registry.cpp index 8d7aaf0..32cd26b 100644 --- a/app/src/component_type_registry.cpp +++ b/app/src/component_type_registry.cpp @@ -4,29 +4,40 @@ #include "adc_engine.h" #include "amplifier_engine.h" #include "attenuator_engine.h" +#include "coax_cable_engine.h" #include "combiner_engine.h" #include "component_registry.h" #include "equalizer_engine.h" #include "ideal_filter_engine.h" #include "mixer_engine.h" +#include "pfb_channelizer_engine.h" +#include "signal_generator_engine.h" #include "splitter_engine.h" -const ComponentTypeRegistry &ComponentTypeRegistry::instance() { +ComponentTypeRegistry &ComponentTypeRegistry::instance() { static ComponentTypeRegistry reg; return reg; } -const ComponentTypeDescriptor *ComponentTypeRegistry::find(const std::string &type) const { +const ComponentTypeDescriptor *ComponentTypeRegistry::find(std::string_view type) const { for (const auto &d : m_descriptors) if (d.type == type) return &d; return nullptr; } -std::vector ComponentTypeRegistry::all() const { - std::vector result; - result.reserve(m_descriptors.size()); +const ComponentTypeDescriptor * +ComponentTypeRegistry::findByProjectType(std::string_view name) const { for (const auto &d : m_descriptors) + if (d.type == name || d.project_type == name) + return &d; + return nullptr; +} + +std::vector ComponentTypeRegistry::all() { + std::vector result; + result.reserve(m_descriptors.size()); + for (auto &d : m_descriptors) result.push_back(&d); return result; } @@ -34,7 +45,12 @@ std::vector ComponentTypeRegistry::all() const ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor amp; amp.type = "amplifier"; + amp.project_type = "Amplifier"; amp.display_name = "Amplifier"; + amp.menu_label = "Add Amplifier"; + amp.label_prefix = "Amplifier"; + amp.kind = NodeKind::Amplifier; + amp.authorable = true; amp.supports_sparam_file = true; amp.fields = { {"gain_dB", "Gain", "dB", FieldKind::Number, true, -50.0, 100.0, {}, {}, ""}, @@ -43,6 +59,10 @@ ComponentTypeRegistry::ComponentTypeRegistry() { {"oip3_dBm", "OIP3", "dBm", FieldKind::Number, false, -20.0, 100.0, {}, {}, ""}, {"p1db_dBm", "P1dB", "dBm", FieldKind::Number, false, -20.0, 100.0, {}, {}, ""}, }; + amp.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + // Legacy factory: applied library params directly. Removed in Task 2b. amp.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -66,10 +86,18 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor att; att.type = "attenuator"; + att.project_type = "Attenuator"; att.display_name = "Attenuator"; + att.menu_label = "Add Attenuator"; + att.label_prefix = "Attenuator"; + att.kind = NodeKind::Attenuator; + att.authorable = true; att.fields = { {"attenuation_dB", "Attenuation", "dB", FieldKind::Number, true, 0.0, 100.0, {}, {}, ""}, }; + att.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; att.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -81,7 +109,15 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor spl; spl.type = "splitter"; + spl.project_type = "Splitter"; spl.display_name = "Splitter"; + spl.menu_label = "Add Splitter"; + spl.label_prefix = "Splitter"; + spl.kind = NodeKind::Splitter; + spl.authorable = true; + spl.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; spl.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json &) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -91,7 +127,12 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor flt; flt.type = "filter"; - flt.display_name = "Filter"; + flt.project_type = "IdealFilter"; + flt.display_name = "IdealFilter"; + flt.menu_label = "Add Ideal Filter"; + flt.label_prefix = "IdealFilter"; + flt.kind = NodeKind::IdealFilter; + flt.authorable = true; flt.fields = { {"filter_type", "Filter Type", @@ -106,6 +147,9 @@ ComponentTypeRegistry::ComponentTypeRegistry() { {"fc_low_Hz", "Low Cutoff", "Hz", FieldKind::Number, false, 0.0, 1e12, {}, {}, ""}, {"fc_high_Hz", "High Cutoff", "Hz", FieldKind::Number, false, 0.0, 1e12, {}, {}, ""}, }; + flt.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; flt.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -132,7 +176,12 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor mix; mix.type = "mixer"; + mix.project_type = "Mixer"; mix.display_name = "Mixer"; + mix.menu_label = "Add Mixer"; + mix.label_prefix = "Mixer"; + mix.kind = NodeKind::Mixer; + mix.authorable = true; mix.fields = { {"lo_freq_Hz", "LO Frequency", "Hz", FieldKind::Number, true, 0.0, 1e12, {}, {}, ""}, {"conversion_gain_dB", @@ -147,6 +196,9 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ""}, {"nf_dB", "Noise Figure", "dB", FieldKind::Number, false, 0.0, 30.0, {}, {}, ""}, }; + mix.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; mix.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -162,7 +214,12 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor eq; eq.type = "equalizer"; + eq.project_type = "Equalizer"; eq.display_name = "Equalizer"; + eq.menu_label = "Add Equalizer"; + eq.label_prefix = "Equalizer"; + eq.kind = NodeKind::Equalizer; + eq.authorable = true; eq.fields = { {"ref_gain_dB", "Reference Gain", "dB", FieldKind::Number, false, -50.0, 50.0, {}, {}, ""}, {"ref_freq_Hz", @@ -186,6 +243,9 @@ ComponentTypeRegistry::ComponentTypeRegistry() { {}, ""}, }; + eq.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; eq.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -201,10 +261,18 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor comb; comb.type = "combiner"; + comb.project_type = "Combiner"; comb.display_name = "Combiner"; + comb.menu_label = "Add Combiner"; + comb.label_prefix = "Combiner"; + comb.kind = NodeKind::Combiner; + comb.authorable = true; comb.fields = { {"manual_mode", "Manual Mode", "", FieldKind::Bool, false, 0, 0, {}, false, ""}, }; + comb.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; comb.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -216,7 +284,12 @@ ComponentTypeRegistry::ComponentTypeRegistry() { ComponentTypeDescriptor adc; adc.type = "adc"; + adc.project_type = "ADC"; adc.display_name = "ADC"; + adc.menu_label = "Add RF ADC"; + adc.label_prefix = "ADC"; + adc.kind = NodeKind::Adc; + adc.authorable = true; adc.fields = { {"fs_Hz", "Sample Rate", "Hz", FieldKind::Number, true, 0.0, 1e12, {}, {}, ""}, {"nsd_dBm_per_Hz", @@ -230,6 +303,9 @@ ComponentTypeRegistry::ComponentTypeRegistry() { {}, ""}, }; + adc.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; adc.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, const nlohmann::json ¶meters) -> IComponentEngine * { auto &e = registry.add(id, graph); @@ -240,4 +316,40 @@ ComponentTypeRegistry::ComponentTypeRegistry() { return &e; }; m_descriptors.push_back(adc); + + ComponentTypeDescriptor gen; + gen.type = "generator"; + gen.project_type = "SignalGenerator"; + gen.display_name = "Generator"; + gen.menu_label = "Add Generator"; + gen.label_prefix = "Generator"; + gen.kind = NodeKind::Generator; + gen.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + m_descriptors.push_back(gen); + + ComponentTypeDescriptor coax; + coax.type = "coax"; + coax.project_type = "CoaxCable"; + coax.display_name = "Coax Cable"; + coax.menu_label = "Add Coax Cable"; + coax.label_prefix = "Coax Cable"; + coax.kind = NodeKind::CoaxCable; + coax.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + m_descriptors.push_back(coax); + + ComponentTypeDescriptor pfb; + pfb.type = "pfb"; + pfb.project_type = "PFBChannelizer"; + pfb.display_name = "PFB"; + pfb.menu_label = "Add PFB Channelizer"; + pfb.label_prefix = "PFB"; + pfb.kind = NodeKind::PFB; + pfb.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { + return static_cast(®istry.add(id, graph)); + }; + m_descriptors.push_back(pfb); } diff --git a/tests/test_component_authoring.cpp b/tests/test_component_authoring.cpp index 43bf81e..c59ca7d 100644 --- a/tests/test_component_authoring.cpp +++ b/tests/test_component_authoring.cpp @@ -22,14 +22,15 @@ // --- Task 1: ComponentTypeRegistry --- -TEST_CASE("ComponentTypeRegistry covers all 8 existing types", "[type_registry]") { +TEST_CASE("ComponentTypeRegistry covers all 11 existing types", "[type_registry]") { auto all = ComponentTypeRegistry::instance().all(); std::vector types; for (auto *d : all) types.push_back(d->type); std::sort(types.begin(), types.end()); - std::vector expected = {"adc", "amplifier", "attenuator", "combiner", - "equalizer", "filter", "mixer", "splitter"}; + std::vector expected = {"adc", "amplifier", "attenuator", "coax", + "combiner", "equalizer", "filter", "generator", + "mixer", "pfb", "splitter"}; REQUIRE(types == expected); } From 0c7e44e85a5e90a585df5d39055d6788a96f6d0e Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 17:55:49 +0200 Subject: [PATCH 05/15] refactor: instantiate via create()+deserialize(), drop factory field --- adc/src/adc_engine.cpp | 3 +- amplifier/src/amplifier_engine.cpp | 11 ++- app/include/component_type_registry.h | 8 +-- app/src/component_library.cpp | 3 +- app/src/component_type_registry.cpp | 92 ------------------------ attenuator/src/attenuator_engine.cpp | 3 +- ideal_filter/src/ideal_filter_engine.cpp | 36 +++++++--- mixer/src/mixer_engine.cpp | 3 +- 8 files changed, 46 insertions(+), 113 deletions(-) diff --git a/adc/src/adc_engine.cpp b/adc/src/adc_engine.cpp index 475bf62..53fa2f7 100644 --- a/adc/src/adc_engine.cpp +++ b/adc/src/adc_engine.cpp @@ -120,7 +120,8 @@ nlohmann::json AdcEngine::serialize() const { } void AdcEngine::deserialize(const nlohmann::json &j) { - m_fs_Hz = j.value("sample_rate_Hz", 1e9); + m_fs_Hz = + j.contains("sample_rate_Hz") ? j["sample_rate_Hz"].get() : j.value("fs_Hz", 1e9); m_nsd_dBm_per_Hz = j.value("nsd_dBm_per_Hz", -155.0); m_dirty = true; } diff --git a/amplifier/src/amplifier_engine.cpp b/amplifier/src/amplifier_engine.cpp index 43923ac..7ce58b9 100644 --- a/amplifier/src/amplifier_engine.cpp +++ b/amplifier/src/amplifier_engine.cpp @@ -216,9 +216,16 @@ void AmplifierEngine::deserialize(const nlohmann::json &j) { m_gain_dB = j.value("gain_dB", 0.0); m_nf_dB = j.value("nf_dB", 0.0); m_nonlinear.setEnabled(j.value("enable_nonlinear", false)); - m_nonlinear.setOIP2_dBm(j.value("oip2_dBm", 50.0)); - m_nonlinear.setOIP3_dBm(j.value("oip3_dBm", 50.0)); + m_nonlinear.setOIP2_dBm(j.value("oip2_dBm", 100.0)); + m_nonlinear.setOIP3_dBm(j.value("oip3_dBm", 100.0)); m_nonlinear.setP1dB_dBm(j.value("p1db_dBm", 100.0)); + // Library definitions (schema v1/v2) omit `enable_nonlinear` but include + // OIP/P1dB params. The old registry factory enabled nonlinearity whenever + // any of those were present; project files always serialize the explicit + // key, so only fall back when it is absent. + if (!j.contains("enable_nonlinear") && + (j.contains("oip2_dBm") || j.contains("oip3_dBm") || j.contains("p1db_dBm"))) + m_nonlinear.setEnabled(true); m_sparam_mode = j.value("sparam_mode", false); m_sparam_filepath = j.value("sparam_filepath", ""); m_sparam_fwd_idx = j.value("sparam_fwd_idx", 0); diff --git a/app/include/component_type_registry.h b/app/include/component_type_registry.h index 945fb49..c6c7655 100644 --- a/app/include/component_type_registry.h +++ b/app/include/component_type_registry.h @@ -40,17 +40,11 @@ struct ComponentTypeDescriptor { std::vector fields; // Create a default engine of this type (no params). Callers apply params - // via engine->deserialize(). Replaces the old params-taking `factory`. + // via engine->deserialize(). std::function create; // Inspector property draw. Receives the panel so PFB's multi-instance // selector and dirty-flag state stay reachable. std::function draw_inspector; - - // Legacy params-taking factory; still used by ComponentLibrary until - // Task 2b migrates instantiate to create()+deserialize(). - std::function - factory; }; class ComponentTypeRegistry { diff --git a/app/src/component_library.cpp b/app/src/component_library.cpp index eaa26c9..9675b35 100644 --- a/app/src/component_library.cpp +++ b/app/src/component_library.cpp @@ -161,9 +161,10 @@ IComponentEngine *ComponentLibrary::instantiate(const ComponentDefinition &def, return nullptr; } - IComponentEngine *result = descriptor->factory(registry, graph, id, def.parameters); + IComponentEngine *result = descriptor->create(registry, graph, id); if (!result) return nullptr; + result->deserialize(def.parameters); if (def.type == "amplifier") { auto *amp = dynamic_cast(result); diff --git a/app/src/component_type_registry.cpp b/app/src/component_type_registry.cpp index 32cd26b..5da77c5 100644 --- a/app/src/component_type_registry.cpp +++ b/app/src/component_type_registry.cpp @@ -62,26 +62,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { amp.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - // Legacy factory: applied library params directly. Removed in Task 2b. - amp.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("gain_dB")) - e.setGain_dB(parameters["gain_dB"].get()); - if (parameters.contains("nf_dB")) - e.setNF_dB(parameters["nf_dB"].get()); - if (parameters.contains("oip2_dBm")) - e.setOIP2_dBm(parameters["oip2_dBm"].get()); - if (parameters.contains("oip3_dBm")) - e.setOIP3_dBm(parameters["oip3_dBm"].get()); - if (parameters.contains("p1db_dBm")) - e.setP1dB_dBm(parameters["p1db_dBm"].get()); - bool has_nonlinear = parameters.contains("oip2_dBm") || parameters.contains("oip3_dBm") || - parameters.contains("p1db_dBm"); - if (has_nonlinear) - e.setEnableNonlinear(true); - return &e; - }; m_descriptors.push_back(amp); ComponentTypeDescriptor att; @@ -98,13 +78,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { att.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - att.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("attenuation_dB")) - e.setAttenuation(parameters["attenuation_dB"].get()); - return &e; - }; m_descriptors.push_back(att); ComponentTypeDescriptor spl; @@ -118,11 +91,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { spl.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - spl.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json &) -> IComponentEngine * { - auto &e = registry.add(id, graph); - return &e; - }; m_descriptors.push_back(spl); ComponentTypeDescriptor flt; @@ -150,28 +118,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { flt.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - flt.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("filter_type")) { - std::string ft = parameters["filter_type"].get(); - if (ft == "LPF") - e.setFilterType(FilterType::LPF); - else if (ft == "HPF") - e.setFilterType(FilterType::HPF); - else if (ft == "BPF") - e.setFilterType(FilterType::BPF); - else if (ft == "BSF") - e.setFilterType(FilterType::BSF); - } - double fc_low = parameters.value("fc_low_Hz", 100e6); - double fc_high = parameters.value("fc_high_Hz", 200e6); - if (parameters.contains("fc_low_Hz") && parameters.contains("fc_high_Hz")) - e.setCutoffs_Hz(fc_low, fc_high); - else if (parameters.contains("fc_low_Hz")) - e.setCutoff_Hz(fc_low); - return &e; - }; m_descriptors.push_back(flt); ComponentTypeDescriptor mix; @@ -199,17 +145,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { mix.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - mix.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("lo_freq_Hz")) - e.setLoFreq_Hz(parameters["lo_freq_Hz"].get()); - if (parameters.contains("conversion_gain_dB")) - e.setConversionGain_dB(parameters["conversion_gain_dB"].get()); - if (parameters.contains("nf_dB")) - e.setNF_dB(parameters["nf_dB"].get()); - return &e; - }; m_descriptors.push_back(mix); ComponentTypeDescriptor eq; @@ -246,17 +181,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { eq.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - eq.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("ref_gain_dB")) - e.setRefGain_dB(parameters["ref_gain_dB"].get()); - if (parameters.contains("ref_freq_Hz")) - e.setRefFreq_Hz(parameters["ref_freq_Hz"].get()); - if (parameters.contains("slope_dB_per_decade")) - e.setSlope_dBPerDecade(parameters["slope_dB_per_decade"].get()); - return &e; - }; m_descriptors.push_back(eq); ComponentTypeDescriptor comb; @@ -273,13 +197,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { comb.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - comb.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("manual_mode")) - e.setManualMode(parameters["manual_mode"].get()); - return &e; - }; m_descriptors.push_back(comb); ComponentTypeDescriptor adc; @@ -306,15 +223,6 @@ ComponentTypeRegistry::ComponentTypeRegistry() { adc.create = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id) { return static_cast(®istry.add(id, graph)); }; - adc.factory = [](ComponentRegistry ®istry, NodeGraphEngine &graph, int id, - const nlohmann::json ¶meters) -> IComponentEngine * { - auto &e = registry.add(id, graph); - if (parameters.contains("fs_Hz")) - e.setFs_Hz(parameters["fs_Hz"].get()); - if (parameters.contains("nsd_dBm_per_Hz")) - e.setNsd_dBm_per_Hz(parameters["nsd_dBm_per_Hz"].get()); - return &e; - }; m_descriptors.push_back(adc); ComponentTypeDescriptor gen; diff --git a/attenuator/src/attenuator_engine.cpp b/attenuator/src/attenuator_engine.cpp index 1bfdd8d..7e4da45 100644 --- a/attenuator/src/attenuator_engine.cpp +++ b/attenuator/src/attenuator_engine.cpp @@ -181,7 +181,8 @@ nlohmann::json AttenuatorEngine::serialize() const { } void AttenuatorEngine::deserialize(const nlohmann::json &j) { - m_atten_dB = j.value("atten_dB", 0.0); + m_atten_dB = + j.contains("atten_dB") ? j["atten_dB"].get() : j.value("attenuation_dB", 0.0); m_sparam_mode = j.value("sparam_mode", false); m_sparam_path = j.value("sparam_path", ""); m_dirty = true; diff --git a/ideal_filter/src/ideal_filter_engine.cpp b/ideal_filter/src/ideal_filter_engine.cpp index 39634f4..21133bf 100644 --- a/ideal_filter/src/ideal_filter_engine.cpp +++ b/ideal_filter/src/ideal_filter_engine.cpp @@ -172,14 +172,34 @@ nlohmann::json IdealFilterEngine::serialize() const { } void IdealFilterEngine::deserialize(const nlohmann::json &j) { - int ft = j.value("filter_type", 0); - if (ft < 0) - ft = 0; - if (ft > 3) - ft = 3; - m_type = static_cast(ft); - m_fc_low_Hz = j.value("fc_low_Hz", 100e6); - m_fc_high_Hz = j.value("fc_high_Hz", 200e6); + if (j.contains("filter_type")) { + if (j["filter_type"].is_string()) { + const std::string ft = j["filter_type"].get(); + if (ft == "LPF") + m_type = FilterType::LPF; + else if (ft == "HPF") + m_type = FilterType::HPF; + else if (ft == "BPF") + m_type = FilterType::BPF; + else if (ft == "BSF") + m_type = FilterType::BSF; + } else { + int ft = j.value("filter_type", 0); + if (ft < 0) + ft = 0; + if (ft > 3) + ft = 3; + m_type = static_cast(ft); + } + } + // Preserve the old registry factory's conditional cutoff semantics: + // both present -> setCutoffs; only fc_low -> setCutoff (mirrors high); + // neither -> keep constructor defaults. + if (j.contains("fc_low_Hz") && j.contains("fc_high_Hz")) { + setCutoffs_Hz(j["fc_low_Hz"].get(), j["fc_high_Hz"].get()); + } else if (j.contains("fc_low_Hz")) { + setCutoff_Hz(j["fc_low_Hz"].get()); + } m_sparam_mode = j.value("sparam_mode", false); m_sparam_filepath = j.value("sparam_filepath", ""); m_sparam_fwd_idx = j.value("sparam_fwd_idx", 0); diff --git a/mixer/src/mixer_engine.cpp b/mixer/src/mixer_engine.cpp index 9def1b6..58c0c3a 100644 --- a/mixer/src/mixer_engine.cpp +++ b/mixer/src/mixer_engine.cpp @@ -98,7 +98,8 @@ nlohmann::json MixerEngine::serialize() const { void MixerEngine::deserialize(const nlohmann::json &j) { m_lo_freq_Hz = j.value("lo_freq_Hz", 1e9); - m_conv_gain_dB = j.value("conv_gain_dB", -6.0); + m_conv_gain_dB = j.contains("conv_gain_dB") ? j["conv_gain_dB"].get() + : j.value("conversion_gain_dB", -6.0); m_nf_dB = j.value("nf_dB", 0.0); m_dirty = true; } From fc67e42e00e6f3cadc6062593884ea89a7fa195b Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:04:34 +0200 Subject: [PATCH 06/15] refactor: save/load/duplicate dispatch through ComponentTypeRegistry --- app/src/app.cpp | 140 +++++++++++------------------------------------- 1 file changed, 31 insertions(+), 109 deletions(-) diff --git a/app/src/app.cpp b/app/src/app.cpp index 95c6107..e49b462 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -14,7 +14,6 @@ #include #include #include -#include #include // Keep only filesystem-safe characters for path segments: [A-Za-z0-9-_ ]. // Strips everything else (incl. /, \\, and . which eliminates .. risks). @@ -206,50 +205,29 @@ void RfSimulatorApp::duplicateComponent(int graph_node_id) { } } - // Helper: create a new engine of type T, copy params via serialize/deserialize, - // position it offset from the source, and return a reference to the new engine. - auto dup = [&](auto *typed_src) -> decltype(typed_src) { - using T = std::remove_pointer_t; - auto &new_eng = m_components.add(m_next_component_id++, m_graph_engine); - new_eng.deserialize(typed_src->serialize()); - int new_nid = new_eng.graphNodeId(); - // Register with imnodes pool and set position - ImNodes::SetNodeEditorSpacePos(new_nid, ImVec2(src_pos.x + OFFSET, src_pos.y + OFFSET)); - // Copy library part number - if (!src_part_number.empty()) - m_graph_engine.setNodePartNumber(new_nid, src_part_number); - return &new_eng; - }; - - if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - auto *new_pfb = dup(e); - // PFB also needs IQ plot widget and grid widget (same as onAddPFB) + // Clone via the registry: create a default engine, then copy params through + // serialize/deserialize. Removes the 11-way dynamic_cast chain. + const auto *desc = ComponentTypeRegistry::instance().find(src->type_name()); + if (!desc) + return; + IComponentEngine *copy = desc->create(m_components, m_graph_engine, m_next_component_id++); + copy->deserialize(src->serialize()); + int new_nid = copy->graphNodeId(); + // Register with imnodes pool and set position + ImNodes::EditorContextSet(m_graph_widget->context()); + ImNodes::SetNodeEditorSpacePos(new_nid, ImVec2(src_pos.x + OFFSET, src_pos.y + OFFSET)); + // Copy library part number + if (!src_part_number.empty()) + m_graph_engine.setNodePartNumber(new_nid, src_part_number); + // PFB also needs IQ plot widget and grid widget (same as onAddPFB) + if (desc->type == "pfb") { + auto *new_pfb = static_cast(copy); m_iq_widgets.push_back(std::make_unique(*new_pfb)); m_show_iq_pfbs.push_back(m_state.loadBool( "WindowState", ("IQPlot_" + std::to_string(new_pfb->id())).c_str(), true)); m_pfb_grid_widgets.push_back(std::make_unique(*new_pfb)); m_show_pfb_grids.push_back(m_state.loadBool( "WindowState", ("PFBGrid_" + std::to_string(new_pfb->id())).c_str(), true)); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); - } else if (auto *e = dynamic_cast(src)) { - dup(e); } markDirty(); @@ -418,21 +396,6 @@ void RfSimulatorApp::saveProject(const std::string &path) { auto dot = fname.find_last_of('.'); root["name"] = (dot != std::string::npos) ? fname.substr(0, dot) : fname; - // Type to name mapping for readable type names - static const std::unordered_map s_type_names = { - {std::type_index(typeid(SignalGeneratorEngine)), "SignalGenerator"}, - {std::type_index(typeid(AmplifierEngine)), "Amplifier"}, - {std::type_index(typeid(SplitterEngine)), "Splitter"}, - {std::type_index(typeid(MixerEngine)), "Mixer"}, - {std::type_index(typeid(AttenuatorEngine)), "Attenuator"}, - {std::type_index(typeid(CombinerEngine)), "Combiner"}, - {std::type_index(typeid(EqualizerEngine)), "Equalizer"}, - {std::type_index(typeid(AdcEngine)), "ADC"}, - {std::type_index(typeid(PFBChannelizerEngine)), "PFBChannelizer"}, - {std::type_index(typeid(CoaxCableEngine)), "CoaxCable"}, - {std::type_index(typeid(IdealFilterEngine)), "IdealFilter"}, - }; - // Ensure all engine nodes are registered with the imnodes context // so GetNodeEditorSpacePos() doesn't assert on node IDs added without // a prior render frame (e.g. via newProject then programmatic add). @@ -442,8 +405,8 @@ void RfSimulatorApp::saveProject(const std::string &path) { nlohmann::json comps_arr = nlohmann::json::array(); for (auto *comp : m_components.all()) { nlohmann::json cj; - auto it = s_type_names.find(std::type_index(typeid(*comp))); - cj["type"] = (it != s_type_names.end()) ? it->second : "Unknown"; + const auto *desc = ComponentTypeRegistry::instance().find(comp->type_name()); + cj["type"] = desc ? desc->project_type : "Unknown"; cj["params"] = comp->serialize(); // Save node position via imnodes @@ -587,63 +550,22 @@ void RfSimulatorApp::loadProject(const std::string &path) { std::string type = cj.value("type", ""); auto ¶ms = cj["params"]; - IComponentEngine *comp = nullptr; - if (type == "SignalGenerator") { - auto &ref = - m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "Amplifier") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "Mixer") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "Splitter") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "Attenuator") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "Combiner") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "Equalizer") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "ADC") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "PFBChannelizer") { - auto &ref = - m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - // Restore IQ plot + PFB grid widgets for this PFB - m_iq_widgets.push_back(std::make_unique(ref)); - m_show_iq_pfbs.push_back(true); - m_pfb_grid_widgets.push_back(std::make_unique(ref)); - m_show_pfb_grids.push_back(true); - comp = &ref; - } else if (type == "CoaxCable") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else if (type == "IdealFilter") { - auto &ref = m_components.add(m_next_component_id++, m_graph_engine); - ref.deserialize(params); - comp = &ref; - } else { + const auto *desc = ComponentTypeRegistry::instance().findByProjectType(type); + if (!desc) { LOG_WARN("Unknown component type in project file: %s", type.c_str()); new_node_ids.push_back(-1); continue; } + IComponentEngine *comp = desc->create(m_components, m_graph_engine, m_next_component_id++); + comp->deserialize(params); + if (desc->type == "pfb") { + auto *pfb = static_cast(comp); + // Restore IQ plot + PFB grid widgets for this PFB + m_iq_widgets.push_back(std::make_unique(*pfb)); + m_show_iq_pfbs.push_back(true); + m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); + m_show_pfb_grids.push_back(true); + } new_node_ids.push_back(comp ? comp->graphNodeId() : -1); From afe1572edca6156a0f05ffb02d0688fb5015756c Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:13:44 +0200 Subject: [PATCH 07/15] feat: data-driven canvas menu with unified addComponent path --- app/include/app.h | 1 + app/src/app.cpp | 96 ++++++-------------------- node_graph/include/node_graph_widget.h | 23 +++--- node_graph/src/node_graph_widget.cpp | 48 ++----------- tests/CMakeLists.txt | 8 +++ tests/test_component_dispatch.cpp | 41 +++++++++++ 6 files changed, 90 insertions(+), 127 deletions(-) create mode 100644 tests/test_component_dispatch.cpp diff --git a/app/include/app.h b/app/include/app.h index 05b16b5..47a9c4d 100644 --- a/app/include/app.h +++ b/app/include/app.h @@ -93,6 +93,7 @@ class RfSimulatorApp { void load_window_states(); void rewireInputs(); void duplicateComponent(int graph_node_id); + void addComponent(const ComponentTypeDescriptor *desc, ImVec2 pos); void openNewComponentForm(const std::string &type); void openEditComponentForm(const ComponentDefinition &def); void drawComponentFormModal(); diff --git a/app/src/app.cpp b/app/src/app.cpp index e49b462..1c164f6 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -35,78 +35,12 @@ static std::string sanitizePathSegment(const std::string &s, const std::string & RfSimulatorApp::RfSimulatorApp() : m_components(m_graph_engine, m_view_manager) { m_graph_widget = std::make_unique(m_graph_engine); - m_graph_widget->onAddGenerator = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddAmplifier = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddSplitter = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddMixer = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - - m_graph_widget->onAddAdc = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddPFB = [this](ImVec2 pos) { - auto &pfb = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(pfb.graphNodeId(), pos); - m_iq_widgets.push_back(std::make_unique(pfb)); - m_show_iq_pfbs.push_back( - m_state.loadBool("WindowState", ("IQPlot_" + std::to_string(pfb.id())).c_str(), true)); - m_pfb_grid_widgets.push_back(std::make_unique(pfb)); - m_show_pfb_grids.push_back( - m_state.loadBool("WindowState", ("PFBGrid_" + std::to_string(pfb.id())).c_str(), true)); - markDirty(); - }; - m_graph_widget->onAddCoaxCable = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddEqualizer = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - }; - m_graph_widget->onAddIdealFilter = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddAttenuator = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; - m_graph_widget->onAddCombiner = [this](ImVec2 pos) { - auto &comp = m_components.add(m_next_component_id++, m_graph_engine); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp.graphNodeId(), pos); - markDirty(); - }; + std::vector addable; + for (const auto *desc : ComponentTypeRegistry::instance().all()) { + addable.push_back( + {desc->menu_label, [this, desc](ImVec2 pos) { addComponent(desc, pos); }}); + } + m_graph_widget->setAddableComponents(std::move(addable)); m_graph_widget->onNodeMoved = [this]() { markDirty(); }; m_graph_widget->onLinkChanged = [this]() { markDirty(); }; m_graph_widget->onRemoveNode = [this](int id) { @@ -178,6 +112,22 @@ RfSimulatorApp::RfSimulatorApp() : m_components(m_graph_engine, m_view_manager) load_window_states(); } +void RfSimulatorApp::addComponent(const ComponentTypeDescriptor *desc, ImVec2 pos) { + IComponentEngine *comp = desc->create(m_components, m_graph_engine, m_next_component_id++); + ImNodes::EditorContextSet(m_graph_widget->context()); + ImNodes::SetNodeEditorSpacePos(comp->graphNodeId(), pos); + if (desc->type == "pfb") { + auto *pfb = static_cast(comp); + m_iq_widgets.push_back(std::make_unique(*pfb)); + m_show_iq_pfbs.push_back( + m_state.loadBool("WindowState", ("IQPlot_" + std::to_string(pfb->id())).c_str(), true)); + m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); + m_show_pfb_grids.push_back(m_state.loadBool( + "WindowState", ("PFBGrid_" + std::to_string(pfb->id())).c_str(), true)); + } + markDirty(); // unconditional — fixes the Equalizer missing-markDirty bug +} + void RfSimulatorApp::load_window_states() { m_show_log = m_state.loadBool("WindowState", "Log", true); m_show_spectrum = m_state.loadBool("WindowState", "SpectrumAnalyzer", true); @@ -219,7 +169,7 @@ void RfSimulatorApp::duplicateComponent(int graph_node_id) { // Copy library part number if (!src_part_number.empty()) m_graph_engine.setNodePartNumber(new_nid, src_part_number); - // PFB also needs IQ plot widget and grid widget (same as onAddPFB) + // PFB also needs IQ plot widget and grid widget (same as addComponent) if (desc->type == "pfb") { auto *new_pfb = static_cast(copy); m_iq_widgets.push_back(std::make_unique(*new_pfb)); diff --git a/node_graph/include/node_graph_widget.h b/node_graph/include/node_graph_widget.h index fd8125c..58da1ed 100644 --- a/node_graph/include/node_graph_widget.h +++ b/node_graph/include/node_graph_widget.h @@ -7,6 +7,7 @@ #include #include #include +#include struct ImVec2; @@ -23,22 +24,21 @@ class NodeGraphWidget { // Callbacks for app to create/destroy components std::function onNodeMoved; - std::function onAddGenerator; - std::function onAddAmplifier; - std::function onAddSplitter; - std::function onAddMixer; - std::function onAddAdc; - std::function onAddPFB; - std::function onAddIdealFilter; - std::function onAddCoaxCable; - std::function onAddEqualizer; - std::function onAddAttenuator; - std::function onAddCombiner; std::function onRemoveNode; std::function onDuplicateNode; std::function onLinkChanged; std::function onNodeHover; + // Data-driven canvas menu: app populates from ComponentTypeRegistry. + struct AddableComponent { + std::string menu_label; + std::function on_add; + }; + void setAddableComponents(std::vector addable) { + m_addable_components = std::move(addable); + } + const std::vector &addableComponents() const { return m_addable_components; } + ImNodesEditorContext *context() { return m_context; } void syncNodesFromEngine(); void clearPositionCache() { @@ -99,6 +99,7 @@ class NodeGraphWidget { // Set of node IDs registered in the ImNodes pool, so syncNodesFromEngine // can register new nodes without resetting existing positions. std::unordered_set m_registered_in_pool; + std::vector m_addable_components; bool m_show_create_popup = false; // Internal rendering helpers diff --git a/node_graph/src/node_graph_widget.cpp b/node_graph/src/node_graph_widget.cpp index 1a89e9b..5b16c7a 100644 --- a/node_graph/src/node_graph_widget.cpp +++ b/node_graph/src/node_graph_widget.cpp @@ -352,49 +352,11 @@ void NodeGraphWidget::handleContextMenu(bool editor_hovered) { } if (ImGui::BeginPopup("canvas_context_menu")) { - if (ImGui::MenuItem("Add Generator")) { - if (onAddGenerator) - onAddGenerator(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Amplifier")) { - if (onAddAmplifier) - onAddAmplifier(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Splitter")) { - if (onAddSplitter) - onAddSplitter(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Combiner")) { - if (onAddCombiner) - onAddCombiner(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Coax Cable")) { - if (onAddCoaxCable) - onAddCoaxCable(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Equalizer")) { - if (onAddEqualizer) - onAddEqualizer(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Mixer")) { - if (onAddMixer) - onAddMixer(m_context_menu_pos); - } - if (ImGui::MenuItem("Add RF ADC")) { - if (onAddAdc) - onAddAdc(m_context_menu_pos); - } - if (ImGui::MenuItem("Add PFB Channelizer")) { - if (onAddPFB) - onAddPFB(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Ideal Filter")) { - if (onAddIdealFilter) - onAddIdealFilter(m_context_menu_pos); - } - if (ImGui::MenuItem("Add Attenuator")) { - if (onAddAttenuator) - onAddAttenuator(m_context_menu_pos); + for (const auto &addable : m_addable_components) { + if (ImGui::MenuItem(addable.menu_label.c_str())) { + if (addable.on_add) + addable.on_add(m_context_menu_pos); + } } ImGui::EndPopup(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e0d65e3..adfcc9d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -104,6 +104,14 @@ target_link_libraries(test_issue37_pfb_input_removal PRIVATE ) target_compile_definitions(test_issue37_pfb_input_removal PRIVATE PROJECT_SOURCE_DIR="${CMAKE_SOURCE_DIR}") add_test(NAME test_issue37_pfb_input_removal COMMAND test_issue37_pfb_input_removal WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) + +add_executable(test_component_dispatch test_component_dispatch.cpp) +target_link_libraries(test_component_dispatch PRIVATE + simulator::app + Catch2::Catch2WithMain +) +target_compile_definitions(test_component_dispatch PRIVATE PROJECT_SOURCE_DIR="${CMAKE_SOURCE_DIR}") +add_test(NAME test_component_dispatch COMMAND test_component_dispatch WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) add_executable(test_signal_domain test_signal_domain.cpp) target_link_libraries(test_signal_domain PRIVATE common diff --git a/tests/test_component_dispatch.cpp b/tests/test_component_dispatch.cpp new file mode 100644 index 0000000..03c4dfe --- /dev/null +++ b/tests/test_component_dispatch.cpp @@ -0,0 +1,41 @@ +// Regression test for issue #51: adding an Equalizer from the canvas context +// menu must mark the project dirty. +// +// Root cause: the old onAddEqualizer lambda in RfSimulatorApp's constructor +// (app.cpp) forgot to call markDirty(), unlike every other onAdd* lambda. The +// unified RfSimulatorApp::addComponent() path (data-driven canvas menu built +// from ComponentTypeRegistry::all()) calls markDirty() unconditionally, which +// fixes the bug. +#include "app.h" +#include "imgui.h" +#include "imnodes.h" +#include "implot.h" +#include + +struct ImGuiFixture { + ImGuiFixture() { + ImGui::CreateContext(); + ImPlot::CreateContext(); + ImNodes::CreateContext(); + } + ~ImGuiFixture() { + ImNodes::DestroyContext(); + ImPlot::DestroyContext(); + ImGui::DestroyContext(); + } +}; + +TEST_CASE_METHOD(ImGuiFixture, "Adding an Equalizer marks the project dirty (issue #51)", + "[dispatch][regression]") { + RfSimulatorApp app; + REQUIRE(app.isDirty() == false); + bool clicked = false; + for (const auto &addable : app.testGraphWidget().addableComponents()) { + if (addable.menu_label == "Add Equalizer") { + addable.on_add(ImVec2(0, 0)); + clicked = true; + } + } + REQUIRE(clicked); + REQUIRE(app.isDirty() == true); +} From 710ccdd8414c782e939f75af253cb5fece0dc8ea Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:20:11 +0200 Subject: [PATCH 08/15] refactor: inspector dispatch through ComponentTypeRegistry --- app/include/inspector_panel.h | 47 +++++------ app/src/app.cpp | 1 + app/src/inspector_panel.cpp | 153 ++++++++++++++-------------------- 3 files changed, 82 insertions(+), 119 deletions(-) diff --git a/app/include/inspector_panel.h b/app/include/inspector_panel.h index 4246f06..e122c52 100644 --- a/app/include/inspector_panel.h +++ b/app/include/inspector_panel.h @@ -1,6 +1,7 @@ #pragma once #include "component_interface.h" +#include "component_type_registry.h" #include "node_graph_engine.h" #include "signal_node.h" #include @@ -52,6 +53,23 @@ class InspectorPanel { void setViewToggles(const ViewToggles &t) { m_viewToggles = t; } bool m_param_edited = false; + // Called once at startup; wires ComponentTypeRegistry draw_inspector + // callbacks to this panel's property drawers. + void registerDrawers(ComponentTypeRegistry ®istry); + + void drawAmplifierProperties(AmplifierEngine &engine, int index); + void drawCoaxCableProperties(CoaxCableEngine &engine, int index); + void drawEqualizerProperties(EqualizerEngine &engine, int index); + void drawMixerProperties(MixerEngine &engine, int index); + void drawSplitterProperties(SplitterEngine &engine, int index); + void drawAdcProperties(AdcEngine &engine, int index); + void drawGeneratorProperties(SignalGeneratorEngine &engine, int index); + void drawPFBProperties(PFBChannelizerEngine &engine); + void drawIdealFilterProperties(IdealFilterEngine &engine, int index); + void drawAttenuatorProperties(AttenuatorEngine &engine, int index); + void drawCombinerProperties(CombinerEngine &engine, int index); + void drawGroupPanel(int group_id); + private: NodeGraphEngine &m_graph; ComponentRegistry *m_components = nullptr; @@ -61,37 +79,10 @@ class InspectorPanel { std::vector *m_pfb_iq_visible = nullptr; std::vector *m_pfb_grid_visible = nullptr; - enum class ComponentType { - None, - Generator, - Amplifier, - Splitter, - Mixer, - Adc, - PFB, - IdealFilter, - CoaxCable, - Equalizer, - Attenuator, - Combiner - }; struct Hit { - ComponentType type; + const ComponentTypeDescriptor *desc = nullptr; IComponentEngine *engine = nullptr; }; Hit findSelected() const; std::string labelForHit(const Hit &hit) const; - - void drawAmplifierProperties(AmplifierEngine &engine, int index); - void drawCoaxCableProperties(CoaxCableEngine &engine, int index); - void drawEqualizerProperties(EqualizerEngine &engine, int index); - void drawMixerProperties(MixerEngine &engine, int index); - void drawSplitterProperties(SplitterEngine &engine, int index); - void drawAdcProperties(AdcEngine &engine, int index); - void drawGeneratorProperties(SignalGeneratorEngine &engine, int index); - void drawPFBProperties(PFBChannelizerEngine &engine); - void drawIdealFilterProperties(IdealFilterEngine &engine, int index); - void drawAttenuatorProperties(AttenuatorEngine &engine, int index); - void drawCombinerProperties(CombinerEngine &engine, int index); - void drawGroupPanel(int group_id); }; diff --git a/app/src/app.cpp b/app/src/app.cpp index 1c164f6..96ecf1f 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -77,6 +77,7 @@ RfSimulatorApp::RfSimulatorApp() : m_components(m_graph_engine, m_view_manager) m_components.add(m_next_component_id++, m_graph_engine); m_inspector_panel = std::make_unique(m_graph_engine, m_components); + m_inspector_panel->registerDrawers(ComponentTypeRegistry::instance()); m_inspector_panel->onRemoveNode = [this](int graph_node_id) { if (m_graph_widget->onRemoveNode) m_graph_widget->onRemoveNode(graph_node_id); diff --git a/app/src/inspector_panel.cpp b/app/src/inspector_panel.cpp index 8422efa..fed2eaf 100644 --- a/app/src/inspector_panel.cpp +++ b/app/src/inspector_panel.cpp @@ -23,69 +23,75 @@ InspectorPanel::InspectorPanel(NodeGraphEngine &graph, ComponentRegistry &components) : m_graph(graph), m_components(&components) {} +void InspectorPanel::registerDrawers(ComponentTypeRegistry ®istry) { + for (auto *d : registry.all()) { + if (d->type == "generator") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawGeneratorProperties(static_cast(e), e.id()); + }; + } else if (d->type == "amplifier") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawAmplifierProperties(static_cast(e), e.id()); + }; + } else if (d->type == "splitter") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawSplitterProperties(static_cast(e), e.id()); + }; + } else if (d->type == "mixer") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawMixerProperties(static_cast(e), e.id()); + }; + } else if (d->type == "adc") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawAdcProperties(static_cast(e), e.id()); + }; + } else if (d->type == "pfb") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawPFBProperties(static_cast(e)); + }; + } else if (d->type == "filter") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawIdealFilterProperties(static_cast(e), e.id()); + }; + } else if (d->type == "coax") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawCoaxCableProperties(static_cast(e), e.id()); + }; + } else if (d->type == "equalizer") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawEqualizerProperties(static_cast(e), e.id()); + }; + } else if (d->type == "attenuator") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawAttenuatorProperties(static_cast(e), e.id()); + }; + } else if (d->type == "combiner") { + d->draw_inspector = [](InspectorPanel &p, IComponentEngine &e) { + p.drawCombinerProperties(static_cast(e), e.id()); + }; + } + } +} + InspectorPanel::Hit InspectorPanel::findSelected() const { int n = ImNodes::NumSelectedNodes(); if (n != 1) - return {ComponentType::None, nullptr}; + return {nullptr, nullptr}; int selected_id = -1; ImNodes::GetSelectedNodes(&selected_id); auto *engine = m_components->find(selected_id); if (!engine) - return {ComponentType::None, nullptr}; - - if (dynamic_cast(engine)) - return {ComponentType::Generator, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Amplifier, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Splitter, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Mixer, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Adc, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::PFB, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::IdealFilter, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::CoaxCable, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Equalizer, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Attenuator, engine}; - else if (dynamic_cast(engine)) - return {ComponentType::Combiner, engine}; - - return {ComponentType::None, nullptr}; + return {nullptr, nullptr}; + + return {ComponentTypeRegistry::instance().find(engine->type_name()), engine}; } std::string InspectorPanel::labelForHit(const Hit &hit) const { - if (hit.type == ComponentType::None || !hit.engine) - return ""; - if (hit.type == ComponentType::PFB) - return "PFB " + std::to_string(hit.engine->id()); - switch (hit.type) { - case ComponentType::Amplifier: - return "Amplifier " + std::to_string(hit.engine->id()); - case ComponentType::Mixer: - return "Mixer " + std::to_string(hit.engine->id()); - case ComponentType::Splitter: - return "Splitter " + std::to_string(hit.engine->id()); - case ComponentType::Adc: - return "ADC " + std::to_string(hit.engine->id()); - case ComponentType::Generator: - return "Generator " + std::to_string(hit.engine->id()); - case ComponentType::IdealFilter: - return "IdealFilter " + std::to_string(hit.engine->id()); - case ComponentType::Attenuator: - return "Attenuator " + std::to_string(hit.engine->id()); - case ComponentType::Combiner: - return "Combiner " + std::to_string(hit.engine->id()); - default: + if (!hit.desc || !hit.engine) return ""; - } + return hit.desc->display_name + " " + std::to_string(hit.engine->id()); } void InspectorPanel::draw(const char *title, bool *p_open) { @@ -102,7 +108,7 @@ void InspectorPanel::draw(const char *title, bool *p_open) { } auto hit = findSelected(); - if (hit.type == ComponentType::None || !hit.engine) { + if (!hit.desc || !hit.engine) { ImGui::TextDisabled("Select a component in the Node Editor"); ImGui::SeparatorText("View"); @@ -128,24 +134,8 @@ void InspectorPanel::draw(const char *title, bool *p_open) { ImGui::SeparatorText(labelForHit(hit).c_str()); - switch (hit.type) { - case ComponentType::Amplifier: - drawAmplifierProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Mixer: - drawMixerProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Splitter: - drawSplitterProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Adc: - drawAdcProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Generator: - drawGeneratorProperties(*static_cast(hit.engine), - hit.engine->id()); - break; - case ComponentType::PFB: { + if (hit.desc->type == "pfb") { + // PFB keeps its multi-instance selector combo (needs m_pfb_ptrs). auto *pfb = static_cast(hit.engine); for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { if (m_pfb_ptrs[i] == pfb) { @@ -173,31 +163,12 @@ void InspectorPanel::draw(const char *title, bool *p_open) { } ImGui::EndCombo(); } - if (m_selected_pfb_index < static_cast(m_pfb_ptrs.size()) && - m_pfb_ptrs[m_selected_pfb_index]) - drawPFBProperties(*m_pfb_ptrs[m_selected_pfb_index]); } - break; - } - case ComponentType::IdealFilter: - drawIdealFilterProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::CoaxCable: - drawCoaxCableProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Equalizer: - drawEqualizerProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Attenuator: - drawAttenuatorProperties(*static_cast(hit.engine), hit.engine->id()); - break; - case ComponentType::Combiner: - drawCombinerProperties(*static_cast(hit.engine), hit.engine->id()); - break; - default: - break; } + if (hit.desc->draw_inspector) + hit.desc->draw_inspector(*this, *hit.engine); + if (m_param_edited && onParamChange) onParamChange(); From 49499a564f8b090aaadcef1edd82d71c5ad0a656 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:29:40 +0200 Subject: [PATCH 09/15] refactor: data-driven NodeKind mapping in node graph widget --- app/src/app.cpp | 1 + node_graph/include/node_graph_engine.h | 29 -------------------------- node_graph/include/node_graph_widget.h | 8 +++++++ node_graph/src/node_graph_widget.cpp | 9 +++++++- tests/test_component_dispatch.cpp | 9 ++++++++ tests/test_node_graph_engine.cpp | 20 ------------------ 6 files changed, 26 insertions(+), 50 deletions(-) diff --git a/app/src/app.cpp b/app/src/app.cpp index 96ecf1f..adfaf64 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -39,6 +39,7 @@ RfSimulatorApp::RfSimulatorApp() : m_components(m_graph_engine, m_view_manager) for (const auto *desc : ComponentTypeRegistry::instance().all()) { addable.push_back( {desc->menu_label, [this, desc](ImVec2 pos) { addComponent(desc, pos); }}); + m_graph_widget->registerNodeKind(desc->label_prefix, desc->kind); } m_graph_widget->setAddableComponents(std::move(addable)); m_graph_widget->onNodeMoved = [this]() { markDirty(); }; diff --git a/node_graph/include/node_graph_engine.h b/node_graph/include/node_graph_engine.h index e22c569..1d3f9de 100644 --- a/node_graph/include/node_graph_engine.h +++ b/node_graph/include/node_graph_engine.h @@ -123,35 +123,6 @@ enum class NodeKind { GroupCollapsed }; -// Maps a node label to a NodeKind by prefix matching. Each engine -// constructor sets a unique, stable label prefix. First match wins. -// Unrecognised input (empty, group names, future engines) returns Unknown. -inline NodeKind nodeKindFromLabel(const std::string &label) { - if (label.rfind("Generator", 0) == 0) - return NodeKind::Generator; - if (label.rfind("Amplifier", 0) == 0) - return NodeKind::Amplifier; - if (label.rfind("Splitter", 0) == 0) - return NodeKind::Splitter; - if (label.rfind("Mixer", 0) == 0) - return NodeKind::Mixer; - if (label.rfind("ADC", 0) == 0) - return NodeKind::Adc; - if (label.rfind("PFB", 0) == 0) - return NodeKind::PFB; - if (label.rfind("IdealFilter", 0) == 0) - return NodeKind::IdealFilter; - if (label.rfind("Coax Cable", 0) == 0) - return NodeKind::CoaxCable; - if (label.rfind("Equalizer", 0) == 0) - return NodeKind::Equalizer; - if (label.rfind("Attenuator", 0) == 0) - return NodeKind::Attenuator; - if (label.rfind("Combiner", 0) == 0) - return NodeKind::Combiner; - return NodeKind::Unknown; -} - // Per-NodeKind ARGB color. Engine has no imgui include, so the return type // is plain uint32_t (same bit layout as IM_COL32: 0xAARRGGBB). The widget // casts to ImU32 at the call site. diff --git a/node_graph/include/node_graph_widget.h b/node_graph/include/node_graph_widget.h index 58da1ed..19af7df 100644 --- a/node_graph/include/node_graph_widget.h +++ b/node_graph/include/node_graph_widget.h @@ -39,6 +39,13 @@ class NodeGraphWidget { } const std::vector &addableComponents() const { return m_addable_components; } + // Data-driven label-prefix → NodeKind mapping. The app feeds this from + // ComponentTypeRegistry (label_prefix + kind) at startup. + void registerNodeKind(std::string label_prefix, NodeKind kind) { + m_kind_prefixes.push_back({std::move(label_prefix), kind}); + } + NodeKind kindForLabel(const std::string &label) const; + ImNodesEditorContext *context() { return m_context; } void syncNodesFromEngine(); void clearPositionCache() { @@ -100,6 +107,7 @@ class NodeGraphWidget { // can register new nodes without resetting existing positions. std::unordered_set m_registered_in_pool; std::vector m_addable_components; + std::vector> m_kind_prefixes; bool m_show_create_popup = false; // Internal rendering helpers diff --git a/node_graph/src/node_graph_widget.cpp b/node_graph/src/node_graph_widget.cpp index 5b16c7a..cbe93f9 100644 --- a/node_graph/src/node_graph_widget.cpp +++ b/node_graph/src/node_graph_widget.cpp @@ -133,6 +133,13 @@ void NodeGraphWidget::draw(const char *title, bool *p_open) { ImGui::End(); } +NodeKind NodeGraphWidget::kindForLabel(const std::string &label) const { + for (const auto &[prefix, kind] : m_kind_prefixes) + if (label.rfind(prefix, 0) == 0) + return kind; + return NodeKind::Unknown; +} + void NodeGraphWidget::drawNodes() { // Clear screen position cache - will be repopulated for nodes drawn this frame. // This ensures detectNodeMoves() only checks nodes that were actually drawn, @@ -167,7 +174,7 @@ void NodeGraphWidget::drawNodes() { m_grid_to_screen_offset = screen_pos - grid_pos; first_visible = false; } - const NodeKind kind = nodeKindFromLabel(node.label); + const NodeKind kind = kindForLabel(node.label); const ImU32 color = static_cast(themeColor(kind)); ImNodes::PushColorStyle(ImNodesCol_TitleBar, color); ImNodes::PushColorStyle(ImNodesCol_NodeOutline, color); diff --git a/tests/test_component_dispatch.cpp b/tests/test_component_dispatch.cpp index 03c4dfe..aebe97e 100644 --- a/tests/test_component_dispatch.cpp +++ b/tests/test_component_dispatch.cpp @@ -7,6 +7,7 @@ // from ComponentTypeRegistry::all()) calls markDirty() unconditionally, which // fixes the bug. #include "app.h" +#include "component_type_registry.h" #include "imgui.h" #include "imnodes.h" #include "implot.h" @@ -39,3 +40,11 @@ TEST_CASE_METHOD(ImGuiFixture, "Adding an Equalizer marks the project dirty (iss REQUIRE(clicked); REQUIRE(app.isDirty() == true); } + +TEST_CASE_METHOD(ImGuiFixture, "Every registry label_prefix maps to its kind", "[dispatch]") { + RfSimulatorApp app; + for (const auto *d : ComponentTypeRegistry::instance().all()) { + REQUIRE(app.testGraphWidget().kindForLabel(d->label_prefix + " 1") == d->kind); + } + REQUIRE(app.testGraphWidget().kindForLabel("UnknownThing 1") == NodeKind::Unknown); +} diff --git a/tests/test_node_graph_engine.cpp b/tests/test_node_graph_engine.cpp index ba52d08..a291020 100644 --- a/tests/test_node_graph_engine.cpp +++ b/tests/test_node_graph_engine.cpp @@ -181,26 +181,6 @@ TEST_CASE("NodeGraphEngine group counter accessors", "[node_graph]") { REQUIRE(engine.nextBoundaryPinId() == 1999); } -TEST_CASE("nodeKindFromLabel maps known prefixes", "[node_graph][appearance]") { - REQUIRE(nodeKindFromLabel("Generator 1") == NodeKind::Generator); - REQUIRE(nodeKindFromLabel("Amplifier 2") == NodeKind::Amplifier); - REQUIRE(nodeKindFromLabel("Splitter 3") == NodeKind::Splitter); - REQUIRE(nodeKindFromLabel("Mixer 4") == NodeKind::Mixer); - REQUIRE(nodeKindFromLabel("ADC 6") == NodeKind::Adc); - REQUIRE(nodeKindFromLabel("PFB 7") == NodeKind::PFB); - REQUIRE(nodeKindFromLabel("IdealFilter 8") == NodeKind::IdealFilter); - REQUIRE(nodeKindFromLabel("Coax Cable 9") == NodeKind::CoaxCable); - REQUIRE(nodeKindFromLabel("Equalizer 10") == NodeKind::Equalizer); -} - -TEST_CASE("nodeKindFromLabel returns Unknown for unrecognised input", "[node_graph][appearance]") { - REQUIRE(nodeKindFromLabel("") == NodeKind::Unknown); - REQUIRE(nodeKindFromLabel("Subcircuit 1") == NodeKind::Unknown); // groups handled separately - REQUIRE(nodeKindFromLabel("generator 1") == NodeKind::Unknown); // case-sensitive - REQUIRE(nodeKindFromLabel("Amplifier") == - NodeKind::Amplifier); // no trailing space still matches -} - TEST_CASE("themeColor returns a non-zero color for every NodeKind", "[node_graph][appearance]") { REQUIRE(themeColor(NodeKind::Unknown) != 0u); REQUIRE(themeColor(NodeKind::Generator) != 0u); From 6af0e3f9a836fa6aeda31b5ede893a69433b7872 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:33:32 +0200 Subject: [PATCH 10/15] refactor: New Component form combo driven by registry authorable types --- app/src/app.cpp | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/app/src/app.cpp b/app/src/app.cpp index adfaf64..312eba1 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -742,14 +742,20 @@ void RfSimulatorApp::drawComponentFormModal() { ImGui::TextUnformatted(m_component_form_is_edit ? "Edit Component" : "New Component"); ImGui::Separator(); if (!m_component_form_is_edit) { - const char *type_names[] = {"amplifier", "attenuator", "splitter", "filter", - "mixer", "equalizer", "combiner", "adc"}; + std::vector authorable; + for (auto *d : ComponentTypeRegistry::instance().all()) + if (d->authorable) + authorable.push_back(d); static int type_idx = 0; - for (int i = 0; i < 8; ++i) - if (m_component_form_model->descriptor().type == type_names[i]) - type_idx = i; - if (ImGui::Combo("Type", &type_idx, type_names, 8)) - openNewComponentForm(type_names[type_idx]); + for (size_t i = 0; i < authorable.size(); ++i) + if (m_component_form_model->descriptor().type == authorable[i]->type) + type_idx = static_cast(i); + std::vector type_names; + for (auto *d : authorable) + type_names.push_back(d->type.c_str()); + if (ImGui::Combo("Type", &type_idx, type_names.data(), + static_cast(type_names.size()))) + openNewComponentForm(authorable[type_idx]->type); const char *roots[] = {"Project (./rf-sim-libraries)", "Global (~/.rf-sim/libraries)"}; static int root_idx = 0; From d24964e2b7b9f790917989f9eb6b995ede3906d4 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:41:43 +0200 Subject: [PATCH 11/15] refactor: extract PFBViewManager from RfSimulatorApp --- app/CMakeLists.txt | 1 + app/include/app.h | 8 ++-- app/include/pfb_view_manager.h | 32 +++++++++++++++ app/src/app.cpp | 73 +++++----------------------------- app/src/pfb_view_manager.cpp | 57 ++++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 68 deletions(-) create mode 100644 app/include/pfb_view_manager.h create mode 100644 app/src/pfb_view_manager.cpp diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index b82711c..ba3e39e 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -17,6 +17,7 @@ add_library(app STATIC src/extension_manifest.cpp src/extension_manager.cpp src/external_tool_runner.cpp + src/pfb_view_manager.cpp ) add_library(simulator::app ALIAS app) diff --git a/app/include/app.h b/app/include/app.h index 47a9c4d..490fc11 100644 --- a/app/include/app.h +++ b/app/include/app.h @@ -25,6 +25,7 @@ #include "node_graph_widget.h" #include "pfb_channelizer_engine.h" #include "pfb_channelizer_widget.h" +#include "pfb_view_manager.h" #include "session_state.h" #include "signal_generator_engine.h" #include "signal_generator_widget.h" @@ -105,9 +106,6 @@ class RfSimulatorApp { std::unique_ptr m_graph_widget; std::vector> m_generator_widgets; - std::vector> m_iq_widgets; - std::vector m_show_iq_pfbs; - std::vector> m_pfb_grid_widgets; ComponentLibrary m_library; std::unique_ptr m_library_browser; bool m_show_library = false; @@ -117,10 +115,12 @@ class RfSimulatorApp { std::unique_ptr m_component_form_model; std::unique_ptr m_component_form_widget; std::string m_component_form_error; - std::vector m_show_pfb_grids; std::unique_ptr m_inspector_panel; ComponentRegistry m_components; + // Declared after m_components so the manager (and its widget references to + // engines) is destroyed before the engines themselves. + PFBViewManager m_pfb_views; int m_next_component_id = 100; PendingAction m_pending_action = PendingAction::None; bool m_show_unsaved_dialog = false; diff --git a/app/include/pfb_view_manager.h b/app/include/pfb_view_manager.h new file mode 100644 index 0000000..82c8978 --- /dev/null +++ b/app/include/pfb_view_manager.h @@ -0,0 +1,32 @@ +#pragma once + +#include "iq_plot_widget.h" +#include "pfb_channelizer_engine.h" +#include "pfb_channelizer_widget.h" +#include +#include + +class ComponentRegistry; +class SessionState; + +// Owns the per-PFB view widgets and their visibility flags. The app's old +// four lockstep vectors (m_iq_widgets/m_show_iq_pfbs/m_pfb_grid_widgets/ +// m_show_pfb_grids) were rebuilt by hand at six call sites and caused issue +// #37 (use-after-free). All lifecycle now funnels through this class. +class PFBViewManager { + public: + void addFor(PFBChannelizerEngine &engine, SessionState &state); + void rebuild(const ComponentRegistry &components, SessionState &state); + void clear(); + void draw(); + void saveVisibility(const ComponentRegistry &components, SessionState &state) const; + + std::vector &iqVisibility() { return m_show_iq_pfbs; } + std::vector &gridVisibility() { return m_show_pfb_grids; } + + private: + std::vector> m_iq_widgets; + std::vector m_show_iq_pfbs; + std::vector> m_pfb_grid_widgets; + std::vector m_show_pfb_grids; +}; diff --git a/app/src/app.cpp b/app/src/app.cpp index 312eba1..001754d 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -56,19 +56,7 @@ RfSimulatorApp::RfSimulatorApp() : m_components(m_graph_engine, m_view_manager) // directly during this same draw_ui() call (e.g. PFBChannelizerWidget::draw() / // rebuildCache(), InspectorPanel) would otherwise use-after-free. See issue #37. rewireInputs(); - auto pfb_vec = m_components.byType(); - m_iq_widgets.clear(); - m_show_iq_pfbs.clear(); - m_pfb_grid_widgets.clear(); - m_show_pfb_grids.clear(); - for (auto *pfb : pfb_vec) { - m_iq_widgets.push_back(std::make_unique(*pfb)); - m_show_iq_pfbs.push_back(m_state.loadBool( - "WindowState", ("IQPlot_" + std::to_string(pfb->id())).c_str(), true)); - m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); - m_show_pfb_grids.push_back(m_state.loadBool( - "WindowState", ("PFBGrid_" + std::to_string(pfb->id())).c_str(), true)); - } + m_pfb_views.rebuild(m_components, m_state); }; m_graph_widget->onNodeHover = [this](int id) { return m_components.hoverSummary(id); }; m_graph_widget->onDuplicateNode = [this](int id) { duplicateComponent(id); }; @@ -119,13 +107,7 @@ void RfSimulatorApp::addComponent(const ComponentTypeDescriptor *desc, ImVec2 po ImNodes::EditorContextSet(m_graph_widget->context()); ImNodes::SetNodeEditorSpacePos(comp->graphNodeId(), pos); if (desc->type == "pfb") { - auto *pfb = static_cast(comp); - m_iq_widgets.push_back(std::make_unique(*pfb)); - m_show_iq_pfbs.push_back( - m_state.loadBool("WindowState", ("IQPlot_" + std::to_string(pfb->id())).c_str(), true)); - m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); - m_show_pfb_grids.push_back(m_state.loadBool( - "WindowState", ("PFBGrid_" + std::to_string(pfb->id())).c_str(), true)); + m_pfb_views.addFor(*static_cast(comp), m_state); } markDirty(); // unconditional — fixes the Equalizer missing-markDirty bug } @@ -173,13 +155,7 @@ void RfSimulatorApp::duplicateComponent(int graph_node_id) { m_graph_engine.setNodePartNumber(new_nid, src_part_number); // PFB also needs IQ plot widget and grid widget (same as addComponent) if (desc->type == "pfb") { - auto *new_pfb = static_cast(copy); - m_iq_widgets.push_back(std::make_unique(*new_pfb)); - m_show_iq_pfbs.push_back(m_state.loadBool( - "WindowState", ("IQPlot_" + std::to_string(new_pfb->id())).c_str(), true)); - m_pfb_grid_widgets.push_back(std::make_unique(*new_pfb)); - m_show_pfb_grids.push_back(m_state.loadBool( - "WindowState", ("PFBGrid_" + std::to_string(new_pfb->id())).c_str(), true)); + m_pfb_views.addFor(*static_cast(copy), m_state); } markDirty(); @@ -202,10 +178,7 @@ void RfSimulatorApp::newProject() { m_spectrum_widget->setProbeLabels({}); // Reset IQ / PFB widgets - m_iq_widgets.clear(); - m_show_iq_pfbs.clear(); - m_pfb_grid_widgets.clear(); - m_show_pfb_grids.clear(); + m_pfb_views.clear(); // Reset graph counters m_graph_engine.setNextIds(1, 100, 1000); @@ -511,12 +484,8 @@ void RfSimulatorApp::loadProject(const std::string &path) { IComponentEngine *comp = desc->create(m_components, m_graph_engine, m_next_component_id++); comp->deserialize(params); if (desc->type == "pfb") { - auto *pfb = static_cast(comp); // Restore IQ plot + PFB grid widgets for this PFB - m_iq_widgets.push_back(std::make_unique(*pfb)); - m_show_iq_pfbs.push_back(true); - m_pfb_grid_widgets.push_back(std::make_unique(*pfb)); - m_show_pfb_grids.push_back(true); + m_pfb_views.addFor(*static_cast(comp), m_state); } new_node_ids.push_back(comp ? comp->graphNodeId() : -1); @@ -852,7 +821,8 @@ void RfSimulatorApp::update_dsp() { std::vector pfb_vec(pfb_ptrs.begin(), pfb_ptrs.end()); m_spectrum_widget->setPFBs(pfb_vec); m_inspector_panel->setPFBs(pfb_vec); - m_inspector_panel->setPFBWindowVisibility(&m_show_iq_pfbs, &m_show_pfb_grids); + m_inspector_panel->setPFBWindowVisibility(&m_pfb_views.iqVisibility(), + &m_pfb_views.gridVisibility()); } void RfSimulatorApp::draw_ui() { @@ -1143,23 +1113,7 @@ void RfSimulatorApp::draw_ui() { if (m_show_spectrum) m_spectrum_widget->draw("Spectrum Analyzer", &m_show_spectrum); - for (size_t i = 0; i < m_iq_widgets.size(); ++i) { - if (m_show_iq_pfbs[i]) { - std::string label = "IQ Plot - PFB " + std::to_string(i); - bool show = m_show_iq_pfbs[i]; - m_iq_widgets[i]->draw(label.c_str(), &show); - m_show_iq_pfbs[i] = show; - } - } - - for (size_t i = 0; i < m_pfb_grid_widgets.size(); ++i) { - if (m_show_pfb_grids[i]) { - std::string label = "Channelizer Grid - PFB " + std::to_string(i); - bool show = m_show_pfb_grids[i]; - m_pfb_grid_widgets[i]->draw(label.c_str(), &show); - m_show_pfb_grids[i] = show; - } - } + m_pfb_views.draw(); for (size_t i = 0; i < m_generator_widgets.size(); ++i) { m_generator_widgets[i]->draw("Generators"); @@ -1184,16 +1138,7 @@ RfSimulatorApp::~RfSimulatorApp() { m_state.saveBool("WindowState", "Log", m_show_log); m_state.saveBool("WindowState", "SpectrumAnalyzer", m_show_spectrum); m_state.saveBool("WindowState", "Properties", m_show_properties); - auto pfb_vec = m_components.byType(); - for (size_t i = 0; i < m_show_iq_pfbs.size() && i < pfb_vec.size(); ++i) { - std::string key = "IQPlot_" + std::to_string(pfb_vec[i]->id()); - m_state.saveBool("WindowState", key.c_str(), m_show_iq_pfbs[i]); - } - auto pfb_vec_save = m_components.byType(); - for (size_t i = 0; i < m_show_pfb_grids.size() && i < pfb_vec_save.size(); ++i) { - std::string key = "PFBGrid_" + std::to_string(pfb_vec_save[i]->id()); - m_state.saveBool("WindowState", key.c_str(), m_show_pfb_grids[i]); - } + m_pfb_views.saveVisibility(m_components, m_state); m_state.saveBool("WindowState", "NodeEditor", m_show_node_editor); m_state.saveBool("WindowState", "Help", m_show_help); } diff --git a/app/src/pfb_view_manager.cpp b/app/src/pfb_view_manager.cpp new file mode 100644 index 0000000..530086f --- /dev/null +++ b/app/src/pfb_view_manager.cpp @@ -0,0 +1,57 @@ +#include "pfb_view_manager.h" +#include "component_registry.h" +#include "session_state.h" + +void PFBViewManager::addFor(PFBChannelizerEngine &engine, SessionState &state) { + m_iq_widgets.push_back(std::make_unique(engine)); + m_show_iq_pfbs.push_back( + state.loadBool("WindowState", ("IQPlot_" + std::to_string(engine.id())).c_str(), true)); + m_pfb_grid_widgets.push_back(std::make_unique(engine)); + m_show_pfb_grids.push_back( + state.loadBool("WindowState", ("PFBGrid_" + std::to_string(engine.id())).c_str(), true)); +} + +void PFBViewManager::rebuild(const ComponentRegistry &components, SessionState &state) { + clear(); + for (auto *pfb : components.byType()) + addFor(*pfb, state); +} + +void PFBViewManager::clear() { + m_iq_widgets.clear(); + m_show_iq_pfbs.clear(); + m_pfb_grid_widgets.clear(); + m_show_pfb_grids.clear(); +} + +void PFBViewManager::draw() { + for (size_t i = 0; i < m_iq_widgets.size(); ++i) { + if (m_show_iq_pfbs[i]) { + std::string label = "IQ Plot - PFB " + std::to_string(i); + bool show = m_show_iq_pfbs[i]; + m_iq_widgets[i]->draw(label.c_str(), &show); + m_show_iq_pfbs[i] = show; + } + } + for (size_t i = 0; i < m_pfb_grid_widgets.size(); ++i) { + if (m_show_pfb_grids[i]) { + std::string label = "Channelizer Grid - PFB " + std::to_string(i); + bool show = m_show_pfb_grids[i]; + m_pfb_grid_widgets[i]->draw(label.c_str(), &show); + m_show_pfb_grids[i] = show; + } + } +} + +void PFBViewManager::saveVisibility(const ComponentRegistry &components, + SessionState &state) const { + auto pfb_vec = components.byType(); + for (size_t i = 0; i < m_show_iq_pfbs.size() && i < pfb_vec.size(); ++i) { + std::string key = "IQPlot_" + std::to_string(pfb_vec[i]->id()); + state.saveBool("WindowState", key.c_str(), m_show_iq_pfbs[i]); + } + for (size_t i = 0; i < m_show_pfb_grids.size() && i < pfb_vec.size(); ++i) { + std::string key = "PFBGrid_" + std::to_string(pfb_vec[i]->id()); + state.saveBool("WindowState", key.c_str(), m_show_pfb_grids[i]); + } +} From bb16abe6cadfcc98cb39cfc9692256a2a7ea8dbc Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 18:57:15 +0200 Subject: [PATCH 12/15] refactor: extract ProjectSerializer from RfSimulatorApp --- app/CMakeLists.txt | 1 + app/include/app.h | 4 + app/include/project_serializer.h | 35 ++++ app/src/app.cpp | 296 +--------------------------- app/src/project_serializer.cpp | 324 +++++++++++++++++++++++++++++++ 5 files changed, 370 insertions(+), 290 deletions(-) create mode 100644 app/include/project_serializer.h create mode 100644 app/src/project_serializer.cpp diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index ba3e39e..fffec69 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -18,6 +18,7 @@ add_library(app STATIC src/extension_manager.cpp src/external_tool_runner.cpp src/pfb_view_manager.cpp + src/project_serializer.cpp ) add_library(simulator::app ALIAS app) diff --git a/app/include/app.h b/app/include/app.h index 490fc11..8ce6bc2 100644 --- a/app/include/app.h +++ b/app/include/app.h @@ -26,6 +26,7 @@ #include "pfb_channelizer_engine.h" #include "pfb_channelizer_widget.h" #include "pfb_view_manager.h" +#include "project_serializer.h" #include "session_state.h" #include "signal_generator_engine.h" #include "signal_generator_widget.h" @@ -121,6 +122,9 @@ class RfSimulatorApp { // Declared after m_components so the manager (and its widget references to // engines) is destroyed before the engines themselves. PFBViewManager m_pfb_views; + // Owns .rfsim save/load/new; declared after m_graph_widget and m_pfb_views + // so it is destroyed before them (it holds references to both). + std::unique_ptr m_serializer; int m_next_component_id = 100; PendingAction m_pending_action = PendingAction::None; bool m_show_unsaved_dialog = false; diff --git a/app/include/project_serializer.h b/app/include/project_serializer.h new file mode 100644 index 0000000..c618186 --- /dev/null +++ b/app/include/project_serializer.h @@ -0,0 +1,35 @@ +#pragma once + +#include + +class ComponentRegistry; +class NodeGraphEngine; +class NodeGraphWidget; +class PFBViewManager; +class SessionState; + +// Owns the .rfsim JSON save/load/new logic previously inlined in +// RfSimulatorApp (issue #51: 1320-line god-object). +class ProjectSerializer { + public: + ProjectSerializer(ComponentRegistry &components, NodeGraphEngine &graph, + NodeGraphWidget &graph_widget, PFBViewManager &pfb_views, SessionState &state, + int &next_component_id, bool &show_log, bool &show_spectrum, + bool &show_properties, bool &show_node_editor); + + void save(const std::string &path); + bool load(const std::string &path); // false on parse/unknown-type failure (logged) + void reset(); // newProject: links, components, probes, counters, PFBs + + private: + ComponentRegistry &m_components; + NodeGraphEngine &m_graph; + NodeGraphWidget &m_graph_widget; + PFBViewManager &m_pfb_views; + SessionState &m_state; + int &m_next_component_id; + bool &m_show_log; + bool &m_show_spectrum; + bool &m_show_properties; + bool &m_show_node_editor; +}; diff --git a/app/src/app.cpp b/app/src/app.cpp index 001754d..de32771 100644 --- a/app/src/app.cpp +++ b/app/src/app.cpp @@ -35,6 +35,9 @@ static std::string sanitizePathSegment(const std::string &s, const std::string & RfSimulatorApp::RfSimulatorApp() : m_components(m_graph_engine, m_view_manager) { m_graph_widget = std::make_unique(m_graph_engine); + m_serializer = std::make_unique( + m_components, m_graph_engine, *m_graph_widget, m_pfb_views, m_state, m_next_component_id, + m_show_log, m_show_spectrum, m_show_properties, m_show_node_editor); std::vector addable; for (const auto *desc : ComponentTypeRegistry::instance().all()) { addable.push_back( @@ -162,34 +165,10 @@ void RfSimulatorApp::duplicateComponent(int graph_node_id) { } void RfSimulatorApp::newProject() { - // Remove all links from the graph engine first - m_graph_engine.removeAllLinks(); - - // Remove all components — ComponentRegistry handles cleanup - // Collect IDs first to avoid iterator invalidation - std::vector ids; - for (auto *comp : m_components.all()) - ids.push_back(comp->graphNodeId()); - for (int id : ids) - m_components.remove(id); - - // Clear probes - m_graph_engine.clearProbes(); + m_serializer->reset(); m_spectrum_widget->setProbeLabels({}); - - // Reset IQ / PFB widgets - m_pfb_views.clear(); - - // Reset graph counters - m_graph_engine.setNextIds(1, 100, 1000); - m_graph_engine.setNextGroupId(50000); - m_graph_engine.setNextBoundaryPinId(100000); - - m_next_component_id = 100; m_current_project_path.clear(); - m_graph_widget->clearPositionCache(); refreshExtensions(); - m_dirty = false; } @@ -313,280 +292,17 @@ void RfSimulatorApp::drawExtensionsPanel() { } void RfSimulatorApp::saveProject(const std::string &path) { - nlohmann::json root; - root["version"] = 1; - - auto pos = path.find_last_of("\\/"); - std::string fname = (pos != std::string::npos) ? path.substr(pos + 1) : path; - auto dot = fname.find_last_of('.'); - root["name"] = (dot != std::string::npos) ? fname.substr(0, dot) : fname; - - // Ensure all engine nodes are registered with the imnodes context - // so GetNodeEditorSpacePos() doesn't assert on node IDs added without - // a prior render frame (e.g. via newProject then programmatic add). - m_graph_widget->syncNodesFromEngine(); - - // Save components by iterating the registry - nlohmann::json comps_arr = nlohmann::json::array(); - for (auto *comp : m_components.all()) { - nlohmann::json cj; - const auto *desc = ComponentTypeRegistry::instance().find(comp->type_name()); - cj["type"] = desc ? desc->project_type : "Unknown"; - cj["params"] = comp->serialize(); - - // Save node position via imnodes - int nid = comp->graphNodeId(); - ImNodes::EditorContextSet(m_graph_widget->context()); - ImVec2 pos_n = ImNodes::GetNodeEditorSpacePos(nid); - cj["pos"]["x"] = pos_n.x; - cj["pos"]["y"] = pos_n.y; - - // Save library part number if set - for (const auto &gn : m_graph_engine.nodes()) { - if (gn.node_id == nid && !gn.part_number.empty()) { - cj["part_number"] = gn.part_number; - break; - } - } - - comps_arr.push_back(cj); - } - root["components"] = comps_arr; - - // Save links as component-index + port pairs (not raw pin IDs) - nlohmann::json links_arr = nlohmann::json::array(); - // Build a map: pin_id \u2192 {comp_index, port, is_output} - struct PinInfo { - size_t comp; - int port; - bool is_output; - }; - std::unordered_map pin_map; - for (size_t i = 0; i < m_components.size(); ++i) { - auto *comp = m_components.all()[i]; - int nid = comp->graphNodeId(); - for (const auto &gn : m_graph_engine.nodes()) { - if (gn.node_id == nid) { - for (size_t p = 0; p < gn.input_pin_ids.size(); ++p) - pin_map[gn.input_pin_ids[p]] = {i, (int)p, false}; - for (size_t p = 0; p < gn.output_pin_ids.size(); ++p) - pin_map[gn.output_pin_ids[p]] = {i, (int)p, true}; - break; - } - } - } - for (const auto &link : m_graph_engine.links()) { - auto from_it = pin_map.find(link.start_pin_id); - auto to_it = pin_map.find(link.end_pin_id); - if (from_it == pin_map.end() || to_it == pin_map.end()) - continue; - nlohmann::json lj; - lj["from"] = from_it->second.comp; - lj["from_port"] = from_it->second.port; - lj["to"] = to_it->second.comp; - lj["to_port"] = to_it->second.port; - links_arr.push_back(lj); - } - root["links"] = links_arr; - - // Save probes as component-index + port - nlohmann::json probes_arr = nlohmann::json::array(); - for (int probe_pin : m_graph_engine.probePins()) { - auto it = pin_map.find(probe_pin); - if (it != pin_map.end()) { - nlohmann::json pj; - pj["comp"] = it->second.comp; - pj["port"] = it->second.port; - pj["is_output"] = it->second.is_output; - probes_arr.push_back(pj); - } - } - root["probe_pins"] = probes_arr; - - // Save groups - nlohmann::json groups_arr = nlohmann::json::array(); - // Build node_id \u2192 comp_index map - std::unordered_map nid_to_comp; - for (size_t i = 0; i < m_components.size(); ++i) - nid_to_comp[m_components.all()[i]->graphNodeId()] = i; - - for (const auto &g : m_graph_engine.groups()) { - nlohmann::json gj; - gj["name"] = g.name; - gj["collapsed"] = g.collapsed; - gj["member_components"] = nlohmann::json::array(); - for (int member_nid : g.member_node_ids) { - auto it = nid_to_comp.find(member_nid); - if (it != nid_to_comp.end()) - gj["member_components"].push_back(it->second); - } - groups_arr.push_back(gj); - } - root["groups"] = groups_arr; - - // Window state - root["window_state"]["log"] = m_show_log; - root["window_state"]["spectrum_analyzer"] = m_show_spectrum; - root["window_state"]["properties"] = m_show_properties; - root["window_state"]["node_editor"] = m_show_node_editor; - - // Graph state counters (for later additions) - root["graph_state"]["next_component_id"] = m_next_component_id; - - std::ofstream out(path); - if (!out) { - LOG_ERROR("Failed to open project file for writing: %s", path.c_str()); - return; - } - out << root.dump(2); - out.close(); - + m_serializer->save(path); m_current_project_path = path; m_dirty = false; - LOG_INFO("Saved project to %s", path.c_str()); } void RfSimulatorApp::loadProject(const std::string &path) { - std::ifstream in(path); - if (!in) { - LOG_ERROR("Failed to open project file: %s", path.c_str()); + if (!m_serializer->load(path)) return; - } - nlohmann::json root; - try { - in >> root; - } catch (const nlohmann::json::exception &e) { - LOG_ERROR("Invalid project file: %s", e.what()); - return; - } - - newProject(); - - // Map: type string \u2192 factory lambda - std::vector comp_order; - auto &comps = root["components"]; - for (auto it = comps.begin(); it != comps.end(); ++it) - comp_order.push_back(it); - - // Create components in saved order - std::vector new_node_ids; // maps saved index \u2192 new graph node ID - for (auto &it : comp_order) { - auto &cj = *it; - std::string type = cj.value("type", ""); - auto ¶ms = cj["params"]; - - const auto *desc = ComponentTypeRegistry::instance().findByProjectType(type); - if (!desc) { - LOG_WARN("Unknown component type in project file: %s", type.c_str()); - new_node_ids.push_back(-1); - continue; - } - IComponentEngine *comp = desc->create(m_components, m_graph_engine, m_next_component_id++); - comp->deserialize(params); - if (desc->type == "pfb") { - // Restore IQ plot + PFB grid widgets for this PFB - m_pfb_views.addFor(*static_cast(comp), m_state); - } - - new_node_ids.push_back(comp ? comp->graphNodeId() : -1); - - // Restore position - if (comp && cj.contains("pos")) { - ImNodes::EditorContextSet(m_graph_widget->context()); - ImNodes::SetNodeEditorSpacePos(comp->graphNodeId(), ImVec2(cj["pos"].value("x", 0.0f), - cj["pos"].value("y", 0.0f))); - } - - // Restore library part number - if (comp && cj.contains("part_number")) - m_graph_engine.setNodePartNumber(comp->graphNodeId(), - cj["part_number"].get()); - } - // After restoring all positions, inform the widget so subsequent - // syncNodesFromEngine calls (e.g. from saveProject) don't reset them. - m_graph_widget->markNodesRegistered(); - - // Restore links (saved as component-index + port pairs) - auto &saved_links = root["links"]; - for (const auto &lj : saved_links) { - int from_idx = lj.value("from", -1); - int to_idx = lj.value("to", -1); - int from_port = lj.value("from_port", 0); - int to_port = lj.value("to_port", 0); - if (from_idx < 0 || to_idx < 0 || static_cast(from_idx) >= new_node_ids.size() || - static_cast(to_idx) >= new_node_ids.size()) - continue; - - int from_node = new_node_ids[from_idx]; - int to_node = new_node_ids[to_idx]; - if (from_node < 0 || to_node < 0) - continue; - - auto *from_comp = m_components.find(from_node); - auto *to_comp = m_components.find(to_node); - if (!from_comp || !to_comp) - continue; - - int start_pin = from_comp->outputPinId(from_port); - int end_pin = to_comp->inputPinId(to_port); - if (start_pin >= 0 && end_pin >= 0) - m_graph_engine.addLink(start_pin, end_pin); - } - - // Restore probes - auto &saved_probes = root["probe_pins"]; - for (const auto &pj : saved_probes) { - int comp_idx = pj.value("comp", -1); - int port = pj.value("port", 0); - bool is_output = pj.value("is_output", true); - if (comp_idx < 0 || static_cast(comp_idx) >= m_components.size()) - continue; - auto *comp = m_components.all()[comp_idx]; - int pin = is_output ? comp->outputPinId(port) : comp->inputPinId(port); - if (pin >= 0) - m_graph_engine.addProbePin(pin); - } - - // Restore groups - auto &saved_groups = root["groups"]; - for (const auto &gj : saved_groups) { - std::string name = gj.value("name", "Group"); - std::vector member_ids; - for (const auto &mj : gj["member_components"]) { - int comp_idx = mj.get(); - if (comp_idx >= 0 && static_cast(comp_idx) < new_node_ids.size() && - new_node_ids[comp_idx] >= 0) { - member_ids.push_back(new_node_ids[comp_idx]); - } - } - if (member_ids.size() >= 2) { - int gid = m_graph_engine.addGroup(name, member_ids); - bool collapsed = gj.value("collapsed", true); - if (gid >= 0) - m_graph_engine.setGroupCollapsed(gid, collapsed); - } - } - - // Restore window state - auto &ws = root["window_state"]; - if (!ws.is_null()) { - m_show_log = ws.value("log", true); - m_show_spectrum = ws.value("spectrum_analyzer", true); - m_show_properties = ws.value("properties", true); - m_show_node_editor = ws.value("node_editor", true); - } - - // Restore graph state counters - auto &gs = root["graph_state"]; - if (!gs.is_null()) { - m_next_component_id = gs.value("next_component_id", m_next_component_id); - } - m_current_project_path = path; refreshExtensions(); - m_dirty = false; - LOG_INFO("Loaded project from %s", path.c_str()); } void RfSimulatorApp::openFileDialog() { diff --git a/app/src/project_serializer.cpp b/app/src/project_serializer.cpp new file mode 100644 index 0000000..ac98c3c --- /dev/null +++ b/app/src/project_serializer.cpp @@ -0,0 +1,324 @@ +#include "project_serializer.h" +#include "component_registry.h" +#include "component_type_registry.h" +#include "imgui.h" +#include "imnodes.h" +#include "logging_core.h" +#include "node_graph_engine.h" +#include "node_graph_widget.h" +#include "pfb_channelizer_engine.h" +#include "pfb_view_manager.h" +#include "session_state.h" +#include +#include +#include +#include +#include + +ProjectSerializer::ProjectSerializer(ComponentRegistry &components, NodeGraphEngine &graph, + NodeGraphWidget &graph_widget, PFBViewManager &pfb_views, + SessionState &state, int &next_component_id, bool &show_log, + bool &show_spectrum, bool &show_properties, + bool &show_node_editor) + : m_components(components), m_graph(graph), m_graph_widget(graph_widget), + m_pfb_views(pfb_views), m_state(state), m_next_component_id(next_component_id), + m_show_log(show_log), m_show_spectrum(show_spectrum), m_show_properties(show_properties), + m_show_node_editor(show_node_editor) {} + +void ProjectSerializer::save(const std::string &path) { + nlohmann::json root; + root["version"] = 1; + + auto pos = path.find_last_of("\\/"); + std::string fname = (pos != std::string::npos) ? path.substr(pos + 1) : path; + auto dot = fname.find_last_of('.'); + root["name"] = (dot != std::string::npos) ? fname.substr(0, dot) : fname; + + // Ensure all engine nodes are registered with the imnodes context + // so GetNodeEditorSpacePos() doesn't assert on node IDs added without + // a prior render frame (e.g. via newProject then programmatic add). + m_graph_widget.syncNodesFromEngine(); + + // Save components by iterating the registry + nlohmann::json comps_arr = nlohmann::json::array(); + for (auto *comp : m_components.all()) { + nlohmann::json cj; + const auto *desc = ComponentTypeRegistry::instance().find(comp->type_name()); + cj["type"] = desc ? desc->project_type : "Unknown"; + cj["params"] = comp->serialize(); + + // Save node position via imnodes + int nid = comp->graphNodeId(); + ImNodes::EditorContextSet(m_graph_widget.context()); + ImVec2 pos_n = ImNodes::GetNodeEditorSpacePos(nid); + cj["pos"]["x"] = pos_n.x; + cj["pos"]["y"] = pos_n.y; + + // Save library part number if set + for (const auto &gn : m_graph.nodes()) { + if (gn.node_id == nid && !gn.part_number.empty()) { + cj["part_number"] = gn.part_number; + break; + } + } + + comps_arr.push_back(cj); + } + root["components"] = comps_arr; + + // Save links as component-index + port pairs (not raw pin IDs) + nlohmann::json links_arr = nlohmann::json::array(); + // Build a map: pin_id \u2192 {comp_index, port, is_output} + struct PinInfo { + size_t comp; + int port; + bool is_output; + }; + std::unordered_map pin_map; + for (size_t i = 0; i < m_components.size(); ++i) { + auto *comp = m_components.all()[i]; + int nid = comp->graphNodeId(); + for (const auto &gn : m_graph.nodes()) { + if (gn.node_id == nid) { + for (size_t p = 0; p < gn.input_pin_ids.size(); ++p) + pin_map[gn.input_pin_ids[p]] = {i, (int)p, false}; + for (size_t p = 0; p < gn.output_pin_ids.size(); ++p) + pin_map[gn.output_pin_ids[p]] = {i, (int)p, true}; + break; + } + } + } + for (const auto &link : m_graph.links()) { + auto from_it = pin_map.find(link.start_pin_id); + auto to_it = pin_map.find(link.end_pin_id); + if (from_it == pin_map.end() || to_it == pin_map.end()) + continue; + nlohmann::json lj; + lj["from"] = from_it->second.comp; + lj["from_port"] = from_it->second.port; + lj["to"] = to_it->second.comp; + lj["to_port"] = to_it->second.port; + links_arr.push_back(lj); + } + root["links"] = links_arr; + + // Save probes as component-index + port + nlohmann::json probes_arr = nlohmann::json::array(); + for (int probe_pin : m_graph.probePins()) { + auto it = pin_map.find(probe_pin); + if (it != pin_map.end()) { + nlohmann::json pj; + pj["comp"] = it->second.comp; + pj["port"] = it->second.port; + pj["is_output"] = it->second.is_output; + probes_arr.push_back(pj); + } + } + root["probe_pins"] = probes_arr; + + // Save groups + nlohmann::json groups_arr = nlohmann::json::array(); + // Build node_id \u2192 comp_index map + std::unordered_map nid_to_comp; + for (size_t i = 0; i < m_components.size(); ++i) + nid_to_comp[m_components.all()[i]->graphNodeId()] = i; + + for (const auto &g : m_graph.groups()) { + nlohmann::json gj; + gj["name"] = g.name; + gj["collapsed"] = g.collapsed; + gj["member_components"] = nlohmann::json::array(); + for (int member_nid : g.member_node_ids) { + auto it = nid_to_comp.find(member_nid); + if (it != nid_to_comp.end()) + gj["member_components"].push_back(it->second); + } + groups_arr.push_back(gj); + } + root["groups"] = groups_arr; + + // Window state + root["window_state"]["log"] = m_show_log; + root["window_state"]["spectrum_analyzer"] = m_show_spectrum; + root["window_state"]["properties"] = m_show_properties; + root["window_state"]["node_editor"] = m_show_node_editor; + + // Graph state counters (for later additions) + root["graph_state"]["next_component_id"] = m_next_component_id; + + std::ofstream out(path); + if (!out) { + LOG_ERROR("Failed to open project file for writing: %s", path.c_str()); + return; + } + out << root.dump(2); + out.close(); + + LOG_INFO("Saved project to %s", path.c_str()); +} + +bool ProjectSerializer::load(const std::string &path) { + std::ifstream in(path); + if (!in) { + LOG_ERROR("Failed to open project file: %s", path.c_str()); + return false; + } + nlohmann::json root; + try { + in >> root; + } catch (const nlohmann::json::exception &e) { + LOG_ERROR("Invalid project file: %s", e.what()); + return false; + } + + reset(); + + // Map: type string \u2192 factory lambda + std::vector comp_order; + auto &comps = root["components"]; + for (auto it = comps.begin(); it != comps.end(); ++it) + comp_order.push_back(it); + + // Create components in saved order + std::vector new_node_ids; // maps saved index \u2192 new graph node ID + for (auto &it : comp_order) { + auto &cj = *it; + std::string type = cj.value("type", ""); + auto ¶ms = cj["params"]; + + const auto *desc = ComponentTypeRegistry::instance().findByProjectType(type); + if (!desc) { + LOG_WARN("Unknown component type in project file: %s", type.c_str()); + new_node_ids.push_back(-1); + continue; + } + IComponentEngine *comp = desc->create(m_components, m_graph, m_next_component_id++); + comp->deserialize(params); + if (desc->type == "pfb") { + // Restore IQ plot + PFB grid widgets for this PFB + m_pfb_views.addFor(*static_cast(comp), m_state); + } + + new_node_ids.push_back(comp ? comp->graphNodeId() : -1); + + // Restore position + if (comp && cj.contains("pos")) { + ImNodes::EditorContextSet(m_graph_widget.context()); + ImNodes::SetNodeEditorSpacePos(comp->graphNodeId(), ImVec2(cj["pos"].value("x", 0.0f), + cj["pos"].value("y", 0.0f))); + } + + // Restore library part number + if (comp && cj.contains("part_number")) + m_graph.setNodePartNumber(comp->graphNodeId(), cj["part_number"].get()); + } + // After restoring all positions, inform the widget so subsequent + // syncNodesFromEngine calls (e.g. from saveProject) don't reset them. + m_graph_widget.markNodesRegistered(); + + // Restore links (saved as component-index + port pairs) + auto &saved_links = root["links"]; + for (const auto &lj : saved_links) { + int from_idx = lj.value("from", -1); + int to_idx = lj.value("to", -1); + int from_port = lj.value("from_port", 0); + int to_port = lj.value("to_port", 0); + if (from_idx < 0 || to_idx < 0 || static_cast(from_idx) >= new_node_ids.size() || + static_cast(to_idx) >= new_node_ids.size()) + continue; + + int from_node = new_node_ids[from_idx]; + int to_node = new_node_ids[to_idx]; + if (from_node < 0 || to_node < 0) + continue; + + auto *from_comp = m_components.find(from_node); + auto *to_comp = m_components.find(to_node); + if (!from_comp || !to_comp) + continue; + + int start_pin = from_comp->outputPinId(from_port); + int end_pin = to_comp->inputPinId(to_port); + if (start_pin >= 0 && end_pin >= 0) + m_graph.addLink(start_pin, end_pin); + } + + // Restore probes + auto &saved_probes = root["probe_pins"]; + for (const auto &pj : saved_probes) { + int comp_idx = pj.value("comp", -1); + int port = pj.value("port", 0); + bool is_output = pj.value("is_output", true); + if (comp_idx < 0 || static_cast(comp_idx) >= m_components.size()) + continue; + auto *comp = m_components.all()[comp_idx]; + int pin = is_output ? comp->outputPinId(port) : comp->inputPinId(port); + if (pin >= 0) + m_graph.addProbePin(pin); + } + + // Restore groups + auto &saved_groups = root["groups"]; + for (const auto &gj : saved_groups) { + std::string name = gj.value("name", "Group"); + std::vector member_ids; + for (const auto &mj : gj["member_components"]) { + int comp_idx = mj.get(); + if (comp_idx >= 0 && static_cast(comp_idx) < new_node_ids.size() && + new_node_ids[comp_idx] >= 0) { + member_ids.push_back(new_node_ids[comp_idx]); + } + } + if (member_ids.size() >= 2) { + int gid = m_graph.addGroup(name, member_ids); + bool collapsed = gj.value("collapsed", true); + if (gid >= 0) + m_graph.setGroupCollapsed(gid, collapsed); + } + } + + // Restore window state + auto &ws = root["window_state"]; + if (!ws.is_null()) { + m_show_log = ws.value("log", true); + m_show_spectrum = ws.value("spectrum_analyzer", true); + m_show_properties = ws.value("properties", true); + m_show_node_editor = ws.value("node_editor", true); + } + + // Restore graph state counters + auto &gs = root["graph_state"]; + if (!gs.is_null()) { + m_next_component_id = gs.value("next_component_id", m_next_component_id); + } + + LOG_INFO("Loaded project from %s", path.c_str()); + return true; +} + +void ProjectSerializer::reset() { + // Remove all links from the graph engine first + m_graph.removeAllLinks(); + + // Remove all components — ComponentRegistry handles cleanup + // Collect IDs first to avoid iterator invalidation + std::vector ids; + for (auto *comp : m_components.all()) + ids.push_back(comp->graphNodeId()); + for (int id : ids) + m_components.remove(id); + + // Clear probes + m_graph.clearProbes(); + + // Reset IQ / PFB widgets + m_pfb_views.clear(); + + // Reset graph counters + m_graph.setNextIds(1, 100, 1000); + m_graph.setNextGroupId(50000); + m_graph.setNextBoundaryPinId(100000); + + m_next_component_id = 100; + m_graph_widget.clearPositionCache(); +} From 7042bb6efe1a2edda30bd740ab6b29b0e07483a5 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 19:02:35 +0200 Subject: [PATCH 13/15] test: all-types round-trip and legacy .rfsim backward compat --- tests/test_component_dispatch.cpp | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/test_component_dispatch.cpp b/tests/test_component_dispatch.cpp index aebe97e..92e4e83 100644 --- a/tests/test_component_dispatch.cpp +++ b/tests/test_component_dispatch.cpp @@ -11,7 +11,10 @@ #include "imgui.h" #include "imnodes.h" #include "implot.h" +#include "pfb_channelizer_engine.h" #include +#include +#include struct ImGuiFixture { ImGuiFixture() { @@ -48,3 +51,49 @@ TEST_CASE_METHOD(ImGuiFixture, "Every registry label_prefix maps to its kind", " } REQUIRE(app.testGraphWidget().kindForLabel("UnknownThing 1") == NodeKind::Unknown); } + +TEST_CASE_METHOD(ImGuiFixture, "All 11 registry types round-trip through project save/load", + "[dispatch]") { + auto path = "test_dispatch_all_types.rfsim"; + std::remove(path); + { + RfSimulatorApp app; + app.newProject(); + for (const auto &addable : app.testGraphWidget().addableComponents()) + addable.on_add(ImVec2(0, 0)); + REQUIRE(app.componentCount() == 11); + app.saveProject(path); + } + { + RfSimulatorApp app; + app.loadProject(path); + REQUIRE(app.componentCount() == 11); + } + std::remove(path); +} + +TEST_CASE_METHOD(ImGuiFixture, "Legacy .rfsim type strings still load (backward compat)", + "[dispatch]") { + auto path = "test_dispatch_legacy.rfsim"; + std::ofstream out(path); + out << R"({ + "version": 1, + "name": "legacy", + "components": [ + {"type": "SignalGenerator", "params": {"tones": [{"freq_Hz": 100e6, "power_dBm": -20.0, "phase_deg": 0.0}]}}, + {"type": "Amplifier", "params": {"gain_dB": 10.0, "nf_dB": 2.0}}, + {"type": "ADC", "params": {"sample_rate_Hz": 1e9}}, + {"type": "IdealFilter", "params": {"filter_type": 1}}, + {"type": "PFBChannelizer", "params": {}}, + {"type": "CoaxCable", "params": {}} + ], + "links": [], + "groups": [] + })"; + out.close(); + RfSimulatorApp app; + app.loadProject(path); + REQUIRE(app.componentCount() == 6); + REQUIRE(app.testComponents().byType().size() == 1); + std::remove(path); +} From 9161231f2b9f33c0eea01cf062e2ed7525c2c4bd Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 19:11:41 +0200 Subject: [PATCH 14/15] docs: update DOX for unified registry, PFBViewManager, ProjectSerializer --- app/AGENTS.md | 14 ++++++++------ common/AGENTS.md | 5 +++-- openwiki/testing/guidance.md | 12 +++++++++--- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/app/AGENTS.md b/app/AGENTS.md index 0ee0ec5..74a5785 100644 --- a/app/AGENTS.md +++ b/app/AGENTS.md @@ -1,18 +1,20 @@ # app/AGENTS.md ## Purpose -Application orchestrator layer containing `RfSimulatorApp`, `ComponentRegistry`, and `InspectorPanel`. +Application orchestrator layer containing `RfSimulatorApp`, `ComponentRegistry`, `ComponentTypeRegistry`, `InspectorPanel`, `PFBViewManager`, and `ProjectSerializer`. ## Ownership -- `RfSimulatorApp` — application boot, frame loop, DSP update, UI orchestration, project save/load +- `RfSimulatorApp` — application boot, frame loop, DSP update, UI orchestration (project save/load logic lives in `ProjectSerializer`) - `ComponentRegistry` — polymorphic component lifecycle and type-indexed lookup - `InspectorPanel` — property editing panel with dirty tracking -- `ComponentTypeRegistry` — data-driven type schema table (field lists + factories) used by `ComponentLibrary::instantiate()`/`validate()` and the component authoring form +- `ComponentTypeRegistry` — single dispatch table (11 rows) for canvas menu, add, duplicate, save/load, and inspector drawing: each row carries the canonical `type` + `.rfsim` `project_type` keys, `menu_label`/`label_prefix`, `NodeKind`, a `create()` factory, and a `draw_inspector` callback; also drives `ComponentLibrary::instantiate()`/`validate()` and the component authoring form +- `PFBViewManager` — owns the per-PFB IQ Plot / Channelizer Grid widget lifecycle (replaces the app's four lockstep vectors `m_iq_widgets`/`m_show_iq_pfbs`/`m_pfb_grid_widgets`/`m_show_pfb_grids`, which were rebuilt by hand at six call sites and caused issue #37); all add/rebuild/clear/draw and visibility state funnel through this class +- `ProjectSerializer` — owns the `.rfsim` save/load/new JSON logic (extracted from `RfSimulatorApp`, issue #51) - `ComponentFormModel` / `ComponentFormWidget` — pure-logic + ImGui rendering pair for the New/Edit Component form - `ExtensionManager` — extension manifest discovery and status tracking across built-in/global/project-local roots - `ExternalToolRunner` — structured request/result execution for approved external tools ## Local Contracts -- `saveProject()` / `loadProject()` / `newProject()` handle full project serialization to `.rfsim` JSON format +- `RfSimulatorApp::saveProject()` / `loadProject()` / `newProject()` are thin wrappers that delegate to `ProjectSerializer::save()` / `load()` / `reset()`; all `.rfsim` JSON serialization lives in `ProjectSerializer` - Dirty tracking propagated via `markDirty()` / `onParamChange` / `onLinkChanged` callbacks - File menu bar in `draw_ui()` handles keyboard shortcuts (`Ctrl+N`, `Ctrl+O`, `Ctrl+S`, `Ctrl+Shift+S`) and unsaved-changes modal; Help menu provides F1-toggled help window - View menu's `Layouts` submenu (Save As.../Load/Manage...) drives `LayoutManager` (see `layout/AGENTS.md`) for named window-layout presets; the exe-relative default layout is auto-managed by ImGui itself via `IniFilename`, set in `core/src/core.cpp` @@ -25,12 +27,12 @@ Application orchestrator layer containing `RfSimulatorApp`, `ComponentRegistry`, - `draw_ui()` and `drawExtensionsPanel()` both dispatch through `externalToolActions()`; the Tools menu renders only actions whose `location == "tools"`, while the Extensions panel shows every declared action (or a single fallback Run button) - App-level integration tests may use `testExtensionManager()` and `testExtensionResultMessage()` with an ImGui/ImPlot/ImNodes fixture - `load_window_states()` runs on construction to restore persisted window visibility toggles -- Per-PFB IQ Plot / Channelizer Grid window visibility (`m_show_iq_pfbs`/`m_show_pfb_grids`, indexed in lockstep with `m_pfb_ptrs`) has no View-menu entry since instances are dynamic; closed windows are reopened via "Show IQ Plot"/"Show Channelizer Grid" checkboxes in the PFB properties panel, wired each frame through `InspectorPanel::setPFBWindowVisibility()` (stores the stable vector pointers, not element pointers, since the vectors are rebuilt on add/remove) +- Per-PFB IQ Plot / Channelizer Grid window visibility lives in `PFBViewManager::iqVisibility()`/`gridVisibility()` (indexed in lockstep with the manager's widgets) and has no View-menu entry since instances are dynamic; closed windows are reopened via "Show IQ Plot"/"Show Channelizer Grid" checkboxes in the PFB properties panel, wired each frame through `InspectorPanel::setPFBWindowVisibility()` (stores the stable vector pointers, not element pointers, since the vectors are rebuilt on add/remove) - `update_dsp()`'s signal-routing pass is factored into `rewireInputs()` (sets every component's `node().inputs[k]` from current graph links, nulling severed ones); `onRemoveNode` calls it synchronously right after `ComponentRegistry::remove()` so no surviving component is left holding a dangling `Spectrum*` into the just-destroyed engine's `SignalNode` while the rest of that frame's `draw_ui()` still runs — widgets that dereference `node().inputs[]` directly during draw (e.g. `PFBChannelizerWidget`) would otherwise use-after-free (issue #37) - Destructor saves window state via `SessionState` ## Work Guidance -- Add new component serialization in both `saveProject()` (dump to JSON) and `loadProject()` (read from JSON + create via `ComponentRegistry`) +- Add a new component = one `ComponentTypeRegistry` row (`type`, `project_type`, `menu_label`, `label_prefix`, `kind`, `create`, `draw_inspector`) + a `NodeKind`/symbol entry in `node_graph` — the menu, add, duplicate, save/load, inspector, and form paths all dispatch through the registry, so no per-file edits in `RfSimulatorApp` are needed ## Verification - Round-trip tests in `tests/test_project_file.cpp` diff --git a/common/AGENTS.md b/common/AGENTS.md index 9c45ae2..ecd2887 100644 --- a/common/AGENTS.md +++ b/common/AGENTS.md @@ -9,7 +9,7 @@ Own the header-only data model shared by all RF Simulator modules: `SignalNode`, - `common/common.h` — `MIN_FREQ`, `MAX_FREQ`, `MIN_POWER`, `MAX_POWER`, `DEFAULT_VBW`, `DEFAULT_RBW`, physical constants (`k`, `T`, `R`), `dbToLinear`, `calculateNoiseTemp`, `addedNoiseDensity_W_per_Hz`, `addedNoisePerBin_W`, `buildDefaultFrequencyGrid` - `common/signal_node.h` — `SignalNode` (input + output spectra + view_enabled) - `common/spectrum.h` — `Spectrum` (frequencies, tones, noise vectors, phase, generation counter, `fs_Hz`, `is_complex_baseband`) and `Peak`; also the free helper `conjugateSymmetricExpand()` for expanding real-domain tones into +-fc conjugate-symmetric pairs -- `common/component_interface.h` — `IComponentEngine` (DSP engine contract) +- `common/component_interface.h` — `IComponentEngine` (DSP engine contract; pure-virtual `type_name()` returns the canonical lowercase type key used by registry dispatch) - `common/view_manager.h` — `ViewManager` (registry of `SignalNode*`) - `common/include/group.h` — `Group` and `GroupBoundaryPin` (subcircuit grouping data) - `common/iq_stream.h` — `IQStream` (used by the digital chain) @@ -27,7 +27,8 @@ Own the header-only data model shared by all RF Simulator modules: `SignalNode`, - Changes to `SignalNode` or `Spectrum` affect every engine. Update all engines' `update()` and tests. - `Spectrum::is_complex_baseband` (default `false`) marks spectra downstream of an ADC's DDC (complex baseband/IQ); every pass-through engine propagates it from its input exactly like `fs_Hz`. Only `AdcEngine`'s output sets it to `true`. `conjugateSymmetricExpand()` must stay render-only (used by the spectrum-analyzer render path for real-domain spectra) — never call it from interior DSP (generator, `nonlinear_model.h`, gain/filter/S-param stages, mixer), which must keep operating on the collapsed single-entry-per-tone representation. -- New fields on `IComponentEngine` must keep a default implementation that preserves backward compat for all existing engines. +- `IComponentEngine::type_name()` is an intentional pure virtual returning the canonical lowercase type key (e.g. `"amplifier"`); every engine — including new ones — MUST implement it, and the key must match the `ComponentTypeRegistry` row's `type`. +- New virtual members on `IComponentEngine` (other than `type_name()`, which is intentionally pure) must keep a default implementation that preserves backward compat for all existing engines. - New headers in `common/` or `common/include/` are exposed automatically through the `simulator::common` INTERFACE target's include directories (both directories are added explicitly; there is no glob) — no CMake edit is required for a new header. ## Verification diff --git a/openwiki/testing/guidance.md b/openwiki/testing/guidance.md index 8bc4f91..5f58f81 100644 --- a/openwiki/testing/guidance.md +++ b/openwiki/testing/guidance.md @@ -7,7 +7,7 @@ tags: [testing, catch2, unit-tests, ui-tests] # Testing Guide -RF Simulator has **~210 test cases** (including 14 benchmarks) across **23 test source files** (plus 2 standalone executables), covering all DSP engines, the node graph, touchstone parser, PFB channelizer, amplifier nonlinear model, P1dB, component library, project save/load, subcircuits, and UI. The test suite uses **two frameworks**: Catch2 for unit/benchmark tests and **imgui_test_engine** for UI interaction tests. +RF Simulator has **~210 test cases** (including 14 benchmarks) across **28 test source files** (21 compiled into the main `tests` executable — 22 on Windows with `test_session_state.cpp` — plus **7 standalone executables**), covering all DSP engines, the node graph, touchstone parser, PFB channelizer, amplifier nonlinear model, P1dB, component library, project save/load, subcircuits, and UI. The test suite uses **two frameworks**: Catch2 for unit/benchmark tests and **imgui_test_engine** for UI interaction tests. --- @@ -38,12 +38,12 @@ build/bin/test_ui **Build target:** `tests` (links against `Catch2::Catch2WithMain`). -These test files are compiled into the main `tests` executable (20 files; 21 on Windows with `test_session_state.cpp`). Two additional standalone executables — `test_attenuator` and `test_combiner` — are built separately because they link only specific engine libraries. +These test files are compiled into the main `tests` executable (21 files; 22 on Windows with `test_session_state.cpp`). Seven standalone executables are built separately: `test_attenuator` and `test_combiner` link only specific engine libraries; the newer ones link `simulator::app` (and were kept out of the main `tests` binary because this project's MinGW-w64 toolchain silently drops TEST_CASEs registered beyond the ~217 already linked into `tests.exe`). | Test File | Tags | What It Tests | |---|---|---| | `test_main.cpp` | `[common]`, `[generator]`, `[splitter]`, `[mixer]`, `[amplifier]`, `[phase]` | Core math utils, generator, splitter, mixer, basic amplifier | -| `test_node_graph_engine.cpp` | `[node_graph]`, `[appearance]` | Topology, linking, probes, `nodeKindFromLabel`, `themeColor` | +| `test_node_graph_engine.cpp` | `[node_graph]`, `[appearance]` | Topology, linking, probes, `themeColor` (label→`NodeKind` mapping is covered by `test_component_dispatch`, see standalone executables) | | `test_touchstone.cpp` | `[touchstone]` | .sNp parser: real files, synthetic files, error cases | | `test_adc.cpp` | `[adc]` | ADC DDC, aliasing, NSD noise, Fs clamping | | `test_nonlinear_p1db.cpp` | `[nonlinear]`, `[p1db]` | NonlinearModel P1dB default, setter, OIP3 derivation | @@ -59,6 +59,7 @@ These test files are compiled into the main `tests` executable (20 files; 21 on | `test_equalizer.cpp` | `[equalizer]`, `[sparam]` | Equalizer ideal mode, S-param mode, NaN guards | | `test_group.cpp` | `[group]`, `[integration]` | Group operations, boundary pins, signal flow through groups | | `test_iq_plot.cpp` | `[iq_plot]` | `build_iq_spectrum` IFFT, Parseval, empty/degenerate grids | +| `test_layout_manager.cpp` | `[layout]` | Layout path derivation, name sanitization, named-preset save/load | | `test_project_file.cpp` | `[project_file]` | Save/load round-trip: empty project, linked components, newProject, parameter values, groups, invalid JSON | | `test_session_state.cpp` | `[session]` | Windows-only: INI save/load round-trip | | `test_bench_dsp.cpp` | `[bench]`, `[generator]`, `[amplifier]`, `[mixer]`, `[splitter]`, `[pfb]`, `[spectrum]` | Per-engine dirty/clean benchmarks | @@ -70,6 +71,11 @@ These test files are compiled into the main `tests` executable (20 files; 21 on |---|---|---| | `test_attenuator.cpp` | `test_attenuator` | Pass-through, flat attenuation, passive noise model, noise floor convergence, S-param, clamping, dirty-flag, hover | | `test_combiner.cpp` | `test_combiner` | Basic combination, single/both inputs, dirty-flag, S-param mode | +| `test_component_authoring.cpp` | `test_component_authoring` | ComponentTypeRegistry descriptors, ComponentLibrary validate, ComponentFormModel build/validate/round-trip | +| `test_extensions.cpp` | `test_extensions` | Extension manifest parsing/rejection, discovery across built-in/global/project-local roots, ExternalToolRunner request/result flow | +| `test_issue37_pfb_input_removal.cpp` | `test_issue37_pfb_input_removal` | Issue #37 regression: removing an upstream node immediately nulls downstream dangling input pointers | +| `test_component_dispatch.cpp` | `test_component_dispatch` | Registry-driven dispatch: menu add marks project dirty, `kindForLabel` label→NodeKind mapping, all 11 types round-trip through save/load, legacy `.rfsim` type strings backward compat | +| `test_signal_domain.cpp` | `test_signal_domain` | `is_complex_baseband` defaults and propagation through every engine, `conjugateSymmetricExpand` expansion | ### UI Tests (`test_engine/`) From 7f472e8087ba5726e40943713ab1fff819f50565 Mon Sep 17 00:00:00 2001 From: Jaco du Preez Date: Thu, 6 Aug 2026 19:37:10 +0200 Subject: [PATCH 15/15] fix: PFB inspector selector coherence + draw_inspector completeness test --- app/include/inspector_panel.h | 12 ++++++ app/src/inspector_panel.cpp | 68 +++++++++++++++++-------------- tests/test_component_dispatch.cpp | 19 +++++++++ 3 files changed, 69 insertions(+), 30 deletions(-) diff --git a/app/include/inspector_panel.h b/app/include/inspector_panel.h index e122c52..7dc442f 100644 --- a/app/include/inspector_panel.h +++ b/app/include/inspector_panel.h @@ -36,9 +36,17 @@ class InspectorPanel { void draw(const char *title, bool *p_open = nullptr); void setPFBs(const std::vector &pfbs) { + const bool selection_survives = + m_selected_pfb_index >= 0 && m_selected_pfb_index < static_cast(pfbs.size()) && + m_selected_pfb_index < static_cast(m_pfb_ptrs.size()) && + pfbs[m_selected_pfb_index] == m_pfb_ptrs[m_selected_pfb_index]; m_pfb_ptrs = pfbs; if (m_selected_pfb_index >= static_cast(m_pfb_ptrs.size())) m_selected_pfb_index = std::max(0, static_cast(m_pfb_ptrs.size()) - 1); + // A PFB was added/removed and the combo selection did not survive the + // reshuffle: drop the anchor so draw() re-follows the graph selection. + if (!selection_survives) + m_pfb_combo_graph_id = -1; } // Vectors are owned by the caller (RfSimulatorApp) and stay stable across frames; // only their contents are rebuilt on add/remove, so storing the vector pointers @@ -75,6 +83,10 @@ class InspectorPanel { ComponentRegistry *m_components = nullptr; std::vector m_pfb_ptrs; int m_selected_pfb_index = 0; + // Graph-selected PFB id that the combo selection is anchored to; -1 means + // draw() must re-follow the graph selection (initial state, or after the + // PFB set changed underneath the selection). + int m_pfb_combo_graph_id = -1; ViewToggles m_viewToggles; std::vector *m_pfb_iq_visible = nullptr; std::vector *m_pfb_grid_visible = nullptr; diff --git a/app/src/inspector_panel.cpp b/app/src/inspector_panel.cpp index fed2eaf..892b6c9 100644 --- a/app/src/inspector_panel.cpp +++ b/app/src/inspector_panel.cpp @@ -129,45 +129,53 @@ void InspectorPanel::draw(const char *title, bool *p_open) { m_param_edited = false; - // Build label from graph node - int node_id = hit.engine->graphNodeId(); - - ImGui::SeparatorText(labelForHit(hit).c_str()); - + // The PFB multi-instance combo decides which PFB the panel edits. Keeping + // a single "edited engine" means the header, the property controls, and the + // Show-IQ-Plot / Show-Channelizer-Grid checkboxes always refer to the same + // PFB. The graph-selected PFB is the default until the combo is used. + IComponentEngine *edit_engine = hit.engine; if (hit.desc->type == "pfb") { - // PFB keeps its multi-instance selector combo (needs m_pfb_ptrs). auto *pfb = static_cast(hit.engine); - for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { - if (m_pfb_ptrs[i] == pfb) { - m_selected_pfb_index = i; - break; + if (m_pfb_combo_graph_id != pfb->id()) { + // Graph selection moved to a different PFB (or the anchor was + // dropped after an add/remove): follow the graph selection. + m_pfb_combo_graph_id = pfb->id(); + m_selected_pfb_index = -1; + for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { + if (m_pfb_ptrs[i] == pfb) { + m_selected_pfb_index = i; + break; + } } } - if (!m_pfb_ptrs.empty()) { - int display_id = (m_selected_pfb_index < static_cast(m_pfb_ptrs.size()) && - m_pfb_ptrs[m_selected_pfb_index]) - ? m_pfb_ptrs[m_selected_pfb_index]->id() - : m_selected_pfb_index; - std::string combo_label = "PFB##selector"; - std::string preview = "PFB " + std::to_string(display_id); - if (ImGui::BeginCombo(combo_label.c_str(), preview.c_str())) { - for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { - if (!m_pfb_ptrs[i]) - continue; - bool selected = (i == m_selected_pfb_index); - std::string item = "PFB " + std::to_string(m_pfb_ptrs[i]->id()); - if (ImGui::Selectable(item.c_str(), &selected)) - m_selected_pfb_index = i; - if (selected) - ImGui::SetItemDefaultFocus(); - } - ImGui::EndCombo(); + if (m_selected_pfb_index >= 0 && + m_selected_pfb_index < static_cast(m_pfb_ptrs.size()) && + m_pfb_ptrs[m_selected_pfb_index]) + edit_engine = m_pfb_ptrs[m_selected_pfb_index]; + } + + ImGui::SeparatorText(labelForHit(Hit{hit.desc, edit_engine}).c_str()); + + if (hit.desc->type == "pfb" && !m_pfb_ptrs.empty()) { + std::string combo_label = "PFB##selector"; + std::string preview = "PFB " + std::to_string(edit_engine->id()); + if (ImGui::BeginCombo(combo_label.c_str(), preview.c_str())) { + for (int i = 0; i < static_cast(m_pfb_ptrs.size()); ++i) { + if (!m_pfb_ptrs[i]) + continue; + bool selected = (i == m_selected_pfb_index); + std::string item = "PFB " + std::to_string(m_pfb_ptrs[i]->id()); + if (ImGui::Selectable(item.c_str(), &selected)) + m_selected_pfb_index = i; + if (selected) + ImGui::SetItemDefaultFocus(); } + ImGui::EndCombo(); } } if (hit.desc->draw_inspector) - hit.desc->draw_inspector(*this, *hit.engine); + hit.desc->draw_inspector(*this, *edit_engine); if (m_param_edited && onParamChange) onParamChange(); diff --git a/tests/test_component_dispatch.cpp b/tests/test_component_dispatch.cpp index 92e4e83..dd8ed06 100644 --- a/tests/test_component_dispatch.cpp +++ b/tests/test_component_dispatch.cpp @@ -97,3 +97,22 @@ TEST_CASE_METHOD(ImGuiFixture, "Legacy .rfsim type strings still load (backward REQUIRE(app.testComponents().byType().size() == 1); std::remove(path); } + +TEST_CASE_METHOD(ImGuiFixture, + "Every registry descriptor has create and draw_inspector (issue #51)", + "[dispatch]") { + // Constructing the app runs InspectorPanel::registerDrawers(), which is + // the only place draw_inspector is populated; a forgotten branch would + // otherwise pass CI silently. + RfSimulatorApp app; + int next_id = 1; + for (const auto *d : ComponentTypeRegistry::instance().all()) { + CAPTURE(d->type); + REQUIRE(bool(d->create)); + REQUIRE(bool(d->draw_inspector)); + IComponentEngine *engine = + d->create(app.testComponents(), app.testGraphEngine(), next_id++); + REQUIRE(engine != nullptr); + REQUIRE(engine->type_name() == d->type); + } +}