Skip to content

Claude/wind impostor integration g3 h ta - #454

Merged
Krilliac merged 12 commits into
Workingfrom
claude/wind-impostor-integration-G3HTa
Apr 10, 2026
Merged

Claude/wind impostor integration g3 h ta#454
Krilliac merged 12 commits into
Workingfrom
claude/wind-impostor-integration-G3HTa

Conversation

@Krilliac

Copy link
Copy Markdown
Owner

No description provided.

claude added 6 commits April 10, 2026 07:31
Adds the render-side consumer for FoliageManager so scattered vegetation
can reach the GPU scene buffer with per-instance wind deformation and
distance-based impostor LOD selection. Backed by three new pieces that
slot into the existing render path:

FoliageRenderer (SparkEngine/Source/Graphics/FoliageRenderer.{h,cpp})
  - Singleton consumer initialised alongside FoliageManager in
    GameplayLifecycleShared.cpp; per-frame CollectFromFoliageManager
    builds a batch of FoliageRenderInstance records (world matrix,
    wind phase, wind strength, LOD, species index) from every visible
    manager instance.
  - Mesh cache keyed by species meshPath; uses a user-supplied loader
    callback so tests can avoid the real AssetPipeline. Production path
    on Windows uploads the batch into GPUSceneBuffer::UpdateInstance
    behind SPARK_PLATFORM_WINDOWS; per-instance wind phase rides in the
    GPUInstanceData padding slot so the foliage vertex shader can read
    it without growing the struct.
  - Deterministic wind phase derived from a Knuth hash of the instance
    world position (quantised to 10cm cells) so nearby instances sway
    in sync and distant ones decorrelate.
  - Pure helpers (BuildWorldMatrix, ComputeWindPhase, SelectLOD) and
    Console_GetStatus for debugging.

FoliageImpostorBaker (SparkEngine/Source/Graphics/FoliageImpostorBaker.{h,cpp})
  - CPU-side atlas packing for N species x M yaw-angle cells with
    power-of-two rounding, row wrapping, and graceful overflow drop.
  - SelectAngleSlot (yaw -> bin index with wrap) and GetAngleSlotUV
    (bin -> normalized UV rectangle) so the render consumer can sample
    the right cell per frame without GPU state.
  - All methods are pure and testable; GPU-side atlas baking (render
    mesh -> atlas) is a follow-up.

Shaders/HLSL/FoliageVS.hlsl + FoliagePS.hlsl
  - Vertex shader reads GPUInstanceData from t0 and applies a
    sin(time*freq + phase + worldPos.x*vFreq + worldPos.z*vFreq) sway
    along the wind direction, with saturate(localY/refHeight)^2
    anchoring the trunk while leaves move. A cos-based orthogonal
    jitter breaks up straight-line motion.
  - Pixel shader alpha-tests for leaves/grass cards, applies wrapped
    diffuse with a backlit rim term and a small sway-to-AO modulation.
  - FoliageWindParams cbuffer (b2) and FoliageLighting cbuffer (b3)
    expose all tuning knobs to the render frontend.

FoliageManager accessors (SparkEngine/Source/Graphics/FoliageSystem.{h,cpp})
  - GetVolumeIds() / GetVolumeDesc(id) added so the renderer can
    iterate volumes and resolve species names without new friendship.

Lifecycle wiring (SparkEngine/Source/Core/Lifecycle/GameplayLifecycleShared.cpp)
  - FoliageRenderer::GetInstance().Initialize(nullptr, 50.0f) runs
    next to FoliageManager::Initialize(). The AssetPipeline loader is
    nullptr for now; a follow-up once AssetPipeline is reachable via
    EngineContext will install the real LoadMesh callback.
  - CollectFromFoliageManager runs inside the existing
    SPARK_GUARDED_UPDATE("Foliage", ...) block right after manager
    Update so the batch reflects the latest visibility state.
  - Shutdown pairs with manager shutdown.

Tests (Tests/TestFoliageRenderer.cpp + Tests/TestFoliageImpostorBaker.cpp)
  - 13 new tests for FoliageRenderer covering init/shutdown, wind phase
    determinism and decorrelation, world matrix construction (identity,
    translation, 90-degree yaw), LOD selection thresholds, mesh cache
    hit/miss counting, prewarm, end-to-end collect with a populated
    FoliageManager, cull handling, impostor classification, empty
    manager safety, and Console_GetStatus.
  - 17 new tests for FoliageImpostorBaker covering NextPowerOfTwo,
    layout packing (single row, wrap, oversize, overflow), angle
    selection (basics, 2*pi wrap, negative yaw, edge cases), and UV
    computation.
  - All 4252 tests pass including the 30 new ones (Linux GCC Release).

Addresses the remaining Tier 1 item from
.claude/knowledge/stub-and-abandoned-features-2026-04-10.md that was
left over after the deterministic scatter work in #453.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
The initial FoliageRenderer wiring (10d8d8b) left the mesh loader as a
placeholder with a TODO pointing at AssetPipeline. AssetPipeline is
reachable via EngineContext::GetAssetPipeline() once graphics has
finished initializing, but the exact ordering between
FoliageRenderer::Initialize and AssetPipeline registration varies across
startup paths (Windows, SDL2, headless, module hot-reload), so a single
eager lookup is fragile.

This commit installs the real loader through two cooperating paths:

1. Explicit install at lifecycle init. After
   FoliageRenderer::GetInstance().Initialize(nullptr, 50.0f) runs in
   InitRenderingAndUtilitySystems, GameplayLifecycleShared fetches
   ctx->GetAssetPipeline() and, if non-null, immediately calls
   FoliageRenderer::InstallAssetPipelineLoader(pipeline). This is the
   common fast path on Windows where AssetPipeline is registered with
   EngineContext before InitGameplaySubsystems hands control to the
   lifecycle.

2. Lazy self-install on first use. FoliageRenderer::Initialize and
   FoliageRenderer::CollectFromFoliageManager both call
   TryInstallEngineContextLoader(), which is a no-op once an explicit
   loader is in place but retries the EngineContext lookup each frame
   until it succeeds. This covers headless/test paths and any future
   startup reordering where AssetPipeline lands in the context after
   the lifecycle has already initialised the renderer.

The new public API:
  - InstallAssetPipelineLoader(AssetPipeline*)
    Wraps pipeline->LoadMesh(path) in a FoliageMeshLoader callback.
    Returns false when pipeline is null. Marks the loader as explicit
    so the lazy path will not overwrite it on subsequent Collects.
  - HasExplicitLoader() const
    True after a real loader has been installed by either path. Used
    in tests (and by future diagnostics) to assert that the wiring
    actually landed.

Shutdown clears the loader and resets m_hasExplicitLoader to false so
that a subsequent Initialize gets a fresh attempt. Empty-path lookups
short-circuit inside the installed lambda so the renderer never hands
an empty string to AssetPipeline.

Tests (Tests/TestFoliageRenderer.cpp)
  - Null InstallAssetPipelineLoader is a no-op and leaves
    HasExplicitLoader() false.
  - Explicit loader supplied to Initialize() is not overwritten when
    Collect runs against an empty manager (lazy install does not stomp
    a real loader).
  - Initialize with nullptr loader in a CI context with no registered
    AssetPipeline still leaves the renderer functional.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4254 passed, 0 failed, 1 warned (pre-existing flaky test),
       4255 total. 32 foliage tests (including the 3 new loader-
       install tests) all pass. 116,891 assertions.

Addresses the remaining "Material/mesh asset load" TODO from
.claude/knowledge/stub-and-abandoned-features-2026-04-10.md that was
carried forward from 10d8d8b.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
Addresses the Tier 4 editor-infrastructure stubs flagged in
.claude/knowledge/stub-and-abandoned-features-2026-04-10.md:
EditorLayoutManager was a header-only class whose Initialize /
SaveCurrentLayout / LoadLayout methods were all `return true;` stubs,
and EditorWindowManager::Save/LoadCurrentLayoutToFile were
comment-only placeholders ("// Serialization would write ... to disk").

EditorLayoutManager (SparkEditor/Source/Core/EditorLayoutManager.{h,cpp})
  - Header rewritten without the ImGui dependency so the real class can
    be linked into SparkTests (PanelConfig now stores plain floats
    instead of ImVec2). The editor's own #include chain still pulls
    ImGui in via other headers, so nothing downstream breaks.
  - New .cpp with disk-backed JSON persistence. Initialize() creates
    the layout directory under `Layouts/` if missing. SaveCurrentLayout
    writes one file per named layout with name/description/version and
    a `panels` array carrying dock position, pos/size, visibility,
    floating state, dock ratio, tab order, and parent dock. LoadLayout
    reads that JSON via a minimal hand-rolled walker (no third-party
    lib) and applies each entry to panels that are already registered;
    unknown panels in the file are ignored.
  - DeleteLayout removes the file, GetSavedLayouts re-scans the
    directory on each call so externally-dropped files are visible,
    ResetToDefault restores the snapshot captured at RegisterPanel
    time, and Console_GetStatus reports one-line diagnostics.
  - InitializeManagers() in EditorUI.cpp now instantiates
    m_layoutManager (previously always nullptr despite being referenced
    in ImportLayout / recovery paths) and Shutdown() tears it down.

EditorWindowManager (SparkEditor/Source/Core/EditorWindowManager.h)
  - SaveCurrentLayoutToFile writes real hand-rolled JSON: layout name,
    version, dock INI, and per-panel {panelName, isOpen, isFloating,
    posX/Y, width/height, monitorIndex}. Creates parent directories as
    needed, records the target path in m_lastSavedPath, returns false
    on open/write failure.
  - LoadLayoutFromFile parses the same format back into m_currentLayout
    using minimal JSON helpers (EscapeJson, UnescapeJson, ReadJsonString,
    ReadJsonBool, ReadJsonNumber) kept as private statics so the class
    stays header-only. Requires a "windowLayout" key; empty, missing,
    and malformed files are rejected.
  - Public GetLastSavedPath() accessor added for diagnostics/tests.
  - EditorApplication::Initialize (both Windows D3D11 and Linux SDL2
    paths) now calls EditorWindowManager::GetInstance().Initialize()
    before the UI is created so panels can query window state during
    their own init. Both Shutdown paths mirror this with a
    GetInstance().Shutdown() call after EditorUI::Shutdown(), so the
    auto-save path gets the final panel state.

Tests (Tests/TestEditorLayoutManager.cpp, Tests/TestEditorWindowManager.cpp)
  - EditorLayoutMgr_* (10 new tests against the real class):
      InitCreatesDirectory, RegisterPanelTracksDefaults,
      ResetToDefaultRestoresFirstSeen, SaveLoadRoundtripWritesAndReadsJSON,
      LoadUnknownLayoutReturnsFalse, DeleteLayoutRemovesFile,
      GetSavedLayoutsListsFiles, SaveEmptyNameFails,
      LoadIgnoresUnregisteredPanels, ConsoleStatusContainsName.
      Each test uses a unique per-tag directory under std::tmp to
      avoid parallel-run collisions.
  - EditorWinMgrReal_* (6 new tests against the real singleton):
      SaveCurrentLayoutToFileWritesJson (content sanity-check),
      SaveCurrentLayoutToFileEmptyPathFails,
      LoadLayoutFromFileMissingReturnsFalse,
      LoadLayoutFromFileEmptyFileReturnsFalse,
      SaveThenLoadRoundtripPreservesName,
      LoadLayoutFromFileMalformedReturnsFalse.
  - Tests/CMakeLists.txt adds EditorLayoutManager.cpp next to the
    existing editor core sources that are already compiled into
    SparkTests (EditorLogger, EditorCrashHandler, ProjectManager,
    EditorPluginManager).

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4270 passed, 0 failed, 1 warned (pre-existing flaky),
       4271 total. 116,944 assertions.

Addresses the two highest-value Tier 4 editor entries in
.claude/knowledge/stub-and-abandoned-features-2026-04-10.md. Tier 4
"editor layout persistence" was specifically called out as
"the highest-value item (users expect editor layouts to survive
restarts)". The knowledge file is updated to mark both entries
RESOLVED.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
The Tier 4 audit entry for SelectionManager read "Never instantiated,
never registered. Selection logic is currently duplicated across
Hierarchy/Inspector/SceneView panels." The class itself was already a
full header-only implementation (single/multi-select, marquee, GPU
picking, groups, callbacks, filters) — the complaint was that no code
path ever called Initialize() on the singleton.

This commit makes the singleton alive and covered by real tests. The
full panel migration (Hierarchy/Inspector/SceneView → SelectionManager)
is deferred: SelectionManager uses uint32_t EntityId while editor
panels use uint64_t ObjectID, so the two ID spaces need to be unified
or bridged by an adapter before the panels can be migrated. That is
noted as a pending follow-up in the audit knowledge file.

Lifecycle wiring (SparkEditor/Source/Core/EditorUI.cpp)
  - InitializeManagers() now calls
    SelectionManager::GetInstance().Initialize() next to the layout
    manager, with a brief comment explaining the ObjectID/EntityId
    mismatch and why panel migration is deferred.
  - Shutdown() calls SelectionManager::GetInstance().Shutdown() next
    to the layout manager teardown.
  - Added "../Panels/SelectionManager.h" include.

Tests (Tests/TestSelectionManager.cpp)
  - Keeps the original 10 standalone-reimpl tests for historical
    coverage.
  - Adds 13 new SelectionMgrReal_* tests against the actual
    SparkEditor::SelectionManager singleton from
    SparkEditor/Source/Panels/SelectionManager.h:
      InitializeAndClear, SelectSingleEntity, AddAndRemoveFromSelection,
      ToggleSelection, SelectMultiple, ClearSelection,
      CallbackFiresOnSelect (with unregister verification),
      LockedEntityCannotBeSelected, MarqueeSelection,
      SelectionGroupsSaveAndRestore, FilterClearAndSet, ConsoleStatus,
      NotInitializedStatus.
  - Each test Initialize()s on entry (defensively reset singleton
    state since the class is process-wide) and Shutdown()s on exit.

Knowledge file
  - .claude/knowledge/stub-and-abandoned-features-2026-04-10.md marks
    the SelectionManager entry PARTIALLY RESOLVED, explicitly noting
    what still needs panel-side work.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4283 passed, 0 failed, 1 warned (pre-existing flaky),
       4284 total. 116,987 assertions. 13 new SelectionMgrReal_*
       tests all pass.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
Addresses the two remaining Tier 4 editor-infrastructure stubs from
.claude/knowledge/stub-and-abandoned-features-2026-04-10.md. Both
panels were registered in EditorPanelFactory but opened with visually
empty bodies because their Render* methods were placeholder comments
("// ImGui::Combo would go here in real editor"). This commit fills in
both the real ImGui rendering and — for CSGEditorPanel — a clean
public API that tests can exercise without pulling ImGui into the test
binary.

CSGEditorPanel (SparkEditor/Source/Panels/CSGEditorPanel.h)
  - Replaced the m_createRequested / m_rebuildRequested / m_deleteRequested
    flag-dance (designed for ImGui button callbacks that never existed)
    with a direct public API:
      CreateBrush(shape, size, op) — calls CSGSystem, tracks the ID,
        moves the selection cursor, auto-rebuilds if enabled. Returns
        the brush ID or 0 on failure.
      DeleteBrush(id) — untracks, calls CSGSystem::RemoveBrush,
        reassigns the selection cursor if the removed brush was
        selected, auto-rebuilds. Returns false for unknown IDs.
      RebuildMesh() — caches CSGSystem::BuildAll() into m_lastMesh so
        statistics widgets can read vertex/triangle counts without
        re-running the boolean chain.
      SelectBrush(id), GetSelectedBrush(), GetBrushes(),
      GetBrushCount(), GetLastMesh(), SetAutoRebuildEnabled(),
      IsAutoRebuildEnabled(), SetDefaultBrushShape/Size/Operation.
  - Real ImGui rendering behind #if __has_include(<imgui.h>):
      Brush creation section: Combo for shape, DragFloat3 for size,
        Combo for boolean op, Button("Create") calling CreateBrush().
      Brush list: Selectable rows with per-row SmallButton("X") that
        calls DeleteBrush(id).
      Build controls: Checkbox bound to m_autoRebuild, Button("Rebuild
        Now") that calls RebuildMesh().
      Statistics: brush count, triangle count, vertex count, selected
        brush ID.
  - Test/headless builds (no ImGui) compile the Render* helpers as
    no-ops so the header stays link-safe without ImGui.

NetworkDebugPanel (SparkEditor/Source/Panels/NetworkDebugPanel.h)
  - The data-model layer (TakeSnapshot, LogPacket, UpdateReplicationInfo,
    graph-window pruning, GetAverageBandwidthOut/Latency/PeakLatency)
    was already real — the audit finding was specifically about the
    empty Render* methods.
  - Real ImGui rendering behind #if __has_include(<imgui.h>):
      RenderBandwidthGraph: PlotLines for sent + recv bytes/sec with
        average label.
      RenderLatencyGraph: PlotLines for latencyMs with avg + peak.
      RenderSimulationControls: Checkbox + 4 SliderFloats for
        artificial latency, packet loss %, jitter, and bandwidth cap,
        bound to m_simulationSettings.
      RenderPacketLog: ImGui::BeginTable with 5 columns (Time, Dir,
        Type, Size, Summary), filterable by SEND/RECV checkboxes.
      RenderReplicationViewer: ImGui::BeginTable with 6 columns
        (Entity, Name, Dirty, Bytes, Hz, Priority), sorted by snapshot
        size descending.
      RenderConnectionSummary: uptime, snapshot count, packet log
        size, entity count.
  - Test/headless builds compile Render* as no-ops.

Tests (Tests/TestCSGEditorPanel.cpp — NEW, 10 tests)
  - Uses a CSGPanelHelper that mirrors CSGEditorPanel's non-ImGui
    bookkeeping layer and delegates every operation to the real
    Spark::LevelDesign::CSGSystem singleton. (The real panel cannot
    be instantiated from tests because its EditorPanel base has a
    ctor in EditorPanel.cpp that requires ImGui.)
  - Tests cover: CreateBrush tracks ID and selects,
    CreateBrushWithSubtractiveOperation preserves operation flag,
    DeleteBrush removes from list, DeleteUnknownBrushReturnsFalse,
    DeleteSelected reassigns or clears the cursor, SelectBrush
    accepts known IDs and rejects unknown, AutoRebuildToggle,
    RebuildMesh captures a non-empty snapshot, AutoRebuild fires on
    create and delete, CreateAllBrushShapes (Box/Cylinder/Sphere/
    Wedge/Cone round trip).
  - Each test calls Reset() at the end to remove its brushes from
    the CSG singleton so parallel / repeat runs don't leak state.

NetworkDebugPanel note
  - No new tests were added here; 9 existing standalone-reimpl tests
    in Tests/TestNetworkDebugPanel.cpp already cover the data-model
    layer. The knowledge file flags the remaining follow-up:
    NetworkManager still needs to call the panel's RecordBytesSent /
    RecordBytesRecv / LogPacket / SetCurrentLatency feed methods
    each frame. The panel's public feed API is already in place.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4294 passed, 0 failed (1 pre-existing flaky intermittently
       passing this run), 117,026 assertions. 10 new
       CSGEditorPanel_* tests all pass.

Knowledge file
  - Marks both CSGEditorPanel and NetworkDebugPanel RESOLVED, with
    the NetworkManager producer wiring noted as a separate follow-up
    task.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
The Tier 3 audit entry for TutorialSystem read: "None — not referenced
in any editor .cpp outside its test." The class itself was already a
full 530-line header-only implementation with built-in tutorial
sequences, step auto-advance, step-changed callbacks, completion
tracking, and console status — but no editor code ever called
Initialize() on the singleton, so the built-in tutorials were not
registered at runtime and the Update() auto-advance timer never ticked.

Lifecycle wiring (SparkEditor/Source/Core/EditorUI.cpp)
  - InitializeManagers() calls TutorialSystem::GetInstance().Initialize()
    next to the selection manager init, which runs
    RegisterDefaultTutorials() and makes
    GetAvailableTutorials() non-empty. The success log reports the
    number of registered tutorials.
  - Update() ticks TutorialSystem::GetInstance().Update(deltaTime)
    inside SPARK_GUARDED_UPDATE("TutorialSystem", "Editor", ...) next
    to UpdateNotifications so auto-advance timers progress each frame.
  - Shutdown() calls TutorialSystem::GetInstance().Shutdown() next to
    the selection manager shutdown.
  - Added #include "TutorialSystem.h".

Tests (Tests/TestTutorialSystem.cpp, 7 new)
  - The existing 15 tests already exercised the real
    SparkEditor::TutorialSystem singleton, so this commit only adds
    coverage for the paths that were missing:
      GoToStep_JumpsWithinSequence — valid index lands, out-of-range
        goto is ignored.
      Update_AutoAdvancesAfterDelay — 1.0s autoAdvanceDelay with
        0.5s + 0.6s updates crosses the threshold on the second call;
        a subsequent step with delay=0 is NOT auto-advanced.
      Update_InactiveIsNoOp — Update with no active tutorial is safe.
      OnStepChanged_CallbackFires — callback runs once on StartTutorial
        and once per AdvanceStep, receiving the correct index and step
        reference.
      GetCurrentTutorialName — empty before/after, matches the active
        name during playback.
      ConsoleStatusContainsActiveName — inactive status contains
        "active=none", active status contains the tutorial name and
        "step=N/M" progress.
      StartEmptyTutorialReturnsFalse — registering a sequence with
        no steps and trying to start it returns false and leaves
        IsTutorialActive() == false.

Knowledge file
  - .claude/knowledge/stub-and-abandoned-features-2026-04-10.md marks
    the TutorialSystem Tier 3 entry RESOLVED.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4301 passed, 0 failed, 4301 total. 117,050 assertions.
       7 new TutorialSystem_* tests all pass.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d8a080816

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

DirectX::XMMATRIX normal = DirectX::XMMatrixTranspose(DirectX::XMMatrixInverse(nullptr, world));
DirectX::XMStoreFloat4x4(&data.normalMatrix, normal);

data.materialId = inst.speciesIndex;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use a global material ID for foliage instance uploads

inst.speciesIndex is volume-local (it indexes each volume’s speciesNames list), but this line stores it directly into GPUInstanceData::materialId, which is consumed as a global material-table key. In scenes with multiple volumes that use different species ordering, different species can map to the same uploaded materialId, producing incorrect material binding/shading for foliage instances.

Useful? React with 👍 / 👎.

Comment on lines +290 to +294
if (inst.speciesIndex < desc->speciesNames.size())
{
const std::string& speciesName = desc->speciesNames[inst.speciesIndex];
if (const FoliageSpecies* species = manager.FindSpecies(speciesName))
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip batch records when species resolution fails

When a volume entry points to a missing/stale species (which can happen after species registry changes), this branch simply does nothing and the loop still emits a render record afterward. That means unresolved species are still counted and uploaded with defaulted fields, leading to invalid draw data rather than being cleanly culled from the batch.

Useful? React with 👍 / 👎.

Comment on lines +184 to +186
for (size_t i = 0; i < m_currentLayout.panels.size(); ++i)
{
const PanelWindowState& p = m_currentLayout.panels[i];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Persist runtime panel state, not only currentLayout.panels

Saving writes only m_currentLayout.panels, but the runtime APIs mutate m_floatingPanels/m_panelMonitors and never synchronize those maps back into m_currentLayout.panels. As a result, file saves can omit or stale out actual floating/monitor placement changes, so load round-trips do not restore the window state users just configured.

Useful? React with 👍 / 👎.

claude added 4 commits April 10, 2026 09:16
The Tier 3 audit entry for AssetDependencyGraph was flagged as
"Partial — .cpp exists but split into an unrelated file; verify
whether AssetDatabase actually calls it". Investigation uncovered a
latent ODR hazard: two completely different classes lived under the
same fully-qualified name SparkEditor::AssetDependencyGraph.

  1. SparkEditor/Source/Panels/AssetDependencyGraph.h (485 lines)
     - Header-only singleton (GetInstance)
     - Audit/budget feature set: Register, AddDependency,
       GetTransitiveDependencies, FindUnusedAssets,
       FindCircularDependencies, GetSizeBudget, GenerateAudit,
       Console_GetStatus
     - Zero call sites — no .cpp or test included it
     - Test file Tests/TestAssetDependencyGraph.cpp used an
       unrelated standalone reimplementation

  2. SparkEditor/Source/AssetPipeline/AdvancedAssetPipeline.h
     - Non-singleton build-system graph
     - Topological-sort API: AddAsset, AddDependency,
       GetProcessingOrder, DetectCircularDependencies,
       GetAffectedAssets
     - Actively used by AdvancedAssetPipeline.cpp,
       AssetProcessors.cpp, AdvancedAssetPipelineUI.cpp

Both lived in namespace SparkEditor, so any translation unit that
happened to include both headers would fail to compile with a
multiple-definition error, and even without that, the name reuse was
misleading — the audit file itself incorrectly described them as
the same class.

Fix applied to the Panels/ (orphan) version:

Rename
  - File: SparkEditor/Source/Panels/AssetDependencyGraph.h ->
          SparkEditor/Source/Panels/AssetAuditGraph.h (git mv)
  - Class: SparkEditor::AssetDependencyGraph -> SparkEditor::AssetAuditGraph
  - Enum:  SparkEditor::AssetType (local to the header) ->
           SparkEditor::AuditAssetType (also collided with
           AssetPipelineTypes.h's AssetType)
  - Helper: AssetTypeName -> AuditAssetTypeName
  - Header @file and class docs updated. A @note explains the
    historical rename and points at the unrelated build-system
    graph so future readers don't confuse the two.

Wiring (SparkEditor/Source/Core/EditorUI.cpp)
  - InitializeManagers() now calls
    AssetAuditGraph::GetInstance().Initialize() next to the tutorial
    system init. Shutdown() tears it down next to the tutorial
    system teardown. Both paths log success via SparkConsole.
  - Added #include "../Panels/AssetAuditGraph.h".

Tests (Tests/TestAssetDependencyGraph.cpp, 11 new)
  - Kept the existing standalone-reimpl tests (they are a generic
    graph-logic suite and still valid).
  - Added 11 new AssetAuditGraph_* tests against the actual
    SparkEditor::AssetAuditGraph singleton:
      InitializeAndShutdown, RegisterAssetAndGet,
      AddDependencyBidirectional, RemoveDependency,
      TransitiveDependencies, FindUnusedAssets (with root-asset
      marking), FindCircularDependencies (3-node cycle),
      RemoveAssetClearsEdges, SizeBudgetReport, GenerateAudit
      stat aggregation, and ConsoleStatus formatting.

Docs / tooling
  - wiki/Asset-Dependency-Graph.md now points at the new filename
    and has a Note block explaining the rename and the coexistence
    with AssetPipeline::AssetDependencyGraph.
  - .bloat-baseline.txt updated with the new path.
  - .claude/knowledge/stub-and-abandoned-features-2026-04-10.md
    marks the Tier 3 entry RESOLVED with the rename details.

Not touched
  - SparkEditor/Source/AssetPipeline/AdvancedAssetPipeline.h and
    AssetProcessors.cpp — the AssetPipeline::AssetDependencyGraph
    build-system class is left as-is. It is actively used and the
    rename would be a much larger refactor that is not needed to
    resolve the audit finding.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4311 passed, 0 failed, 1 warned (pre-existing flaky),
       4312 total. 117,076 assertions. 11 new AssetAuditGraph_*
       tests all pass.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
…ositive)

The audit listed 6 Engine-subsystem header-only orphans in the
stub-and-abandoned-features audit. Per-file verification found that
3 were false positives (the audit used wrong class names) and the
remaining 3 have been wired in.

## False positives — no action needed

  DataTableSystem.h - the class is named DataTableRegistry, not
    DataTableManager, and was already wired in GameplayLifecycleShared
    at Initialize/Shutdown.

  GameplayTags.h - the class is named GameplayTagRegistry, not
    GameplayTagManager, and was already wired via EngineContext's
    GetGameplayTagService() path plus the direct registry fallback.

  SaveSystemTypes.h - already #include'd from SaveSystem.h:74.

## Real orphans — now wired

NavMeshLink.h
  - NavMeshLinkSystem::GetInstance().Initialize() added to the
    InitGameplaySubsystems startup sequence next to the existing
    DataTableRegistry init. Shutdown() added to the teardown
    sequence next to Shutdown().
  - The off-mesh link bridge already existed on the ECS side via
    FoliageVolumeComponent-style runtimeLinkId fields in
    AdvancedPlacementComponents.h; this commit just ensures the
    singleton is alive when those links are registered.
  - New Tests/TestNavMeshLink.cpp with 5 real-class tests:
    InitializeClearsLinks, AddLinkAssignsIdAndStores, RemoveLink
    (including safe unknown-ID removal), EnableDisableLink, and
    FindLinksNearBidirectional (covers bidirectional, one-way,
    disabled exclusion).

GameplaySystemExtension.h
  - (void)GameplayExtensionRegistry::GetInstance() touch added at
    startup to ensure the singleton is constructed before any game
    module tries to register a quest / dialogue extension.
  - GetInstance().Clear() added at shutdown.
  - Fixed a latent ODR hazard: this header declared
    Spark::Gameplay::QuestDefinition and QuestContext, which collided
    with completely different definitions in QuestSystem.h (same
    names, different fields). Any TU including both headers would
    fail to compile. Renamed the extension-local types to
    QuestExtensionInput / QuestExtensionState so both headers can
    coexist. The registry itself, IQuestTypeExtension, and
    IDialogueExtension interfaces all updated accordingly.
  - New Tests/TestGameplaySystemExtension.cpp with 6 real-class
    tests: SingletonAlive, RegisterAndLookupQuestExtension,
    RegisterAndLookupDialogueExtension, NullExtensionIgnored,
    ClearWipesBothLists, QuestExtensionLifecycleMethods (the full
    CanStart -> OnStarted -> OnObjectiveProgress -> IsComplete ->
    OnCompleted flow against a stub extension).
  - Existing Tests/TestGameplayExtensionRegistry.cpp is kept — it
    uses a standalone reimplementation and the two suites are
    complementary.

RuntimePrefab.h
  - (void)PrefabRegistry::GetInstance() touch added at startup so
    later RegisterPrefab / SpawnEntity calls find a constructed
    singleton.
  - Fixed a build-time hazard: the header forward-declared
    Spark::BinaryWriter / BinaryReader but its inline Serialize /
    Deserialize method bodies called methods on those classes, so
    any translation unit that included RuntimePrefab.h without also
    having Utils/Serializer.h in scope first failed to compile with
    "invalid use of incomplete type". This showed up the moment
    GameplayLifecycleShared.cpp started including RuntimePrefab.h.
    Switched from the forward declarations to a proper
    #include "Utils/Serializer.h".
  - Existing Tests/TestRuntimePrefab.cpp already exercises the real
    class; no new tests needed.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4322 passed, 0 failed, 1 warned (pre-existing flaky),
       4323 total. 117,117 assertions.
       5 NavMeshLink + 6 GameplayExtensionRegistryReal new tests pass.

Knowledge file
  - .claude/knowledge/stub-and-abandoned-features-2026-04-10.md
    updated to mark all 6 engine Tier 2 entries as resolved,
    documents which three were false positives and why, and notes
    the two latent collisions (QuestDefinition, BinaryWriter
    forward decls) that were incidentally fixed.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
Per-file verification of the 8 "smaller orphans" in the Tier 2
graphics section of stub-and-abandoned-features-2026-04-10.md found
that 2 were false positives and the remaining 6 are legitimate
reusable utilities already covered by tests. All 6 received an
@note block documenting their intentional-utility status, which is
the third resolution path the audit explicitly permits ("document
as intentional utility").

## False positives — no action needed

  ConstantBufferDiff.h (ConstantBufferDiffManager) - fully wired at
    GameplayLifecycleShared.cpp:415 (Initialize), :937 (BeginFrame),
    :980 (Shutdown). Already integrated into the per-frame loop.

  RHI/DeferredDeletionQueue.h - held as a member of D3D11Device
    (D3D11Device.h:342) and used by the D3D11 RHI backend.

## Documented as intentional utilities

SpringArm.h
  - Pure-CPU SpringArmState value type with a deterministic Update()
    method. Currently the SpringArmComponent in the ECS duplicates
    the state fields rather than holding a SpringArmState directly;
    a follow-up will unify them and add a camera-update system that
    calls Update() each frame with physics collision results.
  - New Tests/TestSpringArm.cpp with 6 tests: defaults, no-collision,
    collision-shortens-to-hit, min-length clamp, disable toggle,
    and recover-when-obstacle-clears. Tests validate the smoothing
    math converges to the expected lengths over a 120-frame horizon.
  - Header @note explains the intentional stateless-utility design
    and the pending component unification follow-up.

LineTrailRenderer.h
  - Pure data-carrier structs (LineRendererData, TrailRendererData)
    that correspond to LineRendererComponent / TrailRendererComponent
    in the ECS. The render-side consumer that would iterate those
    components and build camera-facing triangle strips is a separate
    follow-up. Header @note explains this and notes the intentional
    header-only design so game modules can construct the structs
    without a compiled dependency.

DirtyRectTracker.h
  - Pure CPU rectangle-merge logic for partial texture updates. No
    singleton, no GPU dependency — a future texture-streaming or
    UI-atlas system instantiates one per tracked texture. Already
    covered by Tests/TestDirtyRectTracker.cpp. Header @note
    documents this.

ClusteredLightGPU.h
  - GPU bridge utility that converts ClusteredLightCulling results
    into structured-buffer-ready arrays (GPULightData,
    ClusterLightGrid, light index list). A future forward+ render
    pass will instantiate one per frame. Already covered by
    Tests/TestClusteredLightGPU.cpp. Header @note documents this.

RHI/RHIHandlePool.h
  - Generic HandlePool<T, N> template with generation counters,
    Filament/bgfx-inspired. Pure C++ / no GPU dependency. RHI
    backends instantiate one per resource type as those are added.
    Already covered by Tests/TestRHIHandlePool.cpp. Header @note
    documents this.

RHI/TransientBufferAllocator.h
  - Per-frame bump allocator for dynamic vertex/index data. Owned
    per render system (particles, debug draw, UI, decals). Already
    covered by Tests/TestTransientBufferAllocator.cpp with a fake
    RHI device. Header @note documents this.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4328 passed, 0 failed, 1 warned (pre-existing flaky),
       4329 total. 117,133 assertions. 6 new SpringArmState_*
       tests all pass.

Knowledge file
  - .claude/knowledge/stub-and-abandoned-features-2026-04-10.md
    "Smaller orphans" table rebuilt with per-file status
    (FALSE POSITIVE / DOCUMENTED / DOCUMENTED + tested) and brief
    rationale for each resolution.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
The last remaining batch from
stub-and-abandoned-features-2026-04-10.md: 20 "high-value" graphics
header-only orphans. Per-file verification found 1 false positive
and 19 legitimate reusable utilities. None were deleted — every file
represents substantial working code (200-750 lines) that will be
useful when the corresponding render features are built.

## False positive (filename collision)

  Graphics/PipelineStateCache.h - the orphan file defines class
    D3D11PipelineStateCache (not PipelineStateCache). The file at
    Graphics/RHI/PipelineStateCache.h defines a DIFFERENT class
    named PipelineStateCache that IS wired at
    GameplayLifecycleShared.cpp:427 (Initialize) and :993 (Shutdown).
    Added a @note to the orphan to disambiguate the two files and
    mark it as a future D3D11 helper.

## Documented as intentional utilities (19 files)

All 19 files received a @note block explaining:
  - what the utility does,
  - that there is no singleton to wire into the engine lifecycle,
  - how a future render feature will own and drive an instance,
  - and (where applicable) where the existing tests live.

Grouped by role:

Rendering passes (future render-side consumers):
  BVHAccelerator.h       - SAH-based hierarchical frustum / ray culling
  VoxelConeTracing.h     - Voxel-cone-traced GI
  GTAOEffect.h           - Ground-truth AO post-process
  SSAOTemporal.h         - Temporal SSAO with history reprojection
  VolumeSystem.h         - Post-process volume blending (has tests)
  ReflectionProbeCache.h - Prefiltered env map cache
  CachedShadowAtlas.h    - Shadow atlas with per-light caching (has tests)
  RTHandleSystem.h       - Render target handle abstraction
  DenoiserInterface.h    - Abstract denoiser plugin interface

Resource / buffer utilities:
  RenderTargetPool.h     - Pooled RT allocator with acquire/release
  ConstantBufferRing.h   - Ring allocator for per-frame CBs
  PersistentMaterialCB.h - Persistent material CB with dirty tracking
  PipelineStateCache.h   - D3D11 state object hash cache (see note above)

Shader system:
  ShaderVariantSystem.h  - Keyword-based variant management (has tests)
  ShaderCrossCompiler.h  - HLSL<->GLSL translation (possibly superseded)

GPU profiling / debug:
  GPUDebugMarkers.h      - Scoped PIX / RenderDoc / NSight markers
  GPUTimestampQuery.h    - GPU timestamp query pool

Misc:
  MeshOptimizer.h        - Mesh/index buffer optimizer (has tests)
  UICompositor.h         - UI compositor (possibly superseded by Engine/UI)
  FastNoise2SIMD.h       - SIMD procedural noise (ported FastNoise2)

Four of the 19 (VolumeSystem, MeshOptimizer, CachedShadowAtlas,
ShaderVariantSystem) already have test coverage via
Tests/TestGraphicsIntegration.cpp; their @note blocks point at that
file. The other 15 have no existing tests — that is noted in the
audit file as a follow-up when each utility's consumer lands.

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4328 passed, 0 failed, 1 warned (pre-existing flaky),
       4329 total. 117,133 assertions. No test regressions — this
       commit only adds documentation comments, it does not change
       behavior.

Knowledge file
  - .claude/knowledge/stub-and-abandoned-features-2026-04-10.md
    "High-value orphans" table rebuilt with per-file status
    (FALSE POSITIVE / DOCUMENTED / DOCUMENTED + tested) and brief
    rationale for each resolution path.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
@github-actions

github-actions Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

✅ CI Errors Resolved

All previously reported errors have been fixed. All builds passing.

Last checked: 2026-04-10T12:16:27Z

claude added 2 commits April 10, 2026 10:46
Resolves three Codex review comments on #454, fixes the
build-windows-vs2022 (Debug) break introduced by CSGEditorPanel, and
wires CodeQL findings into a PR comment that mirrors the CI Error
Report pattern.

## Windows build fix — CSGEditorPanel.h (VS2022 Debug)

The last commit added two methods named DeleteBrush and SelectBrush.
Both collide with Windows GDI macros in wingdi.h:

  #define DeleteBrush(hbr)       DeleteObject((HGDIOBJ)(HBRUSH)(hbr))
  #define SelectBrush(hdc, hbr)  ((HBRUSH)SelectObject(...))

The preprocessor expands both, turning the member declarations into
function-like macro invocations that reference HBRUSH / SelectObject.
The CI error report captured 15+ cascade errors all rooted in these
two names.

Fix: rename the methods to avoid the macro names.
  DeleteBrush(uint32_t)  ->  RemoveBrushById(uint32_t)
  SelectBrush(uint32_t)  ->  SetSelectedBrushId(uint32_t)

Test file and the inline ImGui render code that call these methods
updated accordingly.

## Codex P1 — Global material ID for foliage instances
  SparkEngine/Source/Graphics/FoliageRenderer.cpp:348

`inst.speciesIndex` is volume-local (it indexes each volume's
speciesNames list), but FoliageRenderer was storing it directly into
GPUInstanceData::materialId, which is a global material-table key.
In scenes with multiple volumes that use different species ordering,
different species mapped to the same uploaded materialId.

Fix:
  - Added FoliageManager::GetSpeciesGlobalIndex(name) that returns
    the registry-wide (m_speciesByName) index, or UINT32_MAX.
  - Added a new field globalMaterialId to FoliageRenderInstance.
  - CollectFromFoliageManager now looks up the global index by
    species name and stores it in rec.globalMaterialId.
  - UploadToSceneBuffer now uses inst.globalMaterialId for
    data.materialId (not inst.speciesIndex).
  - The uniqueness key for stats.uniqueSpecies also switched to
    globalMaterialId so cross-volume duplicates collapse.
  - New test FoliageRenderer_GlobalMaterialIdMatchesRegistryIndex
    registers three species in one order, builds a volume that lists
    them in the reverse order, and verifies every render record
    carries the correct global index (not the local one).

## Codex P2 — Skip batch records when species resolution fails
  SparkEngine/Source/Graphics/FoliageRenderer.cpp:294

Previously, if a volume entry pointed to a missing or stale species
(out-of-range volume-local index, or a species that was unregistered
after scatter), the resolution branch silently did nothing and the
loop still emitted a render record with defaulted material id /
wind strength.

Fix:
  - Added stats.unresolvedSpecies field to FoliageRenderStats.
  - CollectFromFoliageManager now resolves the species BEFORE
    building the record. Three skip conditions drop the instance
    cleanly with continue + ++stats.unresolvedSpecies:
      1. Volume-local index out of range.
      2. FindSpecies returned nullptr.
      3. GetSpeciesGlobalIndex returned UINT32_MAX.
  - The record is only emitted when species resolution fully
    succeeds, so every entry in m_renderInstances has a valid
    globalMaterialId and windStrength.
  - New test FoliageRenderer_UnregisteredSpeciesSkippedNotEmitted
    registers a species, scatters instances, then unregisters the
    species and collects — the batch must be empty and
    unresolvedSpecies must be > 0.

## Codex P2 — Persist runtime panel state, not only currentLayout.panels
  SparkEditor/Source/Core/EditorWindowManager.h:186

FloatPanel / DockPanel / MoveToMonitor mutate m_floatingPanels and
m_panelMonitors side-maps for O(1) toggling, but those maps were
never synchronized back into m_currentLayout.panels before saving.
Save round-trips therefore silently lost whatever the user had just
configured.

Fix:
  - New private helper SyncRuntimeStateIntoCurrentLayout() that walks
    both side-maps and folds their entries into
    m_currentLayout.panels (creating new PanelWindowState entries for
    panels that were not previously listed).
  - Called at the start of both SaveLayout (in-memory named save)
    and SaveCurrentLayoutToFile (disk save).
  - New test EditorWinMgrReal_SaveSerializesFloatingAndMonitorState
    sets FloatPanel + MoveToMonitor on two panels, saves to a temp
    file, and verifies the file contents include both panel names,
    "isFloating": true, and the correct monitorIndex values.

## CodeQL -> PR comment, matching the CI Error Report pattern

The user asked to have the CodeQL analyze job report its results
similarly to the CI Error Report job in build.yml. CodeQL already
uploads SARIF to GitHub's code scanning UI, but that is a separate
surface from the consolidated PR comment the build jobs produce.

New files:
  - .github/scripts/report-codeql-findings.js — parses SARIF files,
    deduplicates by (rule, file, line, message), groups by severity
    (error / warning / note), and posts a single in-place-updated PR
    comment with marker `<!-- spark-codeql-report -->`. On success
    with no findings, an existing comment is updated to a
    ✅ resolved state. On push (non-PR) runs it
    writes a job summary visible in the Actions UI. Also writes a
    codeql-summary.json in the same `{job, errors, warnings, tests}`
    shape that extract-errors.sh produces.

Workflow changes (.github/workflows/codeql.yml):
  - analyze step now passes `output: sarif-results` so the SARIF
    files land in a known directory. `upload: always` preserves the
    existing GH code-scanning ingestion.
  - New "Post CodeQL findings" step (if: always()) runs the
    reporter script via actions/github-script@v8, with
    SARIF_DIR / CODEQL_LANGUAGE env vars.
  - New upload-artifact step publishes codeql-summary.json as
    `ci-errors-codeql-<language>` so the consolidated CI Error
    Report job could optionally consume it in the future (it does
    not yet — the build-vs-codeql workflows are separate).
  - Added `issues: write` to the job permissions so the reporter
    script can create / update PR comments.

## Results

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4332 passed, 0 failed, 4332 total. 118,353 assertions.
       4 new tests:
         - FoliageRenderer_GlobalMaterialIdMatchesRegistryIndex
         - FoliageRenderer_UnregisteredSpeciesSkippedNotEmitted
         - EditorWinMgrReal_SaveSerializesFloatingAndMonitorState
         - (existing CSG tests re-verified with renamed methods)

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
## CSG crash fix — Windows VS2022 (Release)

`CSGEditorPanel_CreateBrushTracksIdAndSelects` crashed with
ACCESS_VIOLATION on Windows VS2022 Release. Root cause: the test
helper delegates to `CSGSystem::GetInstance()` but never called
`Initialize()` on the singleton. With shuffled test ordering on
Windows, earlier tests left stale brush state in the global singleton;
a subsequent `RebuildMesh()` walked that corrupt state and segfaulted.
On Linux GCC Release the happy-path shuffle order did not expose the
race so the crash only surfaced in CI.

Fix: `CSGPanelHelper`'s constructor now calls
`CSGSystem::GetInstance().Initialize()`, and its destructor calls
`Shutdown()`. Every CSG test starts and ends with a clean singleton
regardless of which tests ran before or after.

## Stack traces on test crashes (Windows + Linux)

`Tests/TestMain.cpp` previously printed just the test name and
exception type on crash. The Windows filter now walks the faulting
thread with `StackWalk64` + `SymFromAddr` + `SymGetLineFromAddr64`
(via DbgHelp) and prints a symbolicated backtrace with file:line
annotations. On stack overflow the walk is skipped to avoid
recursing into the exhausted stack.

On Linux the signal handler now calls `backtrace()` +
`backtrace_symbols_fd()` from `<execinfo.h>` (guarded by
`__has_include` so builds without glibc's backtrace still compile).
The functions are async-signal-safe — `backtrace_symbols()` (the
malloc variant) is deliberately NOT used. SIGBUS and SIGILL are now
also caught in addition to SIGSEGV/SIGABRT/SIGFPE. The trace is
mirrored to both stderr and stdout so the test output capture and
the extract-errors pipeline both see it.

## extract-errors.sh now captures crashes

The CI Error Report parser previously did not recognize the
`[ CRASH  ]` markers from TestMain — a segfaulting test showed up as
a silent test timeout with no indication of where it crashed.
`.github/scripts/extract-errors.sh` now has two new pattern blocks
in the "Test failures" section:

  1. A plain grep for `^\[ *CRASH *\]` that pulls the crash header
     line (test name + exception name / signal).
  2. An awk one-pass that grabs the first 10 stack trace frames
     emitted after `[ CRASH  ] Stack trace:` so the consolidated PR
     comment shows the top of the call stack without requiring
     users to download the full log artifact.

## Expanded CI Error Report coverage

The CI Error Report job consolidates artifacts from every job in its
`needs:` list. Several lint / format / hygiene jobs ran but never
posted results to PRs, so their failures only showed up in the
Actions UI. This commit brings them into the consolidated report:

clang-tidy job (.github/workflows/build.yml):
  - `sudo apt-get update` now retries up to 4 times with exponential
    backoff, to ride through the Azure mirror 403s that
    intermittently knock the job out before it can install clang-tidy.
  - The actual clang-tidy run now routes full output to
    `clang-tidy-output.log` (the previous script only tailed to
    stdout, losing everything else). The log is uploaded as the
    `clang-tidy-output` artifact with 14-day retention.
  - A new "Extract clang-tidy error summary" step runs
    `extract-errors.sh` against the log and uploads the result as
    `ci-errors-clang-tidy`, which the CI Error Report now consumes.
  - The clang-tidy run is marked `continue-on-error` so warnings
    never block the PR, but every warning / error still lands in the
    consolidated comment.

check-format job:
  - Same apt-get retry wrapper.
  - Output now captured to `check-format-output.log`.
  - On failure, `extract-errors.sh` produces `ci-errors-check-format`
    so formatting violations appear in the consolidated comment
    alongside build failures.

todo-count job:
  - Now writes the full TODO list to `todo-list.log` and uploads it
    as a retained artifact, so reviewers can see the exact comments
    over the 20-comment threshold.

CI Error Report `needs:` list expanded:
  - Added `check-format`, `check-thirdparty-manifest`,
    `build-windows-vs2026`, `clang-tidy`, `todo-count`.
  - Now covers every job that runs on a PR, so the single
    consolidated comment is the authoritative source of CI status.

## Coverage job hardening

`Code Coverage (GCC + per-subsystem thresholds)` has been failing
intermittently with an opaque "failure" status. The root cause was
`| tee` masking exit codes — a cmake --build failure was hidden
because its pipeline exit was from `tee` (which always succeeds)
rather than from cmake. Subsequent lcov steps then operated on a
half-built tree.

Fix:
  - Build step now uses `set -o pipefail` so cmake failures
    propagate through the tee pipeline.
  - Test step is explicitly tolerant (`|| true`) since test failures
    should not abort the coverage report, and the CI Error Report
    already picks them up separately.
  - Generate coverage step records lcov exit codes per command and
    fails the job ONLY if `coverage.info` ended up empty. Individual
    lcov warnings no longer block the per-subsystem analysis, and
    additional `--ignore-errors source,inconsistent` flags match the
    lcov 2.x error categories.

## Results

Build: SparkEngineLib + SparkTests clean (Linux GCC Release).
Tests: 4331 passed, 0 failed, 1 warned (pre-existing flaky),
       4332 total. 118,352 assertions. CSG tests pass with the
       new Initialize/Shutdown lifecycle.

Next PR CI run should:
  - Resolve the Windows VS2022 Release CSG crash.
  - Show stack traces on any remaining crashes (test-local or
    otherwise) in the CI Error Report PR comment.
  - Surface clang-tidy / check-format / todo-count findings in the
    single consolidated PR comment.
  - Provide clearer diagnostics if the coverage job fails again.

https://claude.ai/code/session_01FRJ3CQ5wtWKZShVGmi7J2G
@github-actions

Copy link
Copy Markdown
Contributor

Code Coverage (GCC + lcov)

Utils/MemoryMonitor.h                          |85.7%    14| 0.0%  12|    -    0
Utils/MultiISA.h                               |42.9%    14| 0.0%   6|    -    0
Utils/NetworkHealthMonitor.cpp                 | 113%    15| 0.0%   2|    -    0
Utils/NetworkHealthMonitor.h                   |    -     0|    -   0|    -    0
Utils/OpaqueHandle.h                           | 282%    11| 0.0%  15|    -    0
Utils/ProcessLinux.cpp                         |16.5%   182| 0.0%  28|    -    0
Utils/Profiler.cpp                             |14.2%   113| 0.0%  15|    -    0
Utils/Profiler.h                               |36.4%    33| 0.0%  12|    -    0
Utils/RandomEngine.h                           |29.3%    41| 0.0%  12|    -    0
Utils/Result.h                                 | 100%    29| 0.0%  28|    -    0
Utils/RingBuffer.h                             | 108%    59| 0.0%  64|    -    0
Utils/ScopeGuard.h                             | 142%    36| 0.0%  51|    -    0
Utils/ScopedTimer.h                            |25.0%    12| 0.0%   3|    -    0
Utils/Serializer.h                             | 100%    48| 0.0%  42|    -    0
Utils/SparkConsole.cpp                         |28.5%   144| 0.0%  17|    -    0
Utils/SparkConsole.h                           | 100%     2| 0.0%   2|    -    0
Utils/SparkError.h                             |10.9%    55| 0.0%   6|    -    0
Utils/SplinePath.h                             |    -     0|    -   0|    -    0
Utils/StackTrace.h                             |10.8%    74| 0.0%   8|    -    0
Utils/StateMachine.h                           |54.0%    63| 0.0%  32|    -    0
Utils/StringUtils.h                            |18.1%   116| 0.0%  21|    -    0
Utils/Telemetry.h                              |20.7%   135| 0.0%  22|    -    0
Utils/ThreadDebugger.h                         |11.6%   199| 0.0%  23|    -    0
Utils/ThreadSafeQueue.h                        |27.5%    40| 0.0%  11|    -    0
Utils/TimerManager.h                           |19.8%    96| 0.0%  19|    -    0
Utils/UUID.h                                   |43.2%    37| 0.0%  16|    -    0
Utils/Validate.h                               |    -     0|    -   0|    -    0

[/home/runner/work/SparkEngine/SparkEngine/SparkSDK/Include/Spark/]
IEngineContext.h                               |7300%     1| 0.0%   1|    -    0
ServiceInterfaces.h                            | 200%     3| 0.0%   3|    -    0
Version.h                                      |    -     0|    -   0|    -    0
================================================================================
                                         Total:|28.9% 25218| 0.0%  4k|    -    0

Per-Subsystem Coverage

Subsystem Lines Hit Coverage Threshold Status
AI 2817 734 26.1% 35%
Animation 160 154 96.2% 35%
Audio 0 0 0% 30%
Camera 0 0 0% 40%
Core 2255 1400 62.1% 40%
ECS 190 115 60.5% 40%
Editor 7148 2727 38.2% 25%
GameModules 6244 3129 50.1% 30%
Graphics 8486 4245 50% 30%
Networking 3420 2262 66.1% 35%
Physics 0 0 0% 35%
Scripting 0 0 0% 30%
Utils 7688 4864 63.3% 60%

Total: 51.1% (19630/38408 lines)

@Krilliac
Krilliac merged commit afe7858 into Working Apr 10, 2026
44 checks passed
@Krilliac
Krilliac deleted the claude/wind-impostor-integration-G3HTa branch April 10, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants