Fix resource leaks, stale bindings, dead RenderStats, naming bugs, alignment errors, window config, blend mode improvements, MRT per-attachment blending, and Debug UI pipeline leak found in full code review

.github/workflows/screenshot-tests.yml

@@ -0,0 +1,87 @@
+name: Screenshot Tests
+
+on:
+  pull_request:
+    branches: [main]
+  push:
+    branches: [main]
+  workflow_dispatch:
+
+concurrency:
+  group: screenshot-${{ github.ref }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  screenshot-report:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+
+      - name: Set up JDK 25
+        uses: actions/setup-java@v4
+        with:
+          distribution: temurin
+          java-version: '25'
+          cache: gradle
+
+      - name: Grant Gradle wrapper execute permission
+        run: chmod +x gradlew
+
+      - name: Install Mesa software rendering and virtual display
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y \
+            mesa-utils \
+            libegl-mesa0 \
+            libgl1-mesa-dri \
+            libglx-mesa0 \
+            libvulkan1 \
+            mesa-vulkan-drivers \
+            xvfb \
+            libjemalloc2
+
+      - name: Run screenshot tests (samples) with software rendering
+        run: |
+          export LIBGL_ALWAYS_SOFTWARE=1
+          export MESA_GL_VERSION_OVERRIDE=4.5
+          export MESA_GLSL_VERSION_OVERRIDE=450
+          export GALLIUM_DRIVER=llvmpipe
+          export VK_ICD_FILENAMES=/usr/share/vulkan/icd.d/lvp_icd.x86_64.json
+          xvfb-run -a -s "-screen 0 1280x720x24" \
+            ./gradlew :samples:tests:screenshot:screenshotReport --no-daemon --continue \
+            || true
+
+      - name: Run screenshot tests (examples) with software rendering
+        run: |
+          export LIBGL_ALWAYS_SOFTWARE=1
+          export MESA_GL_VERSION_OVERRIDE=4.5
+          export MESA_GLSL_VERSION_OVERRIDE=450
+          export GALLIUM_DRIVER=llvmpipe
+          export VK_ICD_FILENAMES=/usr/share/vulkan/icd.d/lvp_icd.x86_64.json
+          xvfb-run -a -s "-screen 0 1280x720x24" \
+            ./gradlew :examples:screenshotReport --no-daemon --continue \
+            || true
+
+      - name: Upload screenshot report (samples)
+        if: always()
+        uses: actions/upload-artifact@v4
+        with:
+          name: screenshot-report-samples
+          path: |
+            samples/tests/screenshot/build/screenshots/
+            samples/tests/screenshot/build/test-results/
+          retention-days: 30
+
+      - name: Upload screenshot report (examples)
+        if: always()
+        uses: actions/upload-artifact@v4
+        with:
+          name: screenshot-report-examples
+          path: |
+            examples/build/screenshots/
+            examples/build/test-results/
+          retention-days: 30

TODO.md

@@ -4,12 +4,16 @@ Full code review performed 2026-04-05 across all 467 source files.
 
 ## Critical Bugs
 
-- [ ] **Thread safety: Engine.run() race condition** — `Engine.java:152`. Render thread drains TransactionBuffer while logic thread writes concurrently. TransactionBuffer has no synchronization. Use TransactionBus (which has per-subscriber locks) or add synchronization.
-- [ ] **Profiler.lastFrame() returns currentFrame** — `Profiler.java:29`. Method named `lastFrame()` returns `currentFrame` (in-progress). `previousFrame()` returns `lastFrame`. Either rename or fix return values.
-- [ ] **GPU buffer leak: per-entity UBOs never cleaned up** — `UniformManager.java:79`. Creates objectUBOs ("obj_N") and materialUBOs ("mat_N") per entity. Never removed on EntityRemoved. Leaks GPU buffers for every entity that ever existed.
-- [ ] **Stale shader binding after entity reuse** — `ShaderManager.java:168`. entityShaders keyed by handle index only, no generation check. After entity destroy+recreate at same index, old shader persists.
-- [ ] **Engine.shutdown() doesn't shut down AssetManager** — `Engine.java:187`. FileWatcher thread keeps running. Add `assets.shutdown()` to teardown.
-- [ ] **Renderer.close() doesn't clean up MeshManager** — `Renderer.java:314`. MeshManager holds GPU resources (VBOs, IBOs, VAOs) via meshDataCache that are never released on shutdown.
+- [x] **Thread safety: Engine.run() race condition** — `Engine.java:152`. Render thread drains TransactionBuffer while logic thread writes concurrently. TransactionBuffer has no synchronization. Use TransactionBus (which has per-subscriber locks) or add synchronization.
+- [x] **Profiler.lastFrame() returns currentFrame** — `Profiler.java:29`. Renamed to `currentFrame()`; `lastFrame()` now correctly returns the previous completed frame. Tests updated accordingly.
+- [x] **GPU buffer leak: per-entity UBOs never cleaned up** — `UniformManager.java`. Per-entity UBO maps now keyed by `Handle<?>` (index + generation) instead of string. `removeEntity()` destroys the buffers when an entity is removed. `Renderer.renderFrame()` calls it for each `EntityRemoved` transaction.
+- [x] **Stale shader binding after entity reuse** — `ShaderManager.java`. `removeEntityShader()` added; called by `Renderer.renderFrame()` on `EntityRemoved`.
+- [x] **Engine.shutdown() doesn't shut down AssetManager** — `Engine.java`. Added `assets.shutdown()` to `shutdown()`.
+- [x] **Renderer.close() doesn't clean up MeshManager** — `Renderer.java`. Added `meshManager.close()` to `close()`. Added `MeshManager.close()` that destroys all GPU buffers and vertex inputs.
+- [x] **MeshRenderer leaks entity maps on EntityRemoved** — `MeshRenderer.java`. `meshDataAssignments`, `meshAssignments`, and `materialAssignments` were not cleared on `EntityRemoved`.
+- [x] **Vec2 alignment missing from material UBO write loop** — `UniformManager.java`. Size calculation aligned Vec2 to 8 bytes, but the write offset loop did not. Produced a layout mismatch (GPU reads wrong bytes for any material with Vec2 properties after an unaligned predecessor). Fixed by adding `offset = align(offset, 8)` before writing Vec2.
+- [x] **Remaining hardcoded `"Object"` string in `bindGlobals()`** — `UniformManager.java`. The `bindGlobals()` method still used the raw literal `"Object"` instead of `GlobalParamNames.OBJECT`. Now consistent with the rest of the class.
+- [x] **Material UBO silently wrong-sized on property change** — `UniformManager.uploadAndBindMaterial()`. Added `materialUboSizes` tracking map. When the required UBO size exceeds the allocated size (e.g., after a material gains new scalar properties), the old buffer is destroyed and a correctly-sized buffer is reallocated.
 
 ## Hardcoded Values (should be configurable/dynamic)
 
@@ -19,8 +23,8 @@ Full code review performed 2026-04-05 across all 467 source files.
 - [ ] **Texture/sampler array sizes: magic numbers** — GL: boundTextures[32], boundSamplers[32]. Vk: currentTextures[8], pendingUboBuffers[16], pendingSsboBuffers[8]. Should be queried from DeviceCapability or configurable.
 - [ ] **MAX_FRAMES_IN_FLIGHT=2 hardcoded** — `VkRenderDevice.java:47`. Should be configurable through VulkanConfig.
 - [ ] **MAX_SETS_PER_FRAME=256 hardcoded** — `VkDescriptorManager.java:17`. Could be insufficient for complex scenes. Should auto-grow or be configurable.
-- [ ] **Global param bindings hardcoded** — `Renderer.java:79-81`. "Engine"=0, "Camera"=1, "Object"=2 are string literals scattered throughout. Define as constants, centralize binding assignment.
-- [ ] **Blend function hardcoded to SRC_ALPHA/ONE_MINUS_SRC_ALPHA** — `GlRenderDevice.java:623`. BlendMode exists but only supports NONE vs one hardcoded alpha blend. Need configurable src/dst factors, blend equation.
+- [x] **Global param bindings hardcoded** — `Renderer.java:79-81`. Extracted to `GlobalParamNames` constants class (`ENGINE`, `CAMERA`, `OBJECT`). Used in `Renderer` and `UniformManager`.
+- [x] **Blend function hardcoded to SRC_ALPHA/ONE_MINUS_SRC_ALPHA** — Added `BlendFactor` and `BlendEquation` interfaces; extended `BlendMode` with separate color/alpha factor and equation accessors; added factory `BlendMode.of(...)` for custom blending; all three backends (GL/Vk/WebGPU) now derive blend factors from the `BlendMode` object using `glBlendFuncSeparate`/`glBlendEquationSeparate`. Fixed incorrect Vulkan blend-factor constants (DST_COLOR was 8→4, DST_ALPHA was 10→8). Fixed legacy `SetBlending` command to use `BlendMode.ALPHA` instead of hardcoded values. Added `RenderState.BLEND_MODES` (`BlendMode[]`) for per-attachment MRT blending: GL uses `glBlendFuncSeparatei`/`glBlendEquationSeparatei`/`glEnablei`/`glDisablei`; Vulkan creates one `VkPipelineColorBlendAttachmentState` per attachment; WebGPU silently falls back to single-target blending (current binding limitation).
 - [ ] **All shaders forced to STANDARD_FORMAT vertex layout** — `ShaderManager.java:255`. Uses PrimitiveMeshes.STANDARD_FORMAT for ALL pipelines. Custom vertex formats (tangents, colors, bone weights) won't work.
 - [ ] **Shader entry points hardcoded ("vertexMain"/"fragmentMain")** — `ShaderManager.java:227`. Should be configurable per shader for custom entry points.
 - [ ] **Camera defaults (near=0.1, far=1000) not in config** — `Camera.java:15`. Should be configurable through EngineConfig or named constants.
@@ -30,7 +34,7 @@ Full code review performed 2026-04-05 across all 467 source files.
 
 - [ ] **EngineConfig missing common options** — Missing: FPS cap, VSync toggle, MSAA sample count, anisotropic filtering, gamma/sRGB mode, fullscreen mode, monitor selection, cursor visibility, debug overlay toggle.
 - [ ] **GraphicsConfig missing graphics settings** — Only has headless and validation. Missing: MSAA, VSync mode, sRGB framebuffer, aniso level, GPU selection, shader cache directory, max texture size override.
-- [ ] **WindowDescriptor too minimal** — Only title/width/height. Missing: resizable, fullscreen, decorated, transparent, always-on-top, min/max size, initial position, high-DPI flag, cursor mode.
+- [x] **WindowDescriptor too minimal** — Created `WindowConfig` record in `graphics/api` with `title`, `width`, `height`, `resizable`, `decorated`, `vsync`, `fullscreen`, `alwaysOnTop`. `EngineConfig` now holds a `WindowConfig` (backward-compat `windowTitle()`/`windowSize()` accessors retained); builder gets `windowResizable()`, `windowDecorated()`, `windowVsync()`, `windowFullscreen()`, `windowAlwaysOnTop()`. `BaseApplication` applies all properties after window creation. Added `WindowProperty.ALWAYS_ON_TOP` (GLFW: `GLFW_FLOATING`). Still missing: transparent framebuffer, min/max size, initial position, high-DPI flag, cursor mode (require toolkit hints or additional GLFW calls).
 
 ## Designed but Not Implemented (from NOTES.md)
 
@@ -65,12 +69,18 @@ Full code review performed 2026-04-05 across all 467 source files.
 
 - [ ] **No lighting system** — LightData/LightType exist in core but Renderer has zero light handling. No light buffer upload, no light culling. MeshRenderer ignores LightData components.
 - [ ] **No shadow mapping** — Extensively designed in NOTES.md. No shadow pass, no shadow maps, no light-space matrices. LightData.CASTS_SHADOWS exists but nothing reads it.
-- [ ] **RenderStats never populated** — `RenderStats.java`. Tracks draws, vertices, binds but nothing calls recordDrawCall() etc. All counters always zero.
+- [x] **RenderStats never populated** — `RenderStats.java`. `Renderer.renderFrame()` now calls `recordDrawCall()`, `recordPipelineBind()`, `recordBufferBind()`, `recordTextureBind()`. Stats are reset at the top of each `renderFrame()` call. `Engine.renderStats()` now delegates to `renderer.renderStats()` (stats are owned by Renderer).
+
+## Code Quality / API
+
+- [x] **BaseApplication Javadoc example uses deprecated API** — Updated to show the new `.graphics(new OpenGlConfig(...))` API.
+- [x] **`BaseApplication` sets viewport twice per frame** — Removed unconditional per-frame `setViewport` call. Viewport is now set once before the loop and updated only on `WindowEvent.Resized`.
+- [x] **`DrawCommand.materialData` raw unchecked generic** — Changed to `PropertyMap<MaterialData>` for type safety.
 
 ## Architectural Cleanup
 
 - [ ] **Deprecated APIs to remove** — GraphicsBackendFactory, GraphicsConfigLegacy, EngineConfig.graphicsBackend field, GlfwWindowToolkit in graphics:opengl, deprecated SetDepthTest/SetBlending/SetCullFace/SetWireframe commands. New APIs (GraphicsConfig, SetRenderState) are in place.
-- [ ] **ResourceCleaner.register() package-private** — Can't be used outside core.resource. Make public or provide public API for Cleaner safety net.
+- [x] **ResourceCleaner.register() package-private** — Made `public` so GPU resource owners outside `core.resource` can register cleanup actions.
 - [ ] **NativeResource not used by GPU resources** — Interface exists but no GPU resource implements it. No Cleaner safety net for dropped handles.
 - [ ] **GpuResourceManager deferred deletion delay doesn't match frames-in-flight** — `GpuResourceManager.java:175`. Double-buffer swap means N+2 deletion. If Vulkan uses 3 frames-in-flight, resources freed while still in use. Tie to actual fence/frame count.
 
@@ -83,3 +93,14 @@ Full code review performed 2026-04-05 across all 467 source files.
 - [ ] **Async buffer mapping for TeaVM WebGPU** — Browser can't do synchronous mapping. Needs promise wrapper or skip readback on web.
 - [ ] **Slang generic specialization parsing robustness** — `SlangCompilerNative.java:245-276`. Current regex works. Revisit when it breaks.
 - [ ] **WGSL binding extraction regex** — `WgpuRenderDevice.java:623-625`. Works for current shaders. Revisit on Slang output changes.
+
+## Debug UI (new, from 2026-04-05 rebase review)
+
+- [x] **`DebugUiOverlay.close()` leaked pipeline handle** — `DebugUiOverlay.java`. The pipeline was created via `ShaderManager.compileSlangSource()` which does NOT cache the pipeline in `ShaderManager.shaderCache`. The original `close()` comment ("Pipeline is owned by ShaderManager") was incorrect; the pipeline is owned by `DebugUiOverlay`. Fixed by adding `device.destroyPipeline(pipeline)` to `close()`.
+- [ ] **`DebugUiOverlay` always allocated even in headless mode** — `Engine.java`. `Engine` constructor always creates `NkBuiltinFont` (builds 128×64 atlas in memory) and calls `DebugUiOverlay.init()`. The shader compilation failure is caught and `initialized` stays false, but font atlas allocation is wasted work in tests. Consider a lazy `initUi()` method or guard on `config.headless()`.
+- [ ] **`DebugUiOverlay.render()` scissor clamping only in WebGPU** — `WgpuRenderDevice.java` clamps scissor to RT size, but `GlRenderDevice` and `VkRenderDevice` do not. Negative or out-of-bounds scissor rects cause driver errors (GL: GL_INVALID_VALUE). The flipScissorY conversion can produce negative Y for small scissors near the top; RenderDevice scissor API should clamp to [0, viewport] on all backends.
+- [ ] **`NkContext` no scroll velocity / smooth scroll** — `NkContext.java`. Mouse scroll immediately jumps; no momentum or smooth scroll. Fine for a debug overlay but noticeable for any scrollable list.
+- [ ] **`NkContext.begin()` creates windows on first call, never removes them** — `NkContext.java`. `windows` map grows unboundedly if dynamic window names are used. `NkWindow.closed = true` hides but doesn't remove. Consider evicting windows not opened in N frames.
+- [ ] **`NkDrawList` index buffer uses `short` (max 65535 vertices)** — `NkDrawList.java`. 16-bit indices limit a single draw list to 65535 unique vertices. Fine for small debug overlays; will silently corrupt for complex UI.
+- [ ] **`debug_ui.slang` `[[vk::binding]]` applies to all targets** — `debug_ui.slang:33`. Slang propagates `[[vk::binding(N)]]` to WGSL output, forcing binding index N even in WebGPU where no such offset exists. For WebGPU the texture binding index should be 0. Currently `textureBindingOffset=0` for WebGPU so `N=0`; if that changes this will silently break WebGPU UI rendering. The `ShaderManager.prependParamBlocks` already guards SPIRV-only annotation for material textures — `debug_ui.slang` should do the same or be generated rather than hardcoded.
+

core/src/main/java/dev/engine/core/profiler/Profiler.java

@@ -26,11 +26,11 @@ public void newFrame() {
     }
 
     public Map<String, ProfileResult> lastFrame() {
-        return currentFrame;
+        return lastFrame;
     }
 
-    public Map<String, ProfileResult> previousFrame() {
-        return lastFrame;
+    public Map<String, ProfileResult> currentFrame() {
+        return currentFrame;
     }
 
     private void endScope(ScopeEntry entry) {

core/src/main/java/dev/engine/core/resource/ResourceCleaner.java

@@ -8,7 +8,7 @@ public final class ResourceCleaner {
 
     private ResourceCleaner() {}
 
-    static Cleaner.Cleanable register(Object resource, Runnable cleanupAction) {
+    public static Cleaner.Cleanable register(Object resource, Runnable cleanupAction) {
         return CLEANER.register(resource, cleanupAction);
     }
 }

core/src/test/java/dev/engine/core/profiler/ProfilerTest.java

@@ -19,7 +19,7 @@ class BasicScopes {
             try (var scope = profiler.scope("render")) {
                 spinWait(1_000_000); // ~1ms
             }
-            var results = profiler.lastFrame();
+            var results = profiler.currentFrame();
             assertNotNull(results);
             var render = results.get("render");
             assertNotNull(render);
@@ -35,7 +35,7 @@ class BasicScopes {
                     spinWait(500_000);
                 }
             }
-            var results = profiler.lastFrame();
+            var results = profiler.currentFrame();
             var frame = results.get("frame");
             assertNotNull(frame);
             assertEquals(2, frame.children().size());
@@ -49,7 +49,7 @@ class BasicScopes {
                     spinWait(500_000);
                 }
             }
-            var results = profiler.lastFrame();
+            var results = profiler.currentFrame();
             var parent = results.get("parent");
             var child = parent.children().get("child");
             assertTrue(parent.cpuNanos() >= child.cpuNanos());
@@ -62,7 +62,7 @@ class FrameReset {
             try (var s = profiler.scope("old")) { spinWait(100_000); }
             profiler.newFrame();
             try (var s = profiler.scope("new")) { spinWait(100_000); }
-            var results = profiler.lastFrame();
+            var results = profiler.currentFrame();
             assertNull(results.get("old"));
             assertNotNull(results.get("new"));
         }