Worked human changes

CNA snapshot 009d40f5  ·  Development › Human Takeover  ·  source links pinned to 009d40f5

These are rehearsed investigation-and-validation routes for plausible regressions or enhancements, not claims that snapshot 009d40f5 has these bugs. Each route names the real entry points, invariants and tests that exist in the source; the actual patch should be chosen only after reproducing the symptom on your branch. Nothing here was built or run for this page, and no CNA source was changed.

A. A component removed during Update is still called

Symptom and reproduction. Model a component A whose Update removes and destroys component B, which comes later in the same update order. If B's Update still runs, or something dereferences freed memory, this is a lifetime bug, not an ordering preference. Start at GameTests.cpp, especially GameTest.AComponentRemovedMidFrameIsNotCalledLaterInThatSameFrame (a remover component with order −1 removes and frees a counting component from inside both its Update and its Draw). Run that one test in the STUB unit build:

cmake --preset unit && cmake --build --preset unit
ctest --test-dir cmake-build-unit -R 'GameTest\.AComponentRemovedMidFrameIsNotCalledLaterInThatSameFrame' --output-on-failure
# or the focused runtime executable, run from the repository root:
cmake --build cmake-build-unit --target CnaRuntimeTests
./cmake-build-unit/CnaRuntimeTests --gtest_filter='GameTest.AComponentRemovedMidFrame*'

The test skips when the selected platform cannot create a window, so confirm the output says it ran. Then trace Game::Update's snapshot creation, its loop and Game::OnComponentRemoved in Game.cpp. The same defect is reconstructed from real Git history in the component lifetime case study.

Invariant and patch scope. The in-flight vector currentlyUpdatingComponents_ holds raw IUpdateable*; removal nulls matching entries rather than erasing them while iteration is underway, and both loops skip a null entry. The recursive componentListsMutex_ protects the ordered lists and the snapshot copy, but component callbacks execute after it is released, so user code never runs under the lock. Do not "simplify" to an iterator over the live list or remove the nulling. For a new edge case, add a failing test with two updateables and a destruction sentinel before changing Game.cpp. Then check the draw side's analogous currentlyDrawingComponents_, the background-thread test ComponentsAddedFromALoadingThreadSurviveTheFrameThatIsIteratingThem, and repeated disposal (RepeatedDisposeDoesNotReinvokeUnloadContent).

Evidence. The focused test proves this sequence under STUB. If pointer lifetime changed, the whole CnaTests suite and an AddressSanitizer build are justified: the devices-asan preset configures OPENGLES3 with CNA_SANITIZE=address and builds CnaTests into cmake-build-devices-asan/. Self-review: can any callback still hold B? Did a new lock extend across user code? Does removal from Draw have the same semantics as removal from Update?

B. Change GraphicsDevice render-target behaviour

Frame the decision. Suppose a readback should reject an active render target, or target binding should validate a missing attachment atomically. First read the target lifecycle and the device policy, and the user contract on Render targets. Find GraphicsDevice::SetRenderTargets, GetBackBufferData (which funnels into GetBackBufferDataCore) and their guards in GraphicsDevice.cpp; then inspect the render-target virtuals of IGraphicsRenderer.hpp (CreateRenderTarget2D, SetRenderTarget2D, ReadBackbuffer, and IRenderTargetRenderer::BindAsRenderTarget) and two unlike implementations. A precondition that should hold for every renderer belongs in the neutral device code; image layout, resolve and attachment construction belong in each backend.

Note that many IGraphicsRenderer virtuals have default bodies — CreateRenderTarget2D returns nullptr, SetRenderTarget2D does nothing — so a family that does not override a new or changed virtual inherits the default silently instead of failing to compile. Enumerate every family, not only the ones the compiler complains about.

Evidence before editing. The existing BackBufferFormatContractTest.ReadbackRejectsAnActiveRenderTarget (BackBufferFormatContractTests.cpp) and BackBufferDepthStencilContractTest.ExplicitMissingRenderTargetAttachmentsThrowAtomically (BackBufferDepthStencilContractTests.cpp) are starting fixtures, not proof for every target format. Add a failing neutral test for the new transition. If a virtual contract changes, enumerate all 21 physical families from the renderer selection and compile at least representative CPU, GL and explicit-GPU implementations.

cmake --build --preset unit-graphics                      # CnaGraphicsTests, against STUB
./cmake-build-unit/CnaGraphicsTests --gtest_filter='BackBuffer*ContractTest.*'
cmake --preset multi-renderer && cmake --build cmake-build-multi --target CnaTests   # HEADLESS + SOFTWARE + STUB

After the patch, run CnaGraphicsTests, then the relevant target, resize and readback fixtures. The renderer-agnostic fixtures live in modules/graphics/examples (for example bound_target_lifetime_test.cpp, backbuffer_readback_dimension_test.cpp, rendertarget_preserve_across_resize_test.cpp) and are compiled per family by each renderer's examples/CMakeLists.txt, so they only exist in a build that selects that family with examples on. Then run a second renderer, and real-GPU validation where relevant. Inspect disposal while a target is bound, a resize, and a second frame. Self-review: has state been mutated before validation throws? Does a deferred Vulkan or SDL_gpu draw capture the target state at queue time (Vulkan's Pending3DDraw records carry their render target)? Will a resource be destroyed while native commands still reference it?

C. Fix one Vulkan present/resize path

Localize the failure in time. A blank second frame after resize could originate in the neutral viewport update, the surface extent, swapchain recreation, image acquisition, command recording, submission or presentation. Record where the first Vulkan error appears, but remember that a deferred draw may have been queued much earlier. Read the Vulkan internals in their recommended order — descriptor and construction (VulkanRendererDescriptor.cpp), DrawIndexedPrimitivesEx and Pending3DDraw, SubmitFrame, RecreateSwapchain, the destructor — all in VulkanRenderer.cpp. Compare the neutral GraphicsDevice::UpdateViewportFromWindow path, which Game::PollEvents calls on Resized and PixelSizeChanged before notifying the renderer with OnSurfaceInvalidated, so a backend-only patch does not conceal a common size bug.

Test and patch. Pick a Vulkan resize or swapchain test from vulkan/examples/CMakeLists.txt: Vulkan_SwapchainOutOfDate, Vulkan_Swapchain_Sync, Vulkan_SwapchainChurn, Vulkan_ViewportResetAfterResize, Vulkan_BackbufferResize, and Vulkan_RealWindowResize (registered only where SDL3 exists). The whole block is registered only when CNA_GRAPHICS_RENDERER=VULKAN with both CNA_BUILD_TESTS and CNA_BUILD_EXAMPLES on, outside Windows and Emscripten, and each example links a handler that turns "no usable Vulkan device" into a CTest skip. Reproduce under the private GPU runner, which provides a headless compositor and a rootful Xwayland with DRI3 (Xvfb cannot present Vulkan):

cmake -S . -B cmake-build-vulkan -G Ninja -DCNA_GRAPHICS_RENDERER=VULKAN -DCNA_BUILD_TESTS=ON -DCNA_BUILD_EXAMPLES=ON
cmake --build cmake-build-vulkan
tools/platform/run_gpu_tests_private.sh cmake-build-vulkan -R '^Vulkan_(SwapchainOutOfDate|Swapchain_Sync|SwapchainChurn|ViewportResetAfterResize|BackbufferResize)$'

Add an assertion around two frames or a zero-sized interval, then change the narrow native path. Preserve the out-of-date acquire invariant in SubmitFrame: an VK_ERROR_OUT_OF_DATE_KHR acquire calls RecreateSwapchain() and returns before vkResetFences, so a failed acquire never leaves the frame slot's fence unsignalled — do not reset an in-flight fence before an actual submission. Run with validation layers and the lifetime trace where active, then the neutral graphics tests and another renderer's resize/present path. A skip because no Vulkan device is available is a recorded gap, not success (see tools/platform/run_gpu_tests_private.sh and the test architecture). Self-review: are old image views retired only after GPU completion (RecreateSwapchain waits for the device before cleanup)? Are the frame-slot sync objects, which are indexed by frame slot and deliberately not recreated, incorrectly tied to a changing swapchain image count?

D. Change a Win32 keyboard or resize translation

Find the contract boundary. Native Win32 message dispatch and Win32Platform::PollEvents (Win32Platform.cpp) drain the thread's message queue, let the window procedures translate messages into PlatformEvent values (Win32EventMapper.cpp, Win32KeyCodes.cpp, Win32Scancodes.cpp), and move them into the caller's batch; Game consumes those events and the input services expose snapshots. A change to the virtual-key or scan-code mapping is not automatically a change to Keyboard::GetState unless the snapshot bridge is updated. A resize must distinguish the logical client size from drawable pixels under DPI (Win32DpiSupport.cpp). Read the Win32 backend, compare SDL3 event semantics, and use the event/snapshot split; the user view is Native Win32.

Test and patch. Reproduce with one exact message sequence or DPI transition. Add an assertion to the owning test — Win32EventMapperTests.cpp, Win32KeyCodeTests.cpp, Win32ScancodeTests.cpp, Win32DpiTests.cpp or Win32WindowTests.cpp — if it can run without a real desktop, and keep a live native-window test for DPI and focus behaviour. Patch only the translator or the size conversion if the IPlatform contract is already correct; if the contract changes, update the other backends and PlatformConformanceTests.cpp. Compile and run the Win32 cell under Wine the way CI does, with the standalone platform harness that needs no sibling checkout, SDL or renderer:

cmake -S tools/platform/standalone_tests -B build-win32 -G Ninja \
  -DCMAKE_TOOLCHAIN_FILE="$PWD/cmake/toolchains/mingw-w64.cmake" -DCNA_PLATFORM=WIN32 -DCMAKE_BUILD_TYPE=Debug
cmake --build build-win32 --parallel
wine build-win32/cna_platform_tests.exe

Then run the native MSVC job (the win32-native job in .github/workflows/platform-ci.yml, triggered only by workflow_dispatch) or a live Windows test for real DPI and input. Self-review: does PollEvents clear its caller's batch, preserve event order and update snapshot state? Does a renderer surface see the same drawable extent the window reports? A Wine pass does not establish physical keyboard or monitor behaviour.

E. Add or change a public API without silently breaking bindings

Classify first. Suppose you add a device capability query. Is it an XNA-compatibility member, a CNA extension, or only an internal renderer probe? Locate the public header in modules/graphics/include, implement the neutral semantics in GraphicsDevice, and decide whether the 19-member GraphicsCapability enum (GraphicsCapability.hpp), the finer RendererCapabilityProfile (RendererCapabilityProfile.hpp) or the renderer descriptor should carry the data (see RendererCapabilityProfile). A new enum value can affect ordinal compatibility: GraphicsCapabilityFloatRenderTargetTests.cpp pins the existing ordinals in ExistingCapabilityOrdinalsAreUnchanged (ThreeD = 0 through HalfFloatTextureLinearFiltering = 16) and checks representative format agreement; the two newest members are pinned only indirectly, by a static_assert that IndirectDraw is 18 in modern_gpu_capability_probe.cpp, which is compiled only where that example is built. For backend-derived data, implement truthful answers on every selected renderer, with unsupported defaults where appropriate.

Cross the boundary deliberately. Search modules/c-api/include/CNA/C and modules/c-api/src for the related route. The C ABI does not expose new C++ methods automatically. Here it mirrors the enum as fixed-width constants CNA_GRAPHICS_CAPABILITY_* (0–18, a closed range ending at CNA_GRAPHICS_CAPABILITY_MAXIMUM) in CNA/C/graphics.h, maps them by an explicit switch in CnaCApiGraphics.cpp and answers through cna_graphics_device_supports_capability; the profile has its own _ext feature, limit and format routes. Appending an identity moves a _MAXIMUM, and ABI_VERSIONING.md records that such an append increments the minor version (not the patch) and needs release notes and a regenerated baseline. If a C route is warranted, choose a new function, an appended field in a versioned struct (struct_size) or a new named constant — the four evolution paths that document allows — validate inputs, use the exception barrier (CallWithExceptionBarrier), and specify whether any returned handle is owned or borrowed. Add C-only tests under modules/c-api/tests/pure_c, and let the gates speak: CApi_HeaderAudit, CApi_Exports (ELF exports), CApi_WasmLinkContract (Emscripten builds with Node.js available), and the baseline gates CApiAbiHeaderBaseline and CApiAbiBaseline against tools/c-api/abi_baseline.json (regenerated with tools/c-api/generate_abi_baseline.py).

Then inspect the C#, Java and Python integrations at their recorded revisions, and the other bindings' gates on the bindings boundary. At this snapshot every public binding targets ABI 0.21.x while CNA exports 0.29.0, so do not claim live binding validation until a binding is migrated and run. Update samples and docs only if they consume the API. Self-review: did any old signature or enum ordinal move? Does struct_size still accept old callers? Can a wrapper's finalizer destroy the new object on the wrong thread (the handle registry answers CNA_RESULT_THREAD, but only if the wrapper checks)?

One review memo for every example

Record, for every change:

• the exact CNA commit and the selected platform, audio and renderer axes (and, for graphics, the active renderer);
• the failing test before the patch;
• the implementation function and its owner;
• the invariant restored;
• the focused and broad commands and their results, with skips counted separately;
• the configurations not run, and the native-host checks that were or were not done;
• a one-paragraph reason why untouched backends and bindings are unaffected.

If that last reason is "the diff is in one file", the review is incomplete. Use the blast-radius guide and the test matrix to challenge it, and the investigation record as the pre-patch half of the same memo. The Maintainer Handbook turns each of these rehearsals into a task recipe.