diff --git a/.github/workflows/gpu-conformance-amd.yml b/.github/workflows/gpu-conformance-amd.yml index 5f2004684..a3fd84516 100644 --- a/.github/workflows/gpu-conformance-amd.yml +++ b/.github/workflows/gpu-conformance-amd.yml @@ -90,6 +90,21 @@ jobs: env: BUILD_TYPE: Release + # Build width, set here rather than as a `--parallel` flag so it applies + # to EVERY `cmake --build` in the job, including any added later that + # forget the flag. `cmake --build` reads this whenever `--parallel` is + # absent; an explicit `--parallel` on a step would override it. + # + # This must be set explicitly. Dropping the flag does NOT fall back to + # something conservative — with the Ninja generator the default is + # cores + 2, which is 18 on this 16-core host, i.e. MORE than the + # `--parallel 12` that already ran it out of memory. + # + # 6, not 16: this host also runs the gh-runner-1/2/3 runners for another + # repository. A nightly at ~02:47 local should normally find them idle, + # but "normally" is not "always" — leave headroom so a concurrent build + # elsewhere on the box doesn't turn into swap thrash or an OOM. + CMAKE_BUILD_PARALLEL_LEVEL: 6 # Goldens are baselined on NVIDIA hardware; GoldenImageTests.cpp reads this # to select a per-vendor golden directory (assets/tests/golden/amd/) so an # AMD run can never clobber the NVIDIA baseline set. @@ -154,6 +169,53 @@ jobs: # it — the same reasoning as Windows.yml's checkout. persist-credentials: false + # A self-hosted runner has whatever the box happens to have, and the runner + # user's PATH is NOT the admin's. cmake and ninja were originally pip + # installs under /home/obueker/.local/bin — on this host /home/obueker is + # mode 0700, so gh-runner-olo could not see them and the first run died + # with a bare `cmake: command not found` after a 39 s checkout. Everything + # the build needs must be system-wide (/usr, /opt), never in a person's + # home. Check that up front so the error names the cause. + # + # This runs FIRST, before the GL probe below, because that probe compiles + # its test program with gcc. With the order reversed a missing compiler + # surfaces as a bare `gcc: command not found` from the GL step instead of + # the remediation message here — do not reorder these two. + - name: Preflight — build toolchain + run: | + set -euo pipefail + missing=0 + for tool in cmake ninja ccache gcc g++ python3; do + if command -v "$tool" >/dev/null 2>&1; then + printf ' %-8s %s\n' "$tool" "$(command -v "$tool")" + else + echo "::error::$tool not found on the runner's PATH" + missing=1 + fi + done + # Python modules count as toolchain too. glad2's code generation + # imports jinja2, and a `pip install --user` puts it in the INSTALLING + # user's home — invisible to the runner user for the same 0700 reason + # as everything else. Checking only binaries let this through: the + # preflight passed and the build then died on ModuleNotFoundError + # four minutes in. Check what the build actually imports. + for mod in jinja2; do + if python3 -c "import $mod" 2>/dev/null; then + printf ' %-8s %s\n' "$mod" "$(python3 -c "import $mod,os; print(os.path.dirname($mod.__file__))")" + else + echo "::error::python module '$mod' not importable by $(id -un)" + missing=1 + fi + done + + if [ "$missing" -ne 0 ]; then + echo "::error::Provision the runner: sudo dnf install -y cmake ninja-build ccache gcc gcc-c++ python3 python3-jinja2" + echo "::error::See docs/ops/self-hosted-gpu-runner.md. Everything must be system-wide —" + echo "::error::not under a 0700 home, and not a 'pip install --user' in someone else's account." + exit 1 + fi + cmake --version | head -1 + # --------------------------------------------------------------------- # Preflight: prove we have a HARDWARE context before spending an hour # building. This is the anti-silent-failure guard, and it is the whole @@ -230,7 +292,21 @@ jobs: : "${VULKAN_SDK:?VULKAN_SDK is not set - see docs/ops/self-hosted-gpu-runner.md}" test -d "$VULKAN_SDK/include/vulkan" || { echo "::error::no headers under $VULKAN_SDK"; exit 1; } test -e "$VULKAN_SDK/lib/libshaderc_shared.so" || { echo "::error::no shaderc under $VULKAN_SDK"; exit 1; } + + # The LunarG SDK does NOT place the loader in lib/ — it nests it under + # lib/VulkanLoader/lib/. CMAKE_PREFIX_PATH therefore resolves the + # headers and the *version* but not Vulkan_LIBRARY, and configure dies + # with "Could NOT find Vulkan (missing: Vulkan_LIBRARY) (found version + # 1.4.350)" — found-but-not-found, which reads like a version problem + # and is not one. Locate the loader and hand the configure step an + # exact path rather than letting it guess at a layout that varies + # between SDK versions. + loader=$(find "$VULKAN_SDK" -name 'libvulkan.so' -print -quit 2>/dev/null || true) + [ -n "$loader" ] || { echo "::error::no libvulkan.so anywhere under $VULKAN_SDK"; exit 1; } + echo "Vulkan SDK: $VULKAN_SDK" + echo " loader: $loader" + echo "OLO_VULKAN_LIBRARY=$loader" >> "$GITHUB_ENV" - name: Configure CMake # Heavy optional interchange deps (USD/Alembic/MaterialX) and the FFmpeg @@ -246,18 +322,31 @@ jobs: cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_PREFIX_PATH="$VULKAN_SDK" + -DVulkan_LIBRARY="$OLO_VULKAN_LIBRARY" -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DOLO_VIDEO_FFMPEG=OFF -DOLO_WITH_USD=OFF -DOLO_WITH_ALEMBIC=OFF -DOLO_WITH_MATERIALX=OFF + -DOLO_LINK_JOBS=1 - name: Build tests - # --parallel 12, not nproc (16): this host also runs the gh-runner-1/2/3 - # runners for another repository. A nightly at ~02:47 local should - # normally find them idle, but "normally" is not "always" — leave - # headroom so a concurrent build on the other runners doesn't turn into - # swap thrash or an OOM during the OloEngine-Tests link. - run: cmake --build build --target OloEngine-Tests --parallel 12 + # Width comes from CMAKE_BUILD_PARALLEL_LEVEL in the job env — see the + # note there for why it must be set explicitly rather than left to the + # generator default. + # + # Link concurrency is the second half of it, and the more important + # half for this failure: linking OloEngine-Tests is the memory spike, + # and a link peaks far higher than a compile. A plain -j cap still + # permits several concurrent links. + # + # That cap now belongs to the project — the root CMakeLists owns an + # olo_link job pool sized by OLO_LINK_JOBS (default 2), so `ninja` + # obeys it too, not just `cmake --build`. The configure step above + # therefore sets OLO_LINK_JOBS=1 rather than declaring a second, + # workflow-local pool: two mechanisms binding CMAKE_JOB_POOL_LINK + # would have the workflow's silently win, leaving the project's + # setting looking effective while doing nothing. + run: cmake --build build --target OloEngine-Tests # --------------------------------------------------------------------- # The suite runs from OloEditor/ — OloEditor, OloRuntime and the test @@ -278,8 +367,15 @@ jobs: MESA_LOADER_DRIVER_OVERRIDE: radeonsi run: | mkdir -p ../test_results + # NetworkIntegrationTest is excluded by EVERY other job in this repo + # (see asan.yml's --exclude-regex on all three Linux sanitiser jobs): + # it needs real socket plumbing that CI environments don't provide, and + # it failed all six cases here for the same reason. Excluding it keeps + # this job's red/green meaningful; it is not a GPU test and this job + # has no business gating on it. ../build/OloEngine/tests/OloEngine-Tests \ --gtest_catch_exceptions=1 \ + --gtest_filter=-NetworkIntegrationTest.* \ --gtest_output=xml:../test_results/gpu_amd.xml # --------------------------------------------------------------------- diff --git a/CLAUDE.md b/CLAUDE.md index 157e8d5ce..2c7235b63 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -86,16 +86,40 @@ CMake presets ([CMakePresets.json](CMakePresets.json)) — note all three requir scripts\Win-GenerateProjectVS2022.bat # or VS2026 # Build a target -cmake --build build --target OloEditor --config Debug --parallel -cmake --build build --target OloEngine-Tests --config Debug --parallel -cmake --build build --target OloRuntime --config Debug --parallel -cmake --build build --target OloServer --config Debug --parallel +cmake --build build --target OloEditor --config Debug --parallel 6 +cmake --build build --target OloEngine-Tests --config Debug --parallel 6 +cmake --build build --target OloRuntime --config Debug --parallel 6 +cmake --build build --target OloServer --config Debug --parallel 6 # ClangCL (configure once, then build) cmake --preset clangcl -cmake --build build-clang --target OloEngine-Tests --config Debug --parallel +cmake --build build-clang --target OloEngine-Tests --config Debug --parallel 6 ``` +### Cap build parallelism — a full-width build can OOM this machine + +**Never build uncapped.** Either pass an explicit job count — `--parallel 6`, or `ninja -j6` — or set `CMAKE_BUILD_PARALLEL_LEVEL`, which `cmake --build` uses whenever no `--parallel` is given (this is how the nightly workflow caps itself): + +```powershell +$env:CMAKE_BUILD_PARALLEL_LEVEL = "6" # PowerShell (the primary dev shell here) +``` +```bash +export CMAKE_BUILD_PARALLEL_LEVEL=6 # POSIX shell / the Linux GPU runner +``` + +An explicit `--parallel N` overrides the environment variable, so don't set one expecting the other to win. + +**`CMAKE_BUILD_PARALLEL_LEVEL` caps `cmake --build` only — `ninja` does not read it.** A direct `ninja` invocation must always carry a numeric `-jN` of its own; setting the variable and then running bare `ninja` gives you the full 18-wide default with no warning. What is never acceptable is a bare `--parallel`, or a direct `ninja` without `-jN`. + +This is not a style preference. The dev box is 16 cores / 31 GB and *also* hosts the `gh-runner-1/2/3` runners for another repository, so a build never has the machine to itself. Neither default is a cap: + +- `cmake --build … --parallel` with **no number** does not pick a number itself — it forwards the omission to the native build tool, whose own default applies (unless `CMAKE_BUILD_PARALLEL_LEVEL` is set). So the width you get depends on the generator, and it is never *lower* than the tool's default. +- With Ninja that default is `cores + 2` — 18 on this host, confirmed by `ninja --help` reporting `[default=18 on this system]`. Dropping a `--parallel N` flag therefore *raises* the width rather than lowering it. + +An agent session running repeated uncapped builds — especially with a test suite running alongside — has already OOM-killed this host once. If you need it faster, use ccache (already wired in), not more jobs. + +Link steps are capped separately and automatically: the root `CMakeLists.txt` puts them in a Ninja job pool (`OLO_LINK_JOBS`, default 2) because linking the full static engine is the memory spike. That pool lives in the generated `build.ninja`, so it protects a bare `ninja` too — but it does **not** cap compilation, which is what the job count above is for. + VS Code tasks ([.vscode/tasks.json](.vscode/tasks.json)) wrap the above: `build-oloeditor-debug`, `run-oloeditor-debug`, `build-tests-debug`, `run-tests-debug`, `build-clangcl-tests-debug`, `configure-clangcl`, etc. **Working directory matters.** `OloEditor`, `OloRuntime`, and `OloServer` resolve assets, shaders, and Mono assemblies relative to `OloEditor/`. Always run with `cwd = OloEditor/` (the VS Code tasks already do this; the test binary runs from repo root instead). diff --git a/CMakeLists.txt b/CMakeLists.txt index e7f112547..164636e08 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -77,6 +77,44 @@ list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_BINARY_DIR} ) +# -------------------------------------------------------------------------- +# Link concurrency (Ninja only). +# +# Linking is this project's memory spike, not compiling: OloEngine-Tests and +# OloEditor pull in the whole static engine plus its vendored dependencies, and +# each concurrent link holds that peak at once. Ninja's default width is +# `cores + 2` (18 on a 16-core host), so an unqualified `ninja` will happily +# start several of those together and take the machine out — which is exactly +# how this host was OOM-killed while three other CI runners shared it. +# +# A pool is the right tool rather than a smaller `-j`: it throttles ONLY the +# link step, leaving compilation at full width. It also lives in the generated +# build.ninja, so it protects a bare `ninja` invocation too — not just +# `cmake --build`, which is the only thing CMAKE_BUILD_PARALLEL_LEVEL reaches. +# +# Raise it on a machine with memory to spare: -DOLO_LINK_JOBS=4. +if(CMAKE_GENERATOR MATCHES "Ninja") + set(OLO_LINK_JOBS "2" CACHE STRING "Maximum concurrent link steps (Ninja generators)") + # THIS PROJECT requires a finite positive depth — that is a stricter rule + # than the toolchain's, deliberately. + # + # Measured, rather than assumed: CMake already rejects a non-numeric depth + # at configure time (build.ninja is never generated), so that case needs no + # help here. But `depth = 0` passes straight through CMake AND ninja, and + # the build then runs happily with the cap doing nothing. Since the entire + # point of this pool is to bound peak memory during linking, a value that + # quietly disables it is worse than one that fails — so reject it here and + # say why, instead of leaving a green build that is not actually capped. + if(NOT OLO_LINK_JOBS MATCHES "^[1-9][0-9]*$") + message(FATAL_ERROR + "OLO_LINK_JOBS must be a positive integer (got '${OLO_LINK_JOBS}'). " + "It bounds concurrent link steps to limit peak memory; 0 or empty " + "would leave links effectively uncapped.") + endif() + set_property(GLOBAL PROPERTY JOB_POOLS olo_link=${OLO_LINK_JOBS}) + set(CMAKE_JOB_POOL_LINK olo_link) +endif() + set(CMAKE_CXX_STANDARD 23) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS ON) diff --git a/OloEditor/assets/shaders/AtmosphereSky.glsl b/OloEditor/assets/shaders/AtmosphereSky.glsl index c11bcc700..7756c216c 100644 --- a/OloEditor/assets/shaders/AtmosphereSky.glsl +++ b/OloEditor/assets/shaders/AtmosphereSky.glsl @@ -117,16 +117,46 @@ vec3 dayLayer(vec3 viewDir) // ── Night half — mirrored CPU-side in AtmosphereSky.cpp ── -// Mirrors Hash13 (AtmosphereSky.cpp). -float hash13(vec3 p) +// Integer bit-mixer (PCG output permutation). Mirrors PcgHash +// (AtmosphereSky.cpp) EXACTLY: unsigned wrap, shift and xor are bit-defined +// operations, so every vendor and the CPU produce identical results. +// +// This replaces the classic `fract(sin(dot(p, k)) * 43758.5453)` hash, which +// is NOT portable. That hash feeds sin() an argument in the tens of thousands, +// where a 1-ULP difference in the argument moves the result by a large +// fraction of a period; NVIDIA and Mesa do not implement sin() to identical +// precision there, so the *= 43758 and fract() amplified the disagreement into +// completely different values. The stars therefore landed in different places +// on different GPUs, and the C++ mirror (std::sin) could match neither. +uint pcgHash(uint v) +{ + uint state = v * 747796405u + 2891336453u; + uint word = ((state >> ((state >> 28u) + 4u)) ^ state) * 277803737u; + return (word >> 22u) ^ word; +} + +// Mirrors HashCell (AtmosphereSky.cpp). Integer lattice cell -> uint. +uint hashCell(ivec3 c, uint seed) +{ + uint h = pcgHash(uint(c.x) ^ 0x9E3779B9u); + h = pcgHash(h ^ uint(c.y) ^ 0x85EBCA6Bu); + h = pcgHash(h ^ uint(c.z) ^ 0xC2B2AE35u); + return pcgHash(h ^ seed); +} + +// Mirrors Hash1 (AtmosphereSky.cpp). Result in [0,1). +// Masked to 24 bits so the uint->float conversion is EXACT on every +// implementation (a float mantissa holds 24 bits), and scaled by a power of +// two so the divide introduces no rounding of its own. +float hash1(ivec3 c, uint seed) { - return fract(sin(dot(p, vec3(127.1, 311.7, 74.7))) * 43758.5453); + return float(hashCell(c, seed) & 0xFFFFFFu) * (1.0 / 16777216.0); } -// Mirrors Hash33 (AtmosphereSky.cpp). -vec3 hash33(vec3 p) +// Mirrors Hash3 (AtmosphereSky.cpp). +vec3 hash3(ivec3 c) { - return vec3(hash13(p), hash13(p + vec3(19.19, 0.0, 0.0)), hash13(p + vec3(0.0, 47.31, 0.0))); + return vec3(hash1(c, 0u), hash1(c, 1u), hash1(c, 2u)); } // Mirrors StarField (AtmosphereSky.cpp). @@ -136,12 +166,16 @@ float starField(vec3 dir, float rotation, float intensity) float s = sin(rotation); vec3 d = vec3(c * dir.x + s * dir.z, dir.y, -s * dir.x + c * dir.z); + // `dir` is unit, so p stays within +/-60 and the cell index converts to + // int exactly. A 1-ULP disagreement in cos/sin between vendors now only + // matters exactly on a cell boundary, instead of re-rolling every star. vec3 p = d * 60.0; - vec3 cell = floor(p); - vec3 f = p - cell; - vec3 starPos = hash33(cell); + vec3 cellF = floor(p); + ivec3 cell = ivec3(cellF); + vec3 f = p - cellF; + vec3 starPos = hash3(cell); float dist = length(f - starPos); - float lum = pow(hash13(cell + vec3(17.0)), 14.0); + float lum = pow(hash1(cell, 3u), 14.0); float star = smoothstep(0.18, 0.0, dist) * lum; return star * intensity * 60.0; } diff --git a/OloEditor/assets/shaders/InfiniteGrid.glsl b/OloEditor/assets/shaders/InfiniteGrid.glsl index cd43471a0..1b9a4c15f 100644 --- a/OloEditor/assets/shaders/InfiniteGrid.glsl +++ b/OloEditor/assets/shaders/InfiniteGrid.glsl @@ -143,7 +143,24 @@ void main() { } FragColor = gridColor; - gl_FragDepth = depth; + + // Break the coplanar tie with scene ground geometry. + // + // The grid lies on Y=0, and scenes routinely put a ground plane on Y=0 + // too. The ground mesh gets the rasteriser's INTERPOLATED depth; this + // shader writes gl_FragDepth from a position that was unprojected and + // then re-projected. Those two paths compute the same surface and + // disagree by ~1 ULP, so the depth test resolves it per-pixel on float + // noise. NVIDIA happens to resolve it consistently; Mesa/radeonsi does + // not, and the grid lines break into dashes toward the horizon + // ("z-precision-dashed", RMSE ~22 against the NVIDIA goldens). + // + // Nudging the grid a hair toward the camera makes it win the tie on + // every vendor instead of by luck. 1e-5 in [0,1] depth is ~170 quanta + // of a 24-bit buffer -- comfortably decisive, and far too small to lift + // the grid visibly off the ground. + const float kCoplanarBias = 1e-5; + gl_FragDepth = clamp(depth - kCoplanarBias, 0.0, 1.0); EntityID = -1; // Grid is not pickable o_ViewNormal = vec2(-2.0); diff --git a/OloEditor/assets/shaders/compute/GTAO.comp b/OloEditor/assets/shaders/compute/GTAO.comp index 9ba3b522f..41bb0c145 100644 --- a/OloEditor/assets/shaders/compute/GTAO.comp +++ b/OloEditor/assets/shaders/compute/GTAO.comp @@ -185,6 +185,21 @@ void main() return; } + // Match SSAO: no normal means no occlusion, so store full visibility. + // + // Tested next to the depth early-out rather than at the decode below: a + // sentinel pixel produces exactly this result regardless of depth, + // neighbour depths, edges or reconstructed position, so computing those + // first is pure waste. On a forward-path frame the overlay shaders cover + // a large share of the screen. + vec2 encodedNormal = texelFetch(u_ViewNormals, pixCoord, 0).rg; + if (encodedNormal.x < -1.5) + { + imageStore(o_AOTerm, pixCoord, vec4(1.0)); + imageStore(o_Edges, pixCoord, vec4(0.0)); + return; + } + float viewspaceZ = LinearizeDepth(deviceZ); // Neighbour depths for edge detection @@ -207,8 +222,22 @@ void main() // diverges from the real view ray increasingly toward the edges). vec3 viewVec = normalize(-pixCenterPos); - // Decode world-space normal from GBuffer and convert to view-space - vec2 encodedNormal = texelFetch(u_ViewNormals, pixCoord, 0).rg; + // Decode world-space normal from GBuffer and convert to view-space. + // + // (-2,-2) is the engine-wide "no normal here" sentinel, written by every + // shader that draws into the scene normal RT without a meaningful surface + // normal: InfiniteGrid, the four Particle_* shaders, and the Renderer2D_* + // quad/circle/line/text/polygon shaders. It is outside any valid + // octahedral range, so decoding it yields a garbage normal and GTAO + // FABRICATES occlusion wherever those shaders drew. + // + // SSAO.glsl has always guarded this ("sentinel value (-2,-2) means no + // normal"); GTAO.comp did not, which is why the forward path baked the + // editor grid into the AO buffer as a lattice of dark lines while the + // deferred path stayed clean -- the G-Buffer never carries those overlay + // draws, so only forward sees the sentinel. It cost ~4/255 mean absolute + // difference between the two paths at an off-axis pose. + // vec3 viewNormal = normalize(mat3(u_ViewMatrix) * OctDecode(encodedNormal)); // Noise for slice rotation (.x) and sample distance along the slice (.y). diff --git a/OloEditor/assets/tests/golden/amd/fxaa_hard_edge.png b/OloEditor/assets/tests/golden/amd/fxaa_hard_edge.png new file mode 100644 index 000000000..2db477479 Binary files /dev/null and b/OloEditor/assets/tests/golden/amd/fxaa_hard_edge.png differ diff --git a/OloEditor/assets/tests/golden/amd/scene_shadow_integration.png b/OloEditor/assets/tests/golden/amd/scene_shadow_integration.png new file mode 100644 index 000000000..15d5e7a9d Binary files /dev/null and b/OloEditor/assets/tests/golden/amd/scene_shadow_integration.png differ diff --git a/OloEditor/assets/tests/golden/amd/scene_splatmap_integration.png b/OloEditor/assets/tests/golden/amd/scene_splatmap_integration.png new file mode 100644 index 000000000..b962f9bb1 Binary files /dev/null and b/OloEditor/assets/tests/golden/amd/scene_splatmap_integration.png differ diff --git a/OloEditor/assets/tests/golden/amd/tonemap_reinhard_hdr_ramp.png b/OloEditor/assets/tests/golden/amd/tonemap_reinhard_hdr_ramp.png new file mode 100644 index 000000000..93eb16e02 Binary files /dev/null and b/OloEditor/assets/tests/golden/amd/tonemap_reinhard_hdr_ramp.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_DawnClear.png b/OloEditor/assets/tests/visual/amd/Atmosphere_DawnClear.png new file mode 100644 index 000000000..9178bc347 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_DawnClear.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_DawnOvercast.png b/OloEditor/assets/tests/visual/amd/Atmosphere_DawnOvercast.png new file mode 100644 index 000000000..c1149346a Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_DawnOvercast.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_DawnStorm.png b/OloEditor/assets/tests/visual/amd/Atmosphere_DawnStorm.png new file mode 100644 index 000000000..3a6d72bc3 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_DawnStorm.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_DuskClear.png b/OloEditor/assets/tests/visual/amd/Atmosphere_DuskClear.png new file mode 100644 index 000000000..948c3d2c5 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_DuskClear.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_DuskOvercast.png b/OloEditor/assets/tests/visual/amd/Atmosphere_DuskOvercast.png new file mode 100644 index 000000000..062a73d1e Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_DuskOvercast.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_DuskStorm.png b/OloEditor/assets/tests/visual/amd/Atmosphere_DuskStorm.png new file mode 100644 index 000000000..4f03bb1b3 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_DuskStorm.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NightClear.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NightClear.png new file mode 100644 index 000000000..f9f652ce9 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NightClear.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NightOvercast.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NightOvercast.png new file mode 100644 index 000000000..a9e88aaa5 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NightOvercast.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NightStorm.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NightStorm.png new file mode 100644 index 000000000..dae69bc28 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NightStorm.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NoonClear.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonClear.png new file mode 100644 index 000000000..f124d631a Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonClear.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NoonClearAerial.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonClearAerial.png new file mode 100644 index 000000000..2baea5ff0 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonClearAerial.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NoonOvercast.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonOvercast.png new file mode 100644 index 000000000..f98a1543f Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonOvercast.png differ diff --git a/OloEditor/assets/tests/visual/amd/Atmosphere_NoonStorm.png b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonStorm.png new file mode 100644 index 000000000..6f046f335 Binary files /dev/null and b/OloEditor/assets/tests/visual/amd/Atmosphere_NoonStorm.png differ diff --git a/OloEngine/src/OloEngine/Asset/AssetSerializer.cpp b/OloEngine/src/OloEngine/Asset/AssetSerializer.cpp index da523a5bc..d4ed1486c 100644 --- a/OloEngine/src/OloEngine/Asset/AssetSerializer.cpp +++ b/OloEngine/src/OloEngine/Asset/AssetSerializer.cpp @@ -2739,21 +2739,21 @@ namespace OloEngine stream.WriteRaw(sub.m_VertexCount); { constexpr u32 MAX_SUBMESH_NAME_LEN = 1'024; - if (sub.m_NodeName.size() > MAX_SUBMESH_NAME_LEN) + if (static_cast(sub.m_NodeName.Len()) > MAX_SUBMESH_NAME_LEN) { OLO_CORE_ERROR("MeshSourceSerializer::SerializeToAssetPack - Submesh {} NodeName length ({}) exceeds limit ({})", - i, sub.m_NodeName.size(), MAX_SUBMESH_NAME_LEN); + i, sub.m_NodeName.Len(), MAX_SUBMESH_NAME_LEN); return false; } - if (sub.m_MeshName.size() > MAX_SUBMESH_NAME_LEN) + if (static_cast(sub.m_MeshName.Len()) > MAX_SUBMESH_NAME_LEN) { OLO_CORE_ERROR("MeshSourceSerializer::SerializeToAssetPack - Submesh {} MeshName length ({}) exceeds limit ({})", - i, sub.m_MeshName.size(), MAX_SUBMESH_NAME_LEN); + i, sub.m_MeshName.Len(), MAX_SUBMESH_NAME_LEN); return false; } } - stream.WriteString(sub.m_NodeName); - stream.WriteString(sub.m_MeshName); + stream.WriteString(sub.m_NodeName.ToStdString()); + stream.WriteString(sub.m_MeshName.ToStdString()); stream.WriteRaw(sub.m_IsRigged); } } @@ -3169,20 +3169,29 @@ namespace OloEngine stream.ReadRaw(sub.m_MaterialIndex); stream.ReadRaw(sub.m_IndexCount); stream.ReadRaw(sub.m_VertexCount); - stream.ReadString(sub.m_NodeName); - stream.ReadString(sub.m_MeshName); + // ReadString fills a std::string&; go through temporaries and + // assign, so the on-disk format is untouched by the FString + // conversion. + { + std::string nodeName; + std::string meshName; + stream.ReadString(nodeName); + stream.ReadString(meshName); + sub.m_NodeName = FString(nodeName); + sub.m_MeshName = FString(meshName); + } { constexpr u32 MAX_SUBMESH_NAME_LEN = 1'024; - if (sub.m_NodeName.size() > MAX_SUBMESH_NAME_LEN) + if (static_cast(sub.m_NodeName.Len()) > MAX_SUBMESH_NAME_LEN) { OLO_CORE_ERROR("MeshSourceSerializer::DeserializeFromAssetPack - Submesh {} NodeName length ({}) exceeds limit ({})", - i, sub.m_NodeName.size(), MAX_SUBMESH_NAME_LEN); + i, sub.m_NodeName.Len(), MAX_SUBMESH_NAME_LEN); return false; } - if (sub.m_MeshName.size() > MAX_SUBMESH_NAME_LEN) + if (static_cast(sub.m_MeshName.Len()) > MAX_SUBMESH_NAME_LEN) { OLO_CORE_ERROR("MeshSourceSerializer::DeserializeFromAssetPack - Submesh {} MeshName length ({}) exceeds limit ({})", - i, sub.m_MeshName.size(), MAX_SUBMESH_NAME_LEN); + i, sub.m_MeshName.Len(), MAX_SUBMESH_NAME_LEN); return false; } } diff --git a/OloEngine/src/OloEngine/Asset/Interchange/AssimpMeshExporter.cpp b/OloEngine/src/OloEngine/Asset/Interchange/AssimpMeshExporter.cpp index f9e6e4fa8..63cd289ca 100644 --- a/OloEngine/src/OloEngine/Asset/Interchange/AssimpMeshExporter.cpp +++ b/OloEngine/src/OloEngine/Asset/Interchange/AssimpMeshExporter.cpp @@ -209,9 +209,10 @@ namespace OloEngine if (materialIndex >= realMaterialCount) materialIndex = defaultMaterialIndex; - std::string meshName = submesh.m_MeshName.empty() - ? (submesh.m_NodeName.empty() ? ("Mesh_" + std::to_string(m)) : submesh.m_NodeName) - : submesh.m_MeshName; + std::string meshName = + submesh.m_MeshName.IsEmpty() + ? (submesh.m_NodeName.IsEmpty() ? ("Mesh_" + std::to_string(m)) : submesh.m_NodeName.ToStdString()) + : submesh.m_MeshName.ToStdString(); scene.mMeshes[m] = BuildMesh(source, submesh, materialIndex, meshName); scene.mRootNode->mMeshes[m] = m; } diff --git a/OloEngine/src/OloEngine/Containers/Array.h b/OloEngine/src/OloEngine/Containers/Array.h index 0f89cff57..c1a1c3be3 100644 --- a/OloEngine/src/OloEngine/Containers/Array.h +++ b/OloEngine/src/OloEngine/Containers/Array.h @@ -433,8 +433,20 @@ namespace OloEngine friend class TArray; ElementAllocatorType m_AllocatorInstance; - SizeType m_ArrayNum; - SizeType m_ArrayMax; + // Zero-initialised in-class, NOT left indeterminate. + // + // The copy constructors call CopyToEmpty() on a freshly-declared array + // and CopyToEmpty in turn calls ResizeAllocation(), which short-circuits + // on `if (NewMax != m_ArrayMax)`. With m_ArrayMax indeterminate that + // comparison reads uninitialised memory, and whenever the garbage + // happened to equal the computed NewMax the allocation was SKIPPED — + // leaving GetData() null and the following ConstructItems memcpy'ing to + // address 0. It surfaced as a SIGSEGV in TArray's copy + // constructor (via FString), where the small quantised capacities are + // far likelier to collide with stack garbage than the larger values + // typical of this engine's other element types. + SizeType m_ArrayNum = 0; + SizeType m_ArrayMax = 0; public: // ==================================================================== @@ -552,6 +564,28 @@ namespace OloEngine /** Destructor */ ~TArray() { + // TArray relocates elements BITWISE. ResizeGrow goes through the + // allocator's ResizeAllocation -> FMemory::Realloc, which moves the + // raw byte buffer and never consults any element trait; the + // insert/remove paths use RelocateConstructItems, which memmoves. + // An element type holding a pointer into itself is therefore + // silently corrupted — libstdc++'s std::string does exactly that + // under SSO, which aborted with "free(): invalid pointer". + // + // Ported verbatim in spirit from UE's ~TArray (Array.h), including + // its placement in the destructor and its warning-not-error level: + // + // UE_STATIC_ASSERT_WARN(TIsTriviallyRelocatable_V, + // "TArray can only be used with trivially relocatable types"); + // + // A warning rather than a hard assert because the trait defaults to + // true and is opt-out, so this only fires for types someone has + // explicitly marked non-relocatable — exactly the cases that are + // already broken. Upgrading to a hard error is the right end state + // once the flagged types are fixed. + OLO_STATIC_ASSERT_WARN(TIsTriviallyRelocatable_V, + "TArray can only be used with trivially relocatable types"); + DestructItems(GetData(), m_ArrayNum); // Allocator destructor handles freeing memory } @@ -566,7 +600,18 @@ namespace OloEngine if (this != &Other) { DestructItems(GetData(), m_ArrayNum); - CopyToEmpty(Other.GetData(), Other.Num(), m_ArrayMax); + // Pass 0, not m_ArrayMax. + // + // UE's CopyToEmpty takes a PrevMax parameter and uses it to + // decide whether the existing allocation can be reused. This + // port's CopyToEmpty instead takes ExtraSlack and computes + // `NewMax = Count + ExtraSlack`, which is what the two + // slack-taking constructors above rely on. Handing it + // m_ArrayMax under those semantics asked for + // `Count + current capacity` — so every copy-assignment grew + // the allocation by the current capacity, without bound, for + // arrays that are assigned repeatedly. + CopyToEmpty(Other.GetData(), Other.Num(), 0); } return *this; } diff --git a/OloEngine/src/OloEngine/Containers/CompactSet.h b/OloEngine/src/OloEngine/Containers/CompactSet.h index ce16e7fcd..d2575fbfb 100644 --- a/OloEngine/src/OloEngine/Containers/CompactSet.h +++ b/OloEngine/src/OloEngine/Containers/CompactSet.h @@ -19,6 +19,7 @@ */ #include "OloEngine/Core/Base.h" +#include "OloEngine/Templates/UnrealTypeTraits.h" #include "OloEngine/Containers/ContainerAllocationPolicies.h" #include "OloEngine/Containers/ArrayView.h" #include "OloEngine/Containers/CompactSetBase.h" @@ -322,6 +323,10 @@ namespace OloEngine /** Destructor */ ~TCompactSet() { + // TCompactSet relocates its elements bitwise like the other UE containers. + OLO_STATIC_ASSERT_WARN(TIsTriviallyRelocatable_V, + "This container can only be used with trivially relocatable types"); + Empty(0); } diff --git a/OloEngine/src/OloEngine/Containers/Deque.h b/OloEngine/src/OloEngine/Containers/Deque.h index bb5d06ad9..155e8bd7b 100644 --- a/OloEngine/src/OloEngine/Containers/Deque.h +++ b/OloEngine/src/OloEngine/Containers/Deque.h @@ -4,6 +4,7 @@ #pragma once #include "OloEngine/Core/Base.h" +#include "OloEngine/Templates/UnrealTypeTraits.h" #include "OloEngine/Containers/ContainerAllocationPolicies.h" #include "OloEngine/Memory/MemoryOps.h" #include "OloEngine/Templates/UnrealTemplate.h" @@ -159,6 +160,10 @@ namespace OloEngine ~TDeque() { + // UE guards TDeque with the same assert (Containers/Deque.h). + OLO_STATIC_ASSERT_WARN(TIsTriviallyRelocatable_V, + "This container can only be used with trivially relocatable types"); + Empty(); } diff --git a/OloEngine/src/OloEngine/Containers/SparseArray.h b/OloEngine/src/OloEngine/Containers/SparseArray.h index 9d9060632..56422db96 100644 --- a/OloEngine/src/OloEngine/Containers/SparseArray.h +++ b/OloEngine/src/OloEngine/Containers/SparseArray.h @@ -26,6 +26,7 @@ */ #include "OloEngine/Core/Base.h" +#include "OloEngine/Templates/UnrealTypeTraits.h" #include "OloEngine/Containers/Array.h" #include "OloEngine/Containers/ArrayView.h" #include "OloEngine/Containers/BitArray.h" @@ -691,6 +692,17 @@ namespace OloEngine /** Destructor */ ~TSparseArray() { + // TSparseArray backs TSet, which backs TMap, so this one guard covers all + // three. It checks the USER-FACING element type deliberately: the + // internal storage is a union over TAlignedBytes, which is trivially + // relocatable no matter what the real element is, so a guard on the + // internal type would never fire. + // + // TPair is a TTuple, and TTuple propagates the trait across its + // members, so TMap correctly trips this. + OLO_STATIC_ASSERT_WARN(TIsTriviallyRelocatable_V, + "This container can only be used with trivially relocatable types"); + Empty(); } diff --git a/OloEngine/src/OloEngine/Containers/SparseSetElement.h b/OloEngine/src/OloEngine/Containers/SparseSetElement.h index 623dc5ab9..26761e9df 100644 --- a/OloEngine/src/OloEngine/Containers/SparseSetElement.h +++ b/OloEngine/src/OloEngine/Containers/SparseSetElement.h @@ -12,6 +12,7 @@ */ #include "OloEngine/Containers/SetUtilities.h" +#include "OloEngine/Templates/UnrealTypeTraits.h" #include "OloEngine/Serialization/Archive.h" #include "OloEngine/Templates/UnrealTemplate.h" #include @@ -71,6 +72,25 @@ namespace OloEngine mutable i32 HashIndex; }; + // Propagate relocatability from the wrapped element. + // + // TSparseSet stores TSparseArray>, and TArray relocates + // bitwise. Without this, the wrapper falls through to the permissive default + // (true) and REPORTS ITSELF RELOCATABLE no matter what T is — which silently + // muted ~TSparseArray's guard for every TSet and TMap. That is exactly how + // TMap corrupted Material's uniform tables without any + // compile-time warning: std::string is correctly marked non-relocatable, and + // TTuple propagates it to TPair, but the wrapper threw the information away + // one layer before the guard could see it. + template + struct TIsTriviallyRelocatable> + { + enum + { + Value = TIsTriviallyRelocatable::Value + }; + }; + // ============================================================================ // Internal Helper Functions (matching UE::Core::Private namespace) // ============================================================================ diff --git a/OloEngine/src/OloEngine/Containers/String.h b/OloEngine/src/OloEngine/Containers/String.h new file mode 100644 index 000000000..381ce238e --- /dev/null +++ b/OloEngine/src/OloEngine/Containers/String.h @@ -0,0 +1,722 @@ +#pragma once + +// @file String.h +// @brief FString — a trivially-relocatable string, ported from Unreal Engine. +// +// Ported from UE 5.8's Containers/UnrealString.h.inl, which defines the class +// as a macro-parameterised template instantiated as FString (TCHAR), +// FUtf8String (UTF8CHAR) and FAnsiString (ANSICHAR). This port follows the +// FUtf8String instantiation — a `char` element type — because OloEngine's +// existing string surface (scene YAML, asset paths, ImGui, Lua/C# bindings) is +// UTF-8 `std::string` throughout, so a UTF-8 FString minimises conversion +// friction at the boundaries. +// +// WHY THIS TYPE EXISTS +// -------------------- +// TArray relocates its elements BITWISE: ResizeGrow goes through the +// allocator's ResizeAllocation -> FMemory::Realloc, which moves the raw byte +// buffer; the insert/remove paths memmove via RelocateConstructItems. Neither +// consults any element trait. UE documents this contract explicitly: +// +// "TArray (like many Unreal Engine containers) assumes that the element +// type is trivially relocatable, meaning that elements can safely be +// moved from one location in memory to another by directly copying raw +// bytes." +// +// libstdc++'s std::string violates that contract: under the small-string +// optimisation its internal pointer points into its OWN inline buffer, so a +// bitwise relocation leaves that pointer aimed at the element's old address +// and the destructor frees a non-heap pointer: +// +// free(): invalid pointer (SIGABRT, ~TArray via ~MeshSource) +// +// MSVC's std::string keeps no such self-pointer, which is why this only ever +// aborted against libstdc++. +// +// UE's own string type has no such problem *by construction* — it is a single +// TArray member with NO small-string optimisation, so its pointer +// always targets a separate heap block and byte-copying it is harmless. That +// is precisely why UE can assume relocatability engine-wide. This port keeps +// that property, which is the whole point: FString is safe to store in TArray. +// +// STORAGE INVARIANT (identical to UE) +// ----------------------------------- +// `Data` is either completely empty (Num() == 0, representing "") or holds the +// characters FOLLOWED BY a null terminator, so Num() == Len() + 1. Len() must +// therefore never be `Data.Num()`. + +#include "OloEngine/Containers/Array.h" +#include "OloEngine/Containers/ContainerAllocationPolicies.h" +#include "OloEngine/Core/Base.h" + +#include +#include +#include +#include +#include +#include + +namespace OloEngine +{ + // NAMING EXCEPTION. This is a port of UE's FString, and its members and + // constants deliberately keep UE's spelling (`Data`, `InvalidIndex`) + // rather than the engine's usual `m_PascalCase` / `k`-prefixed forms. The + // point of a port is that UE source and documentation can be read against + // it directly; renaming the members breaks that for a cosmetic gain. New + // engine types outside this file follow the normal conventions. + class FString + { + public: + // Matches UE: TSizedDefaultAllocator<32> — the same allocator TArray + // uses elsewhere in the engine. + using AllocatorType = TSizedDefaultAllocator<32>; + using ElementType = char; + + private: + // Array holding the character data (UE's member name and layout). + using DataType = TArray; + DataType Data; + + using SizeType = typename DataType::SizeType; + + // Debug-only check of the storage invariant described in the header + // comment. UE has the equivalent as CheckInvariants(). + void CheckInvariants() const + { + OLO_CORE_ASSERT(Data.Num() == 0 || Data.Last() == '\0', + "FString storage must be empty or null-terminated"); + } + + public: + FString() = default; + FString(const FString&) = default; + FString(FString&&) noexcept = default; + FString& operator=(const FString&) = default; + FString& operator=(FString&&) noexcept = default; + ~FString() = default; + + // ------------------------------------------------------------------ + // Construction + // ------------------------------------------------------------------ + + FString(const char* src) + { + if (src != nullptr && *src != '\0') + { + const sizet len = std::strlen(src); + ConstructFromPtrSize(src, static_cast(len)); + } + } + + FString(const char* src, SizeType count) + { + ConstructFromPtrSize(src, count); + } + + FString(std::string_view src) + { + ConstructFromPtrSize(src.data(), static_cast(src.size())); + } + + FString(const std::string& src) + { + ConstructFromPtrSize(src.data(), static_cast(src.size())); + } + + // ------------------------------------------------------------------ + // Size / capacity + // ------------------------------------------------------------------ + + // Length in characters, EXCLUDING the stored null terminator. + [[nodiscard("Store this!")]] SizeType Len() const + { + return Data.Num() ? Data.Num() - 1 : 0; + } + + [[nodiscard("Store this!")]] bool IsEmpty() const + { + return Data.Num() <= 1; + } + + [[nodiscard("Store this!")]] bool IsValidIndex(SizeType index) const + { + return index >= 0 && index < Len(); + } + + void Empty(SizeType slack = 0) + { + Data.Empty(slack ? slack + 1 : 0); + } + + void Reset(SizeType newSize = 0) + { + Data.Reset(newSize ? newSize + 1 : 0); + } + + void Reserve(SizeType characters) + { + if (characters > 0) + Data.Reserve(characters + 1); + } + + void Shrink() + { + Data.Shrink(); + } + + [[nodiscard("Store this!")]] u32 GetAllocatedSize() const + { + return Data.GetAllocatedSize(); + } + + // ------------------------------------------------------------------ + // Element / buffer access + // ------------------------------------------------------------------ + + // UE spells the raw-pointer accessor `*Str`. Always returns a valid + // null-terminated buffer, even when empty. + [[nodiscard("Store this!")]] const char* operator*() const + { + return Data.Num() ? Data.GetData() : ""; + } + + [[nodiscard("Store this!")]] const char* GetData() const + { + return **this; + } + + [[nodiscard("Store this!")]] DataType& GetCharArray() + { + return Data; + } + [[nodiscard("Store this!")]] const DataType& GetCharArray() const + { + return Data; + } + + [[nodiscard("Store this!")]] char& operator[](SizeType index) + { + OLO_CORE_ASSERT(IsValidIndex(index), "FString index out of range"); + return Data.GetData()[index]; + } + + [[nodiscard("Store this!")]] const char& operator[](SizeType index) const + { + OLO_CORE_ASSERT(IsValidIndex(index), "FString index out of range"); + return Data.GetData()[index]; + } + + // ------------------------------------------------------------------ + // Interop with the std:: string surface the engine already uses + // ------------------------------------------------------------------ + + [[nodiscard("Store this!")]] std::string ToStdString() const + { + return std::string(**this, static_cast(Len())); + } + + [[nodiscard("Store this!")]] std::string_view ToView() const + { + return std::string_view(**this, static_cast(Len())); + } + + explicit operator std::string() const + { + return ToStdString(); + } + explicit operator std::string_view() const + { + return ToView(); + } + + // ------------------------------------------------------------------ + // Append / concatenation + // ------------------------------------------------------------------ + + FString& AppendChars(const char* str, SizeType count) + { + if (!str || count <= 0) + return *this; + + const SizeType oldLen = Len(); + + // `str` may point INTO our own buffer — `s += s`, or a + // std::string_view taken over this string. The growth below goes + // through FMemory::Realloc, which moves the raw byte buffer, so a + // pointer captured before it dangles afterwards and the copy then + // reads freed heap. Capture the offset while the old buffer is + // still valid and re-derive the pointer after the growth instead + // of carrying one across it. + // + // std::less/std::greater_equal rather than raw < and >=: relational + // comparison of pointers that do not point into the same array is + // UNSPECIFIED behaviour, and an external `str` is exactly that. The + // std:: function objects are required to impose a total order over + // all pointers of a type, so the range test is well-defined however + // `str` was obtained -- and stays O(1), unlike scanning the buffer + // for a matching address. + const char* const oldBegin = Data.GetData(); + const char* const oldEnd = oldBegin + Data.Num(); + const bool selfAliased = (oldBegin != nullptr) && + std::greater_equal{}(str, oldBegin) && + std::less{}(str, oldEnd); + const SizeType srcOffset = selfAliased ? static_cast(str - oldBegin) : 0; + + // +1 for the terminator; Data may be completely empty here. + Data.SetNumUninitialized(oldLen + count + 1); + + // memmove, not memcpy: once re-derived, source and destination are + // in the SAME buffer. They cannot actually overlap — the source is + // a sub-range of [0, oldLen] and the destination starts at oldLen, + // so the two are adjacent at worst — but memmove costs nothing + // here and removes the need to re-derive that argument every time + // this is read. + const char* const src = selfAliased ? (Data.GetData() + srcOffset) : str; + std::memmove(Data.GetData() + oldLen, src, static_cast(count)); + Data.GetData()[oldLen + count] = '\0'; + CheckInvariants(); + return *this; + } + + FString& AppendChar(char c) + { + return AppendChars(&c, 1); + } + + FString& Append(const FString& other) + { + return AppendChars(*other, other.Len()); + } + FString& Append(const char* str) + { + return AppendChars(str, str ? static_cast(std::strlen(str)) : 0); + } + FString& Append(std::string_view sv) + { + return AppendChars(sv.data(), static_cast(sv.size())); + } + + FString& operator+=(const FString& other) + { + return Append(other); + } + FString& operator+=(const char* str) + { + return Append(str); + } + FString& operator+=(std::string_view sv) + { + return Append(sv); + } + FString& operator+=(char c) + { + return AppendChar(c); + } + + [[nodiscard("Store this!")]] friend FString operator+(FString lhs, const FString& rhs) + { + lhs.Append(rhs); + return lhs; + } + + [[nodiscard("Store this!")]] friend FString operator+(FString lhs, const char* rhs) + { + lhs.Append(rhs); + return lhs; + } + + [[nodiscard("Store this!")]] friend FString operator+(const char* lhs, const FString& rhs) + { + FString result(lhs); + result.Append(rhs); + return result; + } + + // ------------------------------------------------------------------ + // Comparison + // ------------------------------------------------------------------ + + // DELIBERATE DEVIATION FROM UE. Every search/compare entry point here + // defaults to CaseSensitive. UE's FString splits it: Equals/Compare + // default to CaseSensitive, but Find/Contains/StartsWith/EndsWith + // default to IgnoreCase — so in UE `Contains` quietly matches "FOO" + // against "foo" while `Equals` does not. + // + // A uniform default is chosen over UE parity because the surprising + // direction is the dangerous one: a case-insensitive match that the + // caller did not ask for silently accepts input it should reject, and + // reads identically at the call site. Callers wanting UE's behaviour + // pass ESearchCase::IgnoreCase explicitly. + enum class ESearchCase + { + CaseSensitive, + IgnoreCase + }; + + [[nodiscard("Store this!")]] bool Equals(const FString& other, ESearchCase cs = ESearchCase::CaseSensitive) const + { + if (Len() != other.Len()) + return false; + return Compare(other, cs) == 0; + } + + [[nodiscard("Store this!")]] i32 Compare(const FString& other, ESearchCase cs = ESearchCase::CaseSensitive) const + { + // Length-aware, NOT strcmp. FString can hold embedded NULs — the + // counted and string_view constructors take an explicit size and + // never stop at one — so a terminator-driven compare would call + // "a\0b" and "a\0c" equal: it stops at index 1, and Equals' length + // guard passes because both are 3 long. Compare exactly Len() + // bytes, then let the shorter string sort first. + const SizeType lenA = Len(); + const SizeType lenB = other.Len(); + const SizeType common = lenA < lenB ? lenA : lenB; + const char* a = **this; + const char* b = *other; + + if (cs == ESearchCase::CaseSensitive) + { + if (common > 0) + { + if (const int diff = std::memcmp(a, b, static_cast(common)); diff != 0) + return diff; + } + } + else + { + for (SizeType i = 0; i < common; ++i) + { + const i32 ca = ToLowerChar(a[i]); + if (const i32 cb = ToLowerChar(b[i]); ca != cb) + return ca - cb; + } + } + + if (lenA == lenB) + return 0; + return lenA < lenB ? -1 : 1; + } + + [[nodiscard("Store this!")]] friend bool operator==(const FString& lhs, const FString& rhs) + { + return lhs.Equals(rhs); + } + [[nodiscard("Store this!")]] friend bool operator==(const FString& lhs, const char* rhs) + { + // Same length-aware path as the FString/FString overload. A raw + // C string cannot carry an embedded NUL, so its length IS its + // strlen — but lhs can, and strcmp would ignore everything past + // lhs's first one. A null rhs compares as the empty string. + return lhs.Equals(FString(rhs ? rhs : "")); + } + [[nodiscard("Store this!")]] friend bool operator<(const FString& lhs, const FString& rhs) + { + return lhs.Compare(rhs) < 0; + } + + // ------------------------------------------------------------------ + // Search + // ------------------------------------------------------------------ + + static constexpr SizeType InvalidIndex = -1; + + [[nodiscard("Store this!")]] SizeType Find(std::string_view sub, ESearchCase cs = ESearchCase::CaseSensitive, + SizeType startPos = 0) const + { + if (sub.empty()) + return InvalidIndex; + const SizeType len = Len(); + const SizeType subLen = static_cast(sub.size()); + if (subLen > len) + return InvalidIndex; + + for (SizeType i = (startPos < 0 ? 0 : startPos); i + subLen <= len; ++i) + { + bool match = true; + for (SizeType j = 0; j < subLen; ++j) + { + const char a = (*this)[i + j]; + const char b = sub[static_cast(j)]; + const bool same = (cs == ESearchCase::CaseSensitive) ? (a == b) + : (ToLowerChar(a) == ToLowerChar(b)); + if (!same) + { + match = false; + break; + } + } + if (match) + return i; + } + return InvalidIndex; + } + + [[nodiscard("Store this!")]] bool Contains(std::string_view sub, ESearchCase cs = ESearchCase::CaseSensitive) const + { + return Find(sub, cs) != InvalidIndex; + } + + [[nodiscard("Store this!")]] bool FindChar(char c, SizeType& outIndex) const + { + const SizeType len = Len(); + for (SizeType i = 0; i < len; ++i) + { + if ((*this)[i] == c) + { + outIndex = i; + return true; + } + } + outIndex = InvalidIndex; + return false; + } + + [[nodiscard("Store this!")]] bool FindLastChar(char c, SizeType& outIndex) const + { + for (SizeType i = Len() - 1; i >= 0; --i) + { + if ((*this)[i] == c) + { + outIndex = i; + return true; + } + } + outIndex = InvalidIndex; + return false; + } + + [[nodiscard("Store this!")]] bool StartsWith(std::string_view prefix, ESearchCase cs = ESearchCase::CaseSensitive) const + { + const SizeType n = static_cast(prefix.size()); + if (n > Len()) + return false; + for (SizeType i = 0; i < n; ++i) + { + const char a = (*this)[i]; + const char b = prefix[static_cast(i)]; + if (cs == ESearchCase::CaseSensitive ? (a != b) : (ToLowerChar(a) != ToLowerChar(b))) + return false; + } + return true; + } + + [[nodiscard("Store this!")]] bool EndsWith(std::string_view suffix, ESearchCase cs = ESearchCase::CaseSensitive) const + { + const SizeType n = static_cast(suffix.size()); + const SizeType len = Len(); + if (n > len) + return false; + for (SizeType i = 0; i < n; ++i) + { + const char a = (*this)[len - n + i]; + const char b = suffix[static_cast(i)]; + if (cs == ESearchCase::CaseSensitive ? (a != b) : (ToLowerChar(a) != ToLowerChar(b))) + return false; + } + return true; + } + + // ------------------------------------------------------------------ + // Substrings + // ------------------------------------------------------------------ + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString Left(SizeType count) const + { + return FString(**this, Clamp(count, 0, Len())); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString Right(SizeType count) const + { + const SizeType len = Len(); + const SizeType n = Clamp(count, 0, len); + return FString(**this + (len - n), n); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString Mid(SizeType start, SizeType count = MAX_i32) const + { + const SizeType len = Len(); + if (start >= len || count <= 0) + return FString(); + const SizeType begin = Clamp(start, 0, len); + const SizeType n = Clamp(count, 0, len - begin); + return FString(**this + begin, n); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString LeftChop(SizeType count) const + { + return Left(Len() - Clamp(count, 0, Len())); + } + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString RightChop(SizeType count) const + { + return Mid(Clamp(count, 0, Len())); + } + + // ------------------------------------------------------------------ + // Case / trimming + // ------------------------------------------------------------------ + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString ToUpper() const + { + FString out(*this); + out.ToUpperInline(); + return out; + } + + void ToUpperInline() + { + const SizeType len = Len(); + char* p = Data.GetData(); + for (SizeType i = 0; i < len; ++i) + p[i] = ToUpperChar(p[i]); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString ToLower() const + { + FString out(*this); + out.ToLowerInline(); + return out; + } + + void ToLowerInline() + { + const SizeType len = Len(); + char* p = Data.GetData(); + for (SizeType i = 0; i < len; ++i) + p[i] = ToLowerChar(p[i]); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString TrimStart() const + { + SizeType i = 0; + const SizeType len = Len(); + while (i < len && IsSpaceChar((*this)[i])) + ++i; + return Mid(i); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString TrimEnd() const + { + SizeType end = Len(); + while (end > 0 && IsSpaceChar((*this)[end - 1])) + --end; + return Left(end); + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] FString TrimStartAndEnd() const + { + return TrimStart().TrimEnd(); + } + + // ------------------------------------------------------------------ + // Split / formatting + // ------------------------------------------------------------------ + + // UE semantics: returns true and fills the out params when `separator` + // is found; otherwise returns false and leaves them untouched. + bool Split(std::string_view separator, FString* outLeft, FString* outRight, + ESearchCase cs = ESearchCase::CaseSensitive) const + { + const SizeType idx = Find(separator, cs); + if (idx == InvalidIndex) + return false; + if (outLeft) + *outLeft = Left(idx); + if (outRight) + *outRight = Mid(idx + static_cast(separator.size())); + return true; + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] static FString Printf(const char* fmt, ...) + { + va_list args; + va_start(args, fmt); + va_list copy; + va_copy(copy, args); + const int needed = std::vsnprintf(nullptr, 0, fmt, copy); + va_end(copy); + + FString result; + if (needed > 0) + { + result.Data.SetNumUninitialized(needed + 1); + std::vsnprintf(result.Data.GetData(), static_cast(needed) + 1, fmt, args); + result.Data.GetData()[needed] = '\0'; + } + va_end(args); + return result; + } + + [[nodiscard("returns a NEW string; the original is unchanged — the *Inline variants modify in place")]] static FString FromInt(i64 value) + { + return Printf("%lld", static_cast(value)); + } + + private: + void ConstructFromPtrSize(const char* src, SizeType count) + { + if (!src || count <= 0) + return; + Data.SetNumUninitialized(count + 1); + std::memcpy(Data.GetData(), src, static_cast(count)); + Data.GetData()[count] = '\0'; + CheckInvariants(); + } + + // ASCII, deliberately — not . + // + // std::toupper/tolower/isspace are LOCALE-SENSITIVE. Under a Turkish + // locale std::toupper('i') is not 'I', so a case-insensitive compare + // of engine identifiers (asset names, shader uniforms, scene keys) + // would change meaning with the user's system locale. They also take + // and return int, which is what makes `&&`-ing them read as a bool + // when it is not. Explicit ASCII is both correct here and free of + // that whole class of problem. + [[nodiscard("Store this!")]] static constexpr char ToLowerChar(char c) noexcept + { + return (c >= 'A' && c <= 'Z') ? static_cast(c - 'A' + 'a') : c; + } + + [[nodiscard("Store this!")]] static constexpr char ToUpperChar(char c) noexcept + { + return (c >= 'a' && c <= 'z') ? static_cast(c - 'a' + 'A') : c; + } + + // Matches std::isspace's default ("C" locale) set exactly. + [[nodiscard("Store this!")]] static constexpr bool IsSpaceChar(char c) noexcept + { + return c == ' ' || c == '\t' || c == '\n' || c == '\v' || c == '\f' || c == '\r'; + } + + [[nodiscard("Store this!")]] static SizeType Clamp(SizeType v, SizeType lo, SizeType hi) + { + return v < lo ? lo : (v > hi ? hi : v); + } + }; + + // FString is a single TArray member, which is itself trivially relocatable + // (pointer + two integers, pointing at a SEPARATE heap block). Stating it + // explicitly documents the property this whole type exists to provide, and + // means TArray is safe where TArray is not. + template<> + struct TIsTriviallyRelocatable + { + enum + { + Value = true + }; + }; + + [[nodiscard("Store this!")]] inline u32 GetTypeHash(const FString& s) + { + return static_cast(std::hash{}(s.ToView())); + } +} // namespace OloEngine + +template<> +struct std::hash +{ + [[nodiscard("Store this!")]] std::size_t operator()(const OloEngine::FString& s) const noexcept + { + return std::hash{}(s.ToView()); + } +}; diff --git a/OloEngine/src/OloEngine/Core/YAMLConverters.h b/OloEngine/src/OloEngine/Core/YAMLConverters.h index 33c04e39c..790a353c1 100644 --- a/OloEngine/src/OloEngine/Core/YAMLConverters.h +++ b/OloEngine/src/OloEngine/Core/YAMLConverters.h @@ -2,6 +2,7 @@ #include "OloEngine/Core/UUID.h" #include "OloEngine/Asset/Asset.h" +#include "OloEngine/Containers/String.h" #include #include @@ -314,6 +315,18 @@ namespace YAML #ifndef OLOENGINE_YAML_EMITTER_GLM_DEFINED #define OLOENGINE_YAML_EMITTER_GLM_DEFINED + // FString emits as a plain scalar, exactly like std::string. Without this + // every FString-keyed map (Material's uniform tables, for one) fails to + // serialize with "no match for operator<<". + // + // Uses the raw buffer rather than ToStdString() so emitting a key costs no + // allocation — `*Str` is always a valid null-terminated pointer. + inline Emitter& operator<<(Emitter& out, const OloEngine::FString& s) + { + out << *s; + return out; + } + inline Emitter& operator<<(Emitter& out, const glm::vec2& v) { out << Flow; diff --git a/OloEngine/src/OloEngine/Renderer/AtmosphereSky.cpp b/OloEngine/src/OloEngine/Renderer/AtmosphereSky.cpp index ca8d9236f..a86155d03 100644 --- a/OloEngine/src/OloEngine/Renderer/AtmosphereSky.cpp +++ b/OloEngine/src/OloEngine/Renderer/AtmosphereSky.cpp @@ -19,55 +19,93 @@ namespace OloEngine { namespace { - [[nodiscard]] f32 SanitizeClamped(f32 v, f32 lo, f32 hi, f32 fallback) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 SanitizeClamped(f32 v, f32 lo, f32 hi, f32 fallback) { return std::isfinite(v) ? std::clamp(v, lo, hi) : fallback; } // ── CPU mirrors of the AtmosphereSky.glsl night-layer helpers ── - // Structurally identical, not bit-exact (same contract as StarNestSky's - // CPU evaluator). Keep BOTH sides in sync — the shader names each - // mirrored function. + // Keep BOTH sides in sync — the shader names each mirrored function. + // + // The hash chain below IS bit-exact against the shader (integer ops + // only), so which lattice cell holds a star, and where in that cell it + // sits, agree between the CPU and every GPU vendor. The surrounding + // float math (cos/sin/pow/smoothstep/length) is still only + // structurally identical, so brightness may differ in the last ULP — + // that is a smooth, sub-quantisation difference, not a star moving. + + // Integer bit-mixer (PCG output permutation). Mirrors pcgHash + // (AtmosphereSky.glsl) EXACTLY — unsigned wraparound, shift and xor are + // bit-defined in both languages, so the CPU and every GPU vendor agree. + // + // The previous `fract(sin(dot(p, k)) * 43758.5453)` hash could not + // deliver that. It evaluates sin() at arguments in the tens of + // thousands, where a 1-ULP input difference shifts the result by a + // large fraction of a period, and the *= 43758 + fract() then amplified + // that into an unrelated value. NVIDIA and Mesa disagreed on star + // positions outright, and std::sin here matched neither — so this + // "mirror" was unverifiable in principle, not merely in practice. + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] u32 PcgHash(u32 v) + { + const u32 state = v * 747796405u + 2891336453u; + const u32 word = ((state >> ((state >> 28u) + 4u)) ^ state) * 277803737u; + return (word >> 22u) ^ word; + } - [[nodiscard]] f32 Hash13(const glm::vec3& p) + // Mirrors hashCell (AtmosphereSky.glsl). Integer lattice cell -> u32. + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] u32 HashCell(const glm::ivec3& c, u32 seed) { - const f32 h = std::sin(glm::dot(p, glm::vec3(127.1f, 311.7f, 74.7f))) * 43758.5453f; - return h - std::floor(h); + u32 h = PcgHash(static_cast(c.x) ^ 0x9E3779B9u); + h = PcgHash(h ^ static_cast(c.y) ^ 0x85EBCA6Bu); + h = PcgHash(h ^ static_cast(c.z) ^ 0xC2B2AE35u); + return PcgHash(h ^ seed); + } + + // Mirrors hash1 (AtmosphereSky.glsl). Result in [0,1). + // Masked to 24 bits so the u32 -> f32 conversion is exact (an f32 + // mantissa holds 24 bits) and scaled by a power of two so the divide + // rounds nothing. + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 Hash1(const glm::ivec3& c, u32 seed) + { + return static_cast(HashCell(c, seed) & 0xFFFFFFu) * (1.0f / 16777216.0f); } - [[nodiscard]] glm::vec3 Hash33(const glm::vec3& p) + // Mirrors hash3 (AtmosphereSky.glsl). + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] glm::vec3 Hash3(const glm::ivec3& c) { - return { Hash13(p), Hash13(p + glm::vec3(19.19f, 0.0f, 0.0f)), - Hash13(p + glm::vec3(0.0f, 47.31f, 0.0f)) }; + return { Hash1(c, 0u), Hash1(c, 1u), Hash1(c, 2u) }; } - [[nodiscard]] f32 SmoothStepF(f32 edge0, f32 edge1, f32 x) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 SmoothStepF(f32 edge0, f32 edge1, f32 x) { const f32 t = std::clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); return t * t * (3.0f - 2.0f * t); } // Mirrors starField() in AtmosphereSky.glsl. - [[nodiscard]] f32 StarField(const glm::vec3& dir, f32 rotation, f32 intensity) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 StarField(const glm::vec3& dir, f32 rotation, f32 intensity) { const f32 c = std::cos(rotation); const f32 s = std::sin(rotation); const glm::vec3 d(c * dir.x + s * dir.z, dir.y, -s * dir.x + c * dir.z); + // `dir` is unit, so p stays within +/-60 and the cell index + // converts to int exactly. const glm::vec3 p = d * 60.0f; - const glm::vec3 cell = glm::floor(p); - const glm::vec3 f = p - cell; - const glm::vec3 starPos = Hash33(cell); + const glm::vec3 cellF = glm::floor(p); + const glm::ivec3 cell(cellF); + const glm::vec3 f = p - cellF; + const glm::vec3 starPos = Hash3(cell); const f32 dist = glm::length(f - starPos); // Sparse bright stars: gate most cells off, sharpen the rest. - const f32 lum = std::pow(Hash13(cell + glm::vec3(17.0f)), 14.0f); + const f32 lum = std::pow(Hash1(cell, 3u), 14.0f); const f32 star = SmoothStepF(0.18f, 0.0f, dist) * lum; return star * intensity * 60.0f; } // Mirrors nightLayer() in AtmosphereSky.glsl: base gradient + moon // glow + moon disk + stars, all scaled by the night brightness lane. - [[nodiscard]] glm::vec3 NightLayer(const glm::vec3& dir, const AtmosphereSkyUBO& ubo) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] glm::vec3 NightLayer(const glm::vec3& dir, const AtmosphereSkyUBO& ubo) { const glm::vec3 moonDir(ubo.MoonDirection); const f32 starIntensity = ubo.NightParams.y; @@ -107,13 +145,13 @@ namespace OloEngine } // Mirrors the 2D value-noise FBM cloud tint in AtmosphereSky.glsl. - [[nodiscard]] f32 Hash12(const glm::vec2& p) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 Hash12(const glm::vec2& p) { const f32 h = std::sin(glm::dot(p, glm::vec2(127.1f, 311.7f))) * 43758.5453f; return h - std::floor(h); } - [[nodiscard]] f32 ValueNoise2D(const glm::vec2& p) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 ValueNoise2D(const glm::vec2& p) { const glm::vec2 i = glm::floor(p); const glm::vec2 f = p - i; @@ -125,7 +163,7 @@ namespace OloEngine return glm::mix(glm::mix(a, b, u.x), glm::mix(c, d, u.x), u.y); } - [[nodiscard]] f32 CloudFBM(const glm::vec2& p) + [[nodiscard("pure computation; discarding the result makes the call a no-op")]] f32 CloudFBM(const glm::vec2& p) { f32 sum = 0.0f; f32 amp = 0.5f; diff --git a/OloEngine/src/OloEngine/Renderer/Material.h b/OloEngine/src/OloEngine/Renderer/Material.h index 133612feb..a1716dd3b 100644 --- a/OloEngine/src/OloEngine/Renderer/Material.h +++ b/OloEngine/src/OloEngine/Renderer/Material.h @@ -2,6 +2,7 @@ #include "OloEngine/Core/Base.h" #include "OloEngine/Containers/Map.h" +#include "OloEngine/Containers/String.h" #include "OloEngine/Renderer/RendererResource.h" #include "OloEngine/Renderer/Shader.h" #include "OloEngine/Renderer/Texture.h" @@ -409,59 +410,59 @@ namespace OloEngine } // Accessors for serialization - const TMap& GetFloatUniforms() const + const TMap& GetFloatUniforms() const { return m_FloatUniforms; } - const TMap& GetIntUniforms() const + const TMap& GetIntUniforms() const { return m_IntUniforms; } - const TMap& GetUIntUniforms() const + const TMap& GetUIntUniforms() const { return m_UIntUniforms; } - const TMap& GetBoolUniforms() const + const TMap& GetBoolUniforms() const { return m_BoolUniforms; } - const TMap& GetVec2Uniforms() const + const TMap& GetVec2Uniforms() const { return m_Vec2Uniforms; } - const TMap& GetVec3Uniforms() const + const TMap& GetVec3Uniforms() const { return m_Vec3Uniforms; } - const TMap& GetVec4Uniforms() const + const TMap& GetVec4Uniforms() const { return m_Vec4Uniforms; } - const TMap& GetIVec2Uniforms() const + const TMap& GetIVec2Uniforms() const { return m_IVec2Uniforms; } - const TMap& GetIVec3Uniforms() const + const TMap& GetIVec3Uniforms() const { return m_IVec3Uniforms; } - const TMap& GetIVec4Uniforms() const + const TMap& GetIVec4Uniforms() const { return m_IVec4Uniforms; } - const TMap& GetMat3Uniforms() const + const TMap& GetMat3Uniforms() const { return m_Mat3Uniforms; } - const TMap& GetMat4Uniforms() const + const TMap& GetMat4Uniforms() const { return m_Mat4Uniforms; } - const TMap>& GetTexture2DUniforms() const + const TMap>& GetTexture2DUniforms() const { return m_Texture2DUniforms; } - const TMap>& GetTextureCubeUniforms() const + const TMap>& GetTextureCubeUniforms() const { return m_TextureCubeUniforms; } @@ -479,20 +480,20 @@ namespace OloEngine // Material properties storage (uniform system) // Using TMap for better cache performance on hot path (every draw call) - TMap m_FloatUniforms; - TMap m_IntUniforms; - TMap m_UIntUniforms; - TMap m_BoolUniforms; - TMap m_Vec2Uniforms; - TMap m_Vec3Uniforms; - TMap m_Vec4Uniforms; - TMap m_IVec2Uniforms; - TMap m_IVec3Uniforms; - TMap m_IVec4Uniforms; - TMap m_Mat3Uniforms; - TMap m_Mat4Uniforms; - TMap> m_Texture2DUniforms; - TMap> m_TextureCubeUniforms; + TMap m_FloatUniforms; + TMap m_IntUniforms; + TMap m_UIntUniforms; + TMap m_BoolUniforms; + TMap m_Vec2Uniforms; + TMap m_Vec3Uniforms; + TMap m_Vec4Uniforms; + TMap m_IVec2Uniforms; + TMap m_IVec3Uniforms; + TMap m_IVec4Uniforms; + TMap m_Mat3Uniforms; + TMap m_Mat4Uniforms; + TMap> m_Texture2DUniforms; + TMap> m_TextureCubeUniforms; // ===================================================================== // PRIVATE MATERIAL PROPERTIES (Encapsulated) diff --git a/OloEngine/src/OloEngine/Renderer/MeshSource.h b/OloEngine/src/OloEngine/Renderer/MeshSource.h index 0d7801c8d..11b08a289 100644 --- a/OloEngine/src/OloEngine/Renderer/MeshSource.h +++ b/OloEngine/src/OloEngine/Renderer/MeshSource.h @@ -13,6 +13,7 @@ #include "OloEngine/Renderer/IndexBuffer.h" #include "OloEngine/Containers/Array.h" +#include "OloEngine/Containers/String.h" #include "OloEngine/Containers/Map.h" #include @@ -42,8 +43,13 @@ namespace OloEngine u32 m_IndexCount = 0; u32 m_VertexCount = 0; - // Variable-sized members and bool at the end - std::string m_NodeName, m_MeshName; + // Variable-sized members and bool at the end. + // + // FString, not std::string: Submesh lives in a TArray, which relocates + // its elements bitwise (see Containers/String.h). libstdc++'s + // std::string points into its own SSO buffer and does not survive that + // — it aborted with "free(): invalid pointer" in ~TArray. + FString m_NodeName, m_MeshName; bool m_IsRigged = false; // Static assertions to verify expected size optimization diff --git a/OloEngine/src/OloEngine/Renderer/Model.cpp b/OloEngine/src/OloEngine/Renderer/Model.cpp index a511eae8d..7d2ba9d9a 100644 --- a/OloEngine/src/OloEngine/Renderer/Model.cpp +++ b/OloEngine/src/OloEngine/Renderer/Model.cpp @@ -792,7 +792,7 @@ namespace OloEngine const bool inRange = matIdx < m_Materials.size(); const std::string matName = (inRange && m_Materials[matIdx]) ? m_Materials[matIdx]->GetName() : ""; OLO_CORE_INFO("Model: cache submesh[{}] '{}' -> matIdx={} ({})", - i, sub.m_NodeName.empty() ? "" : sub.m_NodeName, + i, sub.m_NodeName.IsEmpty() ? "" : *sub.m_NodeName, matIdx, matName); } } diff --git a/OloEngine/src/OloEngine/Renderer/Passes/WaterRenderPass.cpp b/OloEngine/src/OloEngine/Renderer/Passes/WaterRenderPass.cpp index b1770f69a..9350cd385 100644 --- a/OloEngine/src/OloEngine/Renderer/Passes/WaterRenderPass.cpp +++ b/OloEngine/src/OloEngine/Renderer/Passes/WaterRenderPass.cpp @@ -51,10 +51,37 @@ namespace OloEngine [[maybe_unused]] const auto sceneColorRead = builder.Read(board.Scene.SceneColorTexture, RGReadUsage::ShaderSample); } - if (board.Scene.SceneDepthAttachment.IsValid()) + // Sample the SNAPSHOT depth, never the live attachment. + // + // This pass renders INTO SceneColor (WriteNewVersion above), and + // SceneDepthAttachment is that same framebuffer's depth attachment. + // Sampling a texture while it is attached to the bound framebuffer is a + // feedback loop -- undefined behaviour in GL, not merely discouraged. + // + // Water.glsl leans on this sample hard: it reconstructs the floor + // behind the surface (`hasFloorBehind`, `viewPosFromDepth`) to refract + // it. With garbage depth the refraction shows the floor at full + // strength, which on Mesa/radeonsi made the magenta seafloor 20 units + // below the surface read straight through the water in the near field. + // NVIDIA happened to return usable values from the loop, so it only + // ever surfaced on AMD. + // + // Scene.SceneDepth is the semantic snapshot for exactly this purpose, + // and is what ContactShadowRenderPass and FogRenderPass already read. + // No fallback to SceneDepthAttachment. An earlier version fell back to + // it when the snapshot was missing, which contradicts the paragraph + // above: the fallback IS the live attachment, so the "never sample the + // live attachment" rule held only while the snapshot happened to be + // published. Absent the snapshot, Execute's `depthTextureID == 0u` + // guard drops the draw for this frame — losing the water is a visible, + // debuggable outcome, whereas a feedback loop is undefined behaviour + // that reads correctly on one vendor and shows the seafloor through + // the surface on another. + if (board.Scene.SceneDepth.IsValid()) { - m_SelectedSceneDepthTexture = board.Scene.SceneDepthAttachment; - [[maybe_unused]] const auto sceneDepthRead = builder.Read(board.Scene.SceneDepthAttachment, RGReadUsage::ShaderSample); + m_SelectedSceneDepthTexture = board.Scene.SceneDepth; + [[maybe_unused]] const auto sceneDepthRead = + builder.Read(board.Scene.SceneDepth, RGReadUsage::ShaderSample); } if (board.Scene.SceneViewNormals.IsValid()) diff --git a/OloEngine/src/OloEngine/Renderer/Renderer3DMeshSubmission.cpp b/OloEngine/src/OloEngine/Renderer/Renderer3DMeshSubmission.cpp index 2d7b95469..514400792 100644 --- a/OloEngine/src/OloEngine/Renderer/Renderer3DMeshSubmission.cpp +++ b/OloEngine/src/OloEngine/Renderer/Renderer3DMeshSubmission.cpp @@ -45,10 +45,13 @@ namespace OloEngine return nullptr; const Submesh& submesh = submeshes[submeshIndex]; - if (!submesh.m_NodeName.empty()) - return submesh.m_NodeName.c_str(); - if (!submesh.m_MeshName.empty()) - return submesh.m_MeshName.c_str(); + // UE spells the raw-buffer accessor `*Str`; it always returns a + // valid null-terminated pointer, and the storage outlives the call + // because `submesh` is a reference into the MeshSource's array. + if (!submesh.m_NodeName.IsEmpty()) + return *submesh.m_NodeName; + if (!submesh.m_MeshName.IsEmpty()) + return *submesh.m_MeshName; return nullptr; } } // namespace @@ -1128,6 +1131,16 @@ namespace OloEngine return nullptr; } + // Validate the GPU resources BEFORE reserving anything for this draw. + // Both the bone-matrix reservations below and the command packet come + // out of the per-frame arena, and bailing after them strands that space + // until the frame ends — every frame, for a mesh whose resources never + // become valid. + const RHI::ResourceHandle vertexArrayID = vertexArray->GetRHIHandle(); + const RHI::ResourceHandle shaderRendererID = shaderToUse->GetRHIHandle(); + if (!ValidateDrawMeshResources("Renderer3D::DrawAnimatedMesh", vertexArrayID, shaderRendererID)) + return nullptr; + // Allocate space in FrameDataBuffer for bone matrices. FrameDataBuffer& frameBuffer = FrameDataBufferManager::Get(); u32 boneCount = static_cast(boneMatrices.size()); @@ -1169,11 +1182,6 @@ namespace OloEngine auto* cmd = packet->GetCommandData(); cmd->header.type = CommandType::DrawMesh; - const RHI::ResourceHandle vertexArrayID = vertexArray->GetRHIHandle(); - const RHI::ResourceHandle shaderRendererID = shaderToUse->GetRHIHandle(); - if (!ValidateDrawMeshResources("Renderer3D::DrawAnimatedMesh", vertexArrayID, shaderRendererID)) - return nullptr; - // Store asset handles and renderer IDs (POD). cmd->meshHandle = mesh->GetHandle(); cmd->vertexArrayID = vertexArrayID; @@ -1772,6 +1780,25 @@ namespace OloEngine return nullptr; } + // Validate BEFORE reserving worker scratch or a packet — same reasoning + // as the non-parallel path: both come out of per-frame storage that a + // late bail-out strands for the rest of the frame. + // + // The null check mirrors DrawAnimatedMesh's. ValidateDrawMeshResources + // takes handles, so it cannot catch an absent vertex array — by then + // GetRHIHandle() has already been called on nothing. + const auto vertexArray = mesh->GetVertexArray(); + if (!vertexArray) + { + OLO_CORE_ERROR("Renderer3D::DrawAnimatedMeshParallel: Mesh has null VAO (Vertex Array Object)!"); + return nullptr; + } + + const RHI::ResourceHandle vertexArrayID = vertexArray->GetRHIHandle(); + const RHI::ResourceHandle shaderRendererID = shaderToUse->GetRHIHandle(); + if (!ValidateDrawMeshResources("Renderer3D::DrawAnimatedMeshParallel", vertexArrayID, shaderRendererID)) + return nullptr; + // Allocate bone matrices in worker's scratch buffer. FrameDataBuffer& frameBuffer = FrameDataBufferManager::Get(); const u32 boneCount = static_cast(boneMatrices.size()); @@ -1817,11 +1844,6 @@ namespace OloEngine auto* cmd = packet->GetCommandData(); cmd->header.type = CommandType::DrawMesh; - const RHI::ResourceHandle vertexArrayID = mesh->GetVertexArray()->GetRHIHandle(); - const RHI::ResourceHandle shaderRendererID = shaderToUse->GetRHIHandle(); - if (!ValidateDrawMeshResources("Renderer3D::DrawAnimatedMeshParallel", vertexArrayID, shaderRendererID)) - return nullptr; - cmd->meshHandle = mesh->GetHandle(); cmd->vertexArrayID = vertexArrayID; cmd->indexCount = mesh->GetIndexCount(); diff --git a/OloEngine/src/OloEngine/Renderer/Renderer3DUtilityDraws.cpp b/OloEngine/src/OloEngine/Renderer/Renderer3DUtilityDraws.cpp index 71980a73a..7b4784ba9 100644 --- a/OloEngine/src/OloEngine/Renderer/Renderer3DUtilityDraws.cpp +++ b/OloEngine/src/OloEngine/Renderer/Renderer3DUtilityDraws.cpp @@ -592,6 +592,21 @@ namespace OloEngine gridState.blendDstFactor = RHI::BlendFactor::OneMinusSrcAlpha; gridState.depthTestEnabled = true; gridState.depthWriteMask = false; + // Keep the grid out of the view-normals attachment (RT2), which + // is what GTAO samples in the forward path. glEnable(GL_BLEND) + + // glBlendFunc apply to EVERY draw buffer, so the shader's + // `o_ViewNormal = vec2(-2.0)` "no normal" sentinel gets alpha- + // blended against the floor's real normal: a fully covered texel + // keeps -2.0 (GTAO's sentinel guard forces AO=1.0, a bright + // streak), while an antialiased/faded one lands somewhere around + // -1.0 — past the `< -1.5` guard, so GTAO consumes it as a + // genuine normal and computes AO from garbage (a dark streak). + // depthWriteMask=false does NOT prevent this; it only stops depth + // writes. Masking RT2 leaves the floor's true normal in place, so + // the grid — a non-physical editor overlay — no longer perturbs + // AO at all. Deferred is unaffected: its G-Buffer variant draws + // with blending off and never touches GBufferNormal. + gridState.colorAttachmentWriteMask = 0xFFu & ~(1u << 2); } cmd->renderStateIndex = FrameDataBufferManager::Get().AllocateRenderState(gridState); } diff --git a/OloEngine/src/OloEngine/Renderer/Texture.h b/OloEngine/src/OloEngine/Renderer/Texture.h index 32331d909..d98bc031b 100644 --- a/OloEngine/src/OloEngine/Renderer/Texture.h +++ b/OloEngine/src/OloEngine/Renderer/Texture.h @@ -43,11 +43,29 @@ namespace OloEngine // True for the block-compressed ImageFormat values, which take the // glCompressedTextureSubImage2D upload path instead of a client pixel format. - [[nodiscard]] constexpr bool IsCompressedFormat(ImageFormat format) noexcept + [[nodiscard("Store this!")]] constexpr bool IsCompressedFormat(ImageFormat format) noexcept { return format == ImageFormat::BC7 || format == ImageFormat::BC5 || format == ImageFormat::BC6H; } + // True for the integer (non-normalised) ImageFormat values — the ones a + // shader reads through an isampler/usampler rather than a float sampler. + // + // These MUST be sampled with GL_NEAREST. GL requires a texture whose base + // format is integer to use a NEAREST mag filter; with GL_LINEAR it is + // *incomplete*, and sampling an incomplete texture yields zero — including + // through texelFetch. NVIDIA quietly tolerates the linear filter, Mesa + // does not, so a linear-filtered integer texture reads as all-zero on AMD + // and correct on NVIDIA. That asymmetry silently erased every glyph the + // Slug text renderer drew (its RG16UI band texture returned zero bands, so + // each glyph covered no pixels) while leaving the geometry, draw calls and + // logs looking perfectly healthy. + [[nodiscard("Store this!")]] constexpr bool IsIntegerFormat(ImageFormat format) noexcept + { + return format == ImageFormat::R8UI || format == ImageFormat::R16UI || + format == ImageFormat::RG16UI || format == ImageFormat::R32I; + } + struct TextureSpecification { u32 Width = 1; @@ -89,7 +107,7 @@ namespace OloEngine // migration: that one hands out the raw backend name and is deleted once // every caller has moved. Turning a handle back into a native object is // Platform//'s business. - [[nodiscard]] virtual RHI::ResourceHandle GetRHIHandle() const = 0; + [[nodiscard("Store this!")]] virtual RHI::ResourceHandle GetRHIHandle() const = 0; [[nodiscard("Store this!")]] virtual const std::string& GetPath() const = 0; virtual void SetData(void* data, u32 size) = 0; diff --git a/OloEngine/src/OloEngine/Scene/Scene.cpp b/OloEngine/src/OloEngine/Scene/Scene.cpp index 920a9ade8..6cabf2ab6 100644 --- a/OloEngine/src/OloEngine/Scene/Scene.cpp +++ b/OloEngine/src/OloEngine/Scene/Scene.cpp @@ -8209,8 +8209,48 @@ namespace OloEngine // we can safely activate near/above the waterline. Well above // the water (gap < -kWaveReach) stays the water shader's own // refraction/depth tint. Nearest surface (smallest |gap|) wins. - constexpr f32 kWaveReach = 2.0f; // generous max crest height above the flat plane - if (const f32 absGap = std::abs(gap); gap > -kWaveReach && absGap < bestSurfaceDist) + // How far above the FLAT plane a crest can actually reach. + // + // This was a hard-coded 2 m, which is not "generous" for an + // FFT sea: the field's height is scaled by m_FFTAmplitude, + // so a 4 m-amplitude ocean produces crests measured at + // +3.5 m. An eye between 2 m and the real crest height then + // fell into a dead band — waves wash over it, but the fog + // stayed off, so a view angled down through the surface hit + // the seafloor with NO underwater tint at all (issue: the + // FFT ocean's foreground read as the raw magenta seafloor + // while the water above the horizon rendered correctly). + // Derive the reach from the wave configuration instead, and + // keep the old constant as a floor for the Gerstner path, + // whose amplitudes are far smaller. + constexpr f32 kMinWaveReach = 2.0f; + f32 waveReach = kMinWaveReach; + // m_UseFFT alone is not the condition the SHADER runs on. + // The render path only sets fftParams.x = 1 once the field + // has actually produced both textures; until then the + // surface is displaced by Gerstner waves, whose amplitude is + // unrelated to m_FFTAmplitude. Sizing the fog reach from the + // FFT amplitude in that window would size it for waves that + // are not on screen. + const bool fftActive = + water.m_UseFFT && water.m_OceanField && + water.m_OceanField->GetDisplacementTextureHandle().IsValid() && + water.m_OceanField->GetDerivativesTextureHandle().IsValid(); + if (fftActive) + { + // Sanitize exactly as the render path does (clampF with + // the same bounds at the m_Amplitude / fftAmp sites + // above). std::abs would disagree with it on a negative + // amplitude, and skipping the upper bound would disagree + // on an out-of-range one — the fog reach must be derived + // from the wave height actually rendered, not a + // differently-sanitized copy of the same field. + const f32 fftAmplitude = sanitizeParam(water.m_FFTAmplitude, 0.0f, 100.0f, 2.0f); + const f32 heightScale = WaterSurface::ClampFFTHeightScale(water.m_FFTHeightScale); + waveReach = std::max(waveReach, fftAmplitude * heightScale); + } + + if (const f32 absGap = std::abs(gap); gap > -waveReach && absGap < bestSurfaceDist) { bestSurfaceDist = absGap; underwater.Active = true; diff --git a/OloEngine/src/OloEngine/Serialization/MeshBinarySerializer.cpp b/OloEngine/src/OloEngine/Serialization/MeshBinarySerializer.cpp index d9ee0ce55..b0f7fe141 100644 --- a/OloEngine/src/OloEngine/Serialization/MeshBinarySerializer.cpp +++ b/OloEngine/src/OloEngine/Serialization/MeshBinarySerializer.cpp @@ -350,7 +350,8 @@ namespace OloEngine entry.IsRigged = sub.m_IsRigged ? 1 : 0; WriteBytes(payload, &entry, sizeof(entry)); - if (!WriteString(payload, sub.m_NodeName) || !WriteString(payload, sub.m_MeshName)) + if (!WriteString(payload, sub.m_NodeName.ToStdString()) || + !WriteString(payload, sub.m_MeshName.ToStdString())) { return false; } diff --git a/OloEngine/src/OloEngine/Templates/UnrealTypeTraits.h b/OloEngine/src/OloEngine/Templates/UnrealTypeTraits.h index d436ae205..e5e9e712b 100644 --- a/OloEngine/src/OloEngine/Templates/UnrealTypeTraits.h +++ b/OloEngine/src/OloEngine/Templates/UnrealTypeTraits.h @@ -761,6 +761,13 @@ namespace OloEngine }; }; + // Helper for OLO_STATIC_ASSERT_WARN below. Ported from UE's + // TStaticDeprecateExpression (Misc/CoreMiscDefines.h). + template + struct TStaticDeprecateExpression + { + }; + // @struct TUseBitwiseSwap // @brief Determines if bitwise operations (memcpy/memswap) should be used for relocation // @@ -1834,3 +1841,37 @@ namespace OloEngine #undef IS_EMPTY #undef IS_POD #undef HAS_TRIVIAL_CONSTRUCTOR + +// ============================================================================ +// OLO_STATIC_ASSERT_WARN +// ============================================================================ +// A compile-time diagnostic that WARNS rather than errors. Ported from UE's +// UE_STATIC_ASSERT_WARN (Misc/CoreMiscDefines.h); it works by attaching +// [[deprecated]] to the overload selected when the expression is false. +// +// A warning, not a hard static_assert, deliberately — matching UE. Their +// IsTriviallyRelocatable.h explains why the strict default is still commented +// out: "there are a lot of existing violations that will need to be fixed +// first." A hard error makes the guard unadoptable in an existing codebase; +// a warning surfaces every violation without blocking the build. +#define OLO_STATIC_WARN_JOIN_INNER(a, b) a##b +#define OLO_STATIC_WARN_JOIN(a, b) OLO_STATIC_WARN_JOIN_INNER(a, b) + +#define OLO_STATIC_ASSERT_WARN(bExpression, Message) \ + struct OLO_STATIC_WARN_JOIN(FStaticWarningMsg_, __LINE__) \ + { \ + [[deprecated(Message)]] static constexpr int condition( \ + ::OloEngine::TStaticDeprecateExpression) \ + { \ + return 1; \ + } \ + static constexpr int condition(::OloEngine::TStaticDeprecateExpression) \ + { \ + return 1; \ + } \ + }; \ + enum class OLO_STATIC_WARN_JOIN(EStaticWarningMsg_, __LINE__) \ + { \ + Value = OLO_STATIC_WARN_JOIN(FStaticWarningMsg_, __LINE__)::condition( \ + ::OloEngine::TStaticDeprecateExpression()) \ + } diff --git a/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp b/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp index 57a1bf1ba..ceae518b1 100644 --- a/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp +++ b/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp @@ -153,12 +153,19 @@ namespace OloEngine } } + const bool restoreColorMasks = LiftAttachmentColorMasksForClear(); + // A program left bound by the previous pass would be revalidated // against this framebuffer by the driver during the clear (NVIDIA // id 131218 vertex-shader recompile) — unbind it for the clear. Utils::GLClearProgramGuard programGuard; glClear(clearFlags); + if (restoreColorMasks) + { + RestoreAttachmentColorMasks(); + } + if (restoreStencilWriteMask) { glStencilMask(static_cast(previousStencilWriteMask)); @@ -195,10 +202,17 @@ namespace OloEngine glDepthMask(GL_TRUE); } + const bool restoreColorMasks = LiftAttachmentColorMasksForClear(); + // See Clear(): don't let a stale bound program get revalidated here. Utils::GLClearProgramGuard programGuard; glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); + if (restoreColorMasks) + { + RestoreAttachmentColorMasks(); + } + if (!m_DepthMaskEnabled) { glDepthMask(GL_FALSE); @@ -802,6 +816,12 @@ namespace OloEngine OLO_PROFILE_FUNCTION(); glColorMask(red, green, blue, alpha); + + // glColorMask sets EVERY draw buffer, so it also clears any + // per-attachment mask a previous draw installed via glColorMaski. + const AttachmentColorMask mask{ red, green, blue, alpha }; + m_AttachmentColorMasks.fill(mask); + m_AnyAttachmentColorMaskDisabled = !mask.IsFullyEnabled(); } void OpenGLRendererAPI::SetColorMaskForAttachment(u32 attachment, bool red, bool green, bool blue, bool alpha) @@ -809,6 +829,49 @@ namespace OloEngine OLO_PROFILE_FUNCTION(); glColorMaski(attachment, red, green, blue, alpha); + + if (attachment < kMaxTrackedDrawBuffers) + { + m_AttachmentColorMasks[attachment] = { red, green, blue, alpha }; + m_AnyAttachmentColorMaskDisabled = false; + for (const AttachmentColorMask& tracked : m_AttachmentColorMasks) + { + if (!tracked.IsFullyEnabled()) + { + m_AnyAttachmentColorMaskDisabled = true; + break; + } + } + } + } + + bool OpenGLRendererAPI::LiftAttachmentColorMasksForClear() + { + if (!m_AnyAttachmentColorMaskDisabled) + { + return false; + } + + // glClear honours the colour write mask, so an attachment a previous + // draw masked off (the infinite grid keeps itself out of the view- + // normals attachment; skeleton/joint debug draws mask everything but + // RT0) would silently keep last frame's contents. That stale data then + // feeds whatever samples it — GTAO reads the view normals, and a + // never-cleared sky region there reads as occluded geometry. + glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + return true; + } + + void OpenGLRendererAPI::RestoreAttachmentColorMasks() + { + const u32 trackedCount = + m_MaxDrawBuffers > 0 ? std::min(static_cast(m_MaxDrawBuffers), kMaxTrackedDrawBuffers) + : kMaxTrackedDrawBuffers; + for (u32 i = 0; i < trackedCount; ++i) + { + const AttachmentColorMask& mask = m_AttachmentColorMasks[i]; + glColorMaski(i, mask.R, mask.G, mask.B, mask.A); + } } void OpenGLRendererAPI::SetBlendStateForAttachment(u32 attachment, bool enabled) diff --git a/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.h b/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.h index c128cd8ff..a58b024d0 100644 --- a/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.h +++ b/OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.h @@ -3,6 +3,8 @@ #include +#include + namespace OloEngine { @@ -245,6 +247,32 @@ namespace OloEngine } private: + // Per-attachment colour write masks, mirrored from glColorMaski so a + // colour clear can lift them (glClear obeys the colour mask exactly as + // it obeys the depth/stencil write masks the Clear* paths already + // guard) and put them back. Index == draw-buffer index; only the first + // m_MaxDrawBuffers entries are meaningful. + struct AttachmentColorMask + { + bool R = true, G = true, B = true, A = true; + + [[nodiscard]] bool IsFullyEnabled() const + { + return R && G && B && A; + } + }; + static constexpr u32 kMaxTrackedDrawBuffers = 8; + std::array m_AttachmentColorMasks{}; + // True while any tracked attachment has a non-default mask, so the + // clear paths can skip the save/restore entirely in the common case. + bool m_AnyAttachmentColorMaskDisabled = false; + + // Lift every per-attachment colour mask for a clear, returning true if + // anything was changed (in which case RestoreAttachmentColorMasks must + // be called once the clear has been issued). + bool LiftAttachmentColorMasksForClear(); + void RestoreAttachmentColorMasks(); + bool m_DepthTestEnabled = false; bool m_DepthMaskEnabled = true; bool m_StencilTestEnabled = false; diff --git a/OloEngine/src/Platform/OpenGL/OpenGLTexture.cpp b/OloEngine/src/Platform/OpenGL/OpenGLTexture.cpp index ddd3bce6f..91f086fba 100644 --- a/OloEngine/src/Platform/OpenGL/OpenGLTexture.cpp +++ b/OloEngine/src/Platform/OpenGL/OpenGLTexture.cpp @@ -143,6 +143,22 @@ namespace OloEngine } // namespace Utils + namespace + { + // Min filter for a 2D texture, given whether its format is integer and + // whether its mip chain actually holds data. Extracted because the + // inline form was a nested conditional in two places — see + // IsIntegerFormat (integer formats MUST be NEAREST or they sample as + // zero) and OpenGLTexture2D::m_MipsPopulated (allocated levels are not + // written levels). + [[nodiscard("Store this!")]] GLint SelectMinFilter(bool integerFormat, bool mipsUsable) noexcept + { + if (integerFormat) + return mipsUsable ? GL_NEAREST_MIPMAP_NEAREST : GL_NEAREST; + return mipsUsable ? GL_LINEAR_MIPMAP_LINEAR : GL_LINEAR; + } + } // namespace + u32 OpenGLTexture2D::CalculateFullMipCount(u32 width, u32 height) { return static_cast(std::floor(std::log2(static_cast(std::max(width, height))))) + 1; @@ -287,9 +303,19 @@ namespace OloEngine if (m_Specification.Samples == 1u) { + // An integer-format texture is INCOMPLETE under a linear filter and + // then samples as zero (texelFetch included) — see IsIntegerFormat. + const bool integerFormat = IsIntegerFormat(m_Specification.Format); // NOTE: Texture Wrapping - glTextureParameteri(m_RendererID, GL_TEXTURE_MIN_FILTER, GL_LINEAR_MIPMAP_LINEAR); - glTextureParameteri(m_RendererID, GL_TEXTURE_MAG_FILTER, GL_LINEAR); + // Mipmap filters only once the chain HOLDS data — see + // m_MipsPopulated. Allocated-but-unwritten levels sample as + // undefined content, not as an incomplete texture, so this would + // otherwise minify against garbage after a Resize(). + const bool useMips = m_MipLevels > 1u && m_MipsPopulated; + glTextureParameteri(m_RendererID, GL_TEXTURE_MIN_FILTER, + SelectMinFilter(integerFormat, useMips)); + glTextureParameteri(m_RendererID, GL_TEXTURE_MAG_FILTER, + integerFormat ? GL_NEAREST : GL_LINEAR); // NOTE: Texture Filtering glTextureParameteri(m_RendererID, GL_TEXTURE_WRAP_S, GL_REPEAT); @@ -507,7 +533,10 @@ namespace OloEngine glTextureSubImage2D(m_RendererID, 0, 0, 0, static_cast(fw), static_cast(fh), GL_RGBA, GL_FLOAT, rgbaF.data()); if (m_MipLevels > 1u) + { glGenerateTextureMipmap(m_RendererID); + m_MipsPopulated = true; + } OLO_TRACK_GPU_ALLOC(this, rgbaF.size() * sizeof(f32), RendererMemoryTracker::ResourceType::Texture2D, "OpenGL Texture2D (BC6H-fallback)"); GPUResourceInspector::GetInstance().RegisterTexture(m_RendererID, "Texture2D (BC6H-fallback)", "Texture2D"); @@ -550,7 +579,10 @@ namespace OloEngine glTextureSubImage2D(m_RendererID, 0, 0, 0, static_cast(w), static_cast(h), GL_RGBA, GL_UNSIGNED_BYTE, rgba.data()); if (m_MipLevels > 1u) + { glGenerateTextureMipmap(m_RendererID); + m_MipsPopulated = true; + } OLO_TRACK_GPU_ALLOC(this, rgba.size(), RendererMemoryTracker::ResourceType::Texture2D, "OpenGL Texture2D (compressed-fallback)"); GPUResourceInspector::GetInstance().RegisterTexture(m_RendererID, "Texture2D (compressed-fallback)", "Texture2D"); @@ -620,6 +652,10 @@ namespace OloEngine m_Height = height; m_Specification.Width = width; m_Specification.Height = height; + // Fresh storage below: whatever the old object had generated is gone, + // and nothing regenerates here (there is no level-0 data to generate + // FROM). The filter selection reads this to avoid claiming mips. + m_MipsPopulated = false; // Recalculate mip count if auto if (m_Specification.Samples > 1u) @@ -697,9 +733,18 @@ namespace OloEngine if (m_Specification.Samples == 1u) { + // Keep the integer-format NEAREST rule across a resize too — see + // IsIntegerFormat; a linear filter here re-breaks the texture. + const bool integerFormat = IsIntegerFormat(m_Specification.Format); // Reapply sampler state - glTextureParameteri(m_RendererID, GL_TEXTURE_MIN_FILTER, m_MipLevels > 1 ? GL_LINEAR_MIPMAP_LINEAR : GL_LINEAR); - glTextureParameteri(m_RendererID, GL_TEXTURE_MAG_FILTER, GL_LINEAR); + // Resize() recreated storage above and did NOT regenerate the + // chain, so m_MipsPopulated is false here until something uploads + // and generates again — see the member's note. + const bool useMips = m_MipLevels > 1u && m_MipsPopulated; + glTextureParameteri(m_RendererID, GL_TEXTURE_MIN_FILTER, + SelectMinFilter(integerFormat, useMips)); + glTextureParameteri(m_RendererID, GL_TEXTURE_MAG_FILTER, + integerFormat ? GL_NEAREST : GL_LINEAR); glTextureParameteri(m_RendererID, GL_TEXTURE_WRAP_S, GL_REPEAT); glTextureParameteri(m_RendererID, GL_TEXTURE_WRAP_T, GL_REPEAT); } @@ -769,6 +814,13 @@ namespace OloEngine bpp = 4; dataType = GL_FLOAT; break; + case ImageFormat::R32I: + // Without this the default arm leaves GL_UNSIGNED_BYTE. bpp is + // 4 either way, so the size assert below still passes and the + // upload silently reinterprets the data. + bpp = 4; + dataType = GL_INT; + break; case ImageFormat::RG32F: bpp = 8; dataType = GL_FLOAT; @@ -872,6 +924,11 @@ namespace OloEngine case ImageFormat::RGBA16F: dataType = GL_FLOAT; break; + case ImageFormat::R32I: + // Integer format: the default arm would leave GL_UNSIGNED_BYTE + // and silently reinterpret the upload. + dataType = GL_INT; + break; default: break; } @@ -989,6 +1046,7 @@ namespace OloEngine glTextureSubImage2D(m_RendererID, 0, 0, 0, static_cast(m_Width), static_cast(m_Height), dataFormat, GL_UNSIGNED_BYTE, data); glGenerateTextureMipmap(m_RendererID); + m_MipsPopulated = true; } void OpenGLTexture2D::Bind(const u32 slot) const { @@ -1059,6 +1117,12 @@ namespace OloEngine bytesPerPixel = 4; dataType = GL_FLOAT; break; + case ImageFormat::R32I: + // Integer format — readback must use GL_INT, not the default + // GL_UNSIGNED_BYTE, or the entity-ID values come back mangled. + bytesPerPixel = 4; + dataType = GL_INT; + break; case ImageFormat::RG32F: bytesPerPixel = 8; dataType = GL_FLOAT; diff --git a/OloEngine/src/Platform/OpenGL/OpenGLTexture.h b/OloEngine/src/Platform/OpenGL/OpenGLTexture.h index bf64a392b..9249b17f0 100644 --- a/OloEngine/src/Platform/OpenGL/OpenGLTexture.h +++ b/OloEngine/src/Platform/OpenGL/OpenGLTexture.h @@ -92,6 +92,13 @@ namespace OloEngine u32 m_Width{}; u32 m_Height{}; u32 m_MipLevels = 1; + // Whether levels 1..m_MipLevels-1 actually hold data. glTextureStorage2D + // ALLOCATES the whole chain, so m_MipLevels > 1 only means the levels + // exist — sampling one that was never written is defined but returns + // undefined content. Resize() recreates storage without regenerating, + // so selecting a mipmap min-filter on m_MipLevels alone would minify + // against garbage. Only a real upload/generate flips this true. + bool m_MipsPopulated = false; u32 m_RendererID{}; // Generation-checked identity for m_RendererID above, kept in // lockstep by m_RHIHandle.Sync() at every site that assigns the diff --git a/OloEngine/tests/AssetSceneLoadTest.cpp b/OloEngine/tests/AssetSceneLoadTest.cpp index 8e2598f54..c55eeb070 100644 --- a/OloEngine/tests/AssetSceneLoadTest.cpp +++ b/OloEngine/tests/AssetSceneLoadTest.cpp @@ -75,6 +75,12 @@ #include #include +#if defined(_WIN32) +#include +#define getpid _getpid +#else +#include +#endif #include #include #include @@ -91,18 +97,76 @@ namespace OloEngine::Tests namespace fs = std::filesystem; // Stage the entire SandboxProject into a fresh temp dir. - // Returns the temp project's root path (or empty on failure). - fs::path StageSandboxProjectIntoTemp() + // Returns the temp project's root path (or empty on failure, with the + // reason written to `error` — a bare empty path made a permission + // failure here indistinguishable from a copy failure). + fs::path StageSandboxProjectIntoTemp(std::string& error) { const fs::path sandboxRoot = fs::path{ OLO_TEST_EDITOR_ROOT } / "SandboxProject"; - const fs::path tempRoot = - fs::temp_directory_path() / "OloEngineSceneLoad"; std::error_code ec; - fs::remove_all(tempRoot, ec); - fs::create_directories(tempRoot, ec); + const fs::path tempDir = fs::temp_directory_path(ec); if (ec) + { + error = "temp_directory_path failed: " + ec.message(); return {}; + } + + // A FIXED name under a shared, sticky /tmp is not usable. This + // repo's self-hosted CI runs the suite as its own account, so + // /tmp/OloEngineSceneLoad can already exist owned by a different + // user — and the sticky bit then makes it un-removable AND + // un-recreatable for everyone else, failing this test forever. + // Two concurrent runs on one box (several runners share this + // host) would likewise fight over the same tree mid-copy. + // + // Name the root after this process, then claim it EXCLUSIVELY. + // + // An earlier version removed a pre-existing candidate before + // claiming it, on the theory that it was our own leftover. That is + // not a safe assumption: remove_all succeeds for any directory + // owned by the same user, including one a CONCURRENT run by that + // same user is copying into right now — so the reclaim step could + // delete a live staging tree mid-copy, which is precisely the + // collision this loop exists to avoid. + // + // create_directory returns false when the path already exists, so + // it doubles as the claim: we only ever proceed with a directory + // we just created, and never touch one we did not. A crashed run + // therefore leaks its directory rather than risking a live one; + // the pid keeps those out of the way of new runs, and the attempt + // suffix covers a recycled pid whose leftovers are still present. + const long long pid = static_cast(::getpid()); + std::error_code lastError; + fs::path tempRoot; + for (u32 attempt = 0; attempt < 64; ++attempt) + { + const fs::path candidate = + tempDir / ("OloEngineSceneLoad-" + std::to_string(pid) + "-" + std::to_string(attempt)); + + ec.clear(); + if (fs::create_directory(candidate, ec) && !ec) + { + tempRoot = candidate; + break; + } + // Keep the last REAL error. A candidate that merely already + // exists returns false with ec == success, which would + // otherwise overwrite a genuine earlier failure and report + // "last error: Success". + if (ec) + lastError = ec; + } + + if (tempRoot.empty()) + { + error = "could not create a staging dir under " + tempDir.string(); + if (lastError) + error += " (last error: " + lastError.message() + ")"; + else + error += " (all candidate names were already taken)"; + return {}; + } // Recursive copy: every file, every subdirectory. We need // Assets/, AssetRegistry.oar, and the .oloproj at minimum; @@ -113,7 +177,18 @@ namespace OloEngine::Tests fs::copy_options::copy_symlinks, ec); if (ec) + { + error = "copy " + sandboxRoot.string() + " -> " + tempRoot.string() + " failed: " + ec.message(); + std::error_code cleanupEc; + fs::remove_all(tempRoot, cleanupEc); + if (cleanupEc) + { + // Say so rather than swallow it: a partial tree left behind + // is the next run's confusing failure. + error += " (cleanup of " + tempRoot.string() + " also failed: " + cleanupEc.message() + ")"; + } return {}; + } return tempRoot; } @@ -158,9 +233,10 @@ namespace OloEngine::Tests // shared process+context. GLStateGuard glGuard("AssetSceneLoad.Deserialize", GLStateGuard::Policy::Restore); - const fs::path tempRoot = StageSandboxProjectIntoTemp(); + std::string stageError; + const fs::path tempRoot = StageSandboxProjectIntoTemp(stageError); ASSERT_FALSE(tempRoot.empty()) - << "Failed to stage SandboxProject into temp dir."; + << "Failed to stage SandboxProject into temp dir: " << stageError; // RAII cleanup: delete the temp dir on test exit regardless of // assertion outcome. @@ -169,8 +245,36 @@ namespace OloEngine::Tests fs::path Dir; ~Cleanup() { + // Retry, and report if it still fails. + // + // remove_all walks bottom-up, and the asset manager recreates + // its Assets/cache/{mesh,animation} directories during teardown + // — if that lands between the child removal and the parent's, + // the parent fails ENOTEMPTY and the tree is left behind. A + // single silent attempt leaked a staging root per run, which + // used to be masked because a fixed directory name meant the + // next run reclaimed it; now that each run stages under its own + // pid, a leak accumulates instead. std::error_code ec; - fs::remove_all(Dir, ec); + for (int attempt = 0; attempt < 3; ++attempt) + { + ec.clear(); + fs::remove_all(Dir, ec); + // Judge success by whether the directory is GONE, not by + // what remove_all reported. The race here is a recreate + // landing after the removal, which leaves remove_all + // reporting success while the tree survives — so an + // `!ec ||` short-circuit would return without retrying in + // precisely the case the retry exists for. + // + // Non-throwing overload: a destructor is implicitly + // noexcept, so the throwing fs::exists(Dir) would call + // std::terminate rather than report a failure to clean up. + std::error_code existsEc; + if (!fs::exists(Dir, existsEc)) + return; + } + OLO_CORE_WARN("AssetSceneLoad: could not remove staging dir '{}': {}", Dir.string(), ec.message()); } } cleanup{ tempRoot }; diff --git a/OloEngine/tests/CMakeLists.txt b/OloEngine/tests/CMakeLists.txt index b3eda0df2..945b0631b 100644 --- a/OloEngine/tests/CMakeLists.txt +++ b/OloEngine/tests/CMakeLists.txt @@ -74,6 +74,7 @@ add_executable(OloEngine-Tests Tasks/TaskSystemTest.cpp HAL/ThreadManagerTest.cpp Containers/ConcurrentQueuesTest.cpp + Containers/StringTest.cpp Memory/MemoryViewTest.cpp Memory/LockFreeAllocatorConcurrencyTest.cpp Templates/FunctionWithContextTest.cpp diff --git a/OloEngine/tests/ContainerTest.cpp b/OloEngine/tests/ContainerTest.cpp index 40a9d49f0..e4576d735 100644 --- a/OloEngine/tests/ContainerTest.cpp +++ b/OloEngine/tests/ContainerTest.cpp @@ -6,6 +6,7 @@ #include "OloEngine/Containers/SparseArray.h" #include "OloEngine/Containers/Set.h" #include "OloEngine/Containers/Map.h" +#include "OloEngine/Containers/String.h" #include "OloEngine/Templates/UnrealTypeTraits.h" #include @@ -66,12 +67,17 @@ TEST(ContainerSmoke, TSetAddDuplicateAndContains) TEST(ContainerSmoke, TMapAddFindRemove) { - TMap map; - map.Add(1, "one"); - map.Add(2, "two"); + // FString, not std::string. TMap stores its pairs in a TSparseArray, which + // relocates bitwise, and libstdc++'s std::string does not survive that (see + // the TMapRelocation cases below). The relocatability guard flags it, and + // the guard is right -- a std::string value here was always unsound, it + // just happened not to grow enough in this smoke test to corrupt. + TMap map; + map.Add(1, FString("one")); + map.Add(2, FString("two")); auto* found = map.Find(1); ASSERT_NE(found, nullptr); - EXPECT_EQ(*found, "one"); + EXPECT_TRUE(found->Equals(FString("one"))); map.Remove(1); EXPECT_EQ(map.Find(1), nullptr); EXPECT_EQ(map.Num(), 1); @@ -211,3 +217,223 @@ TEST(TArrayTest, TElementTypeWorks) EXPECT_TRUE((std::is_same_v>, i32>)); EXPECT_TRUE((std::is_same_v>, f32>)); } + +// ============================================================================ +// TArray regression tests +// +// Both of these were live defects found while giving Submesh a relocatable +// string type; neither is string-specific. +// ============================================================================ + +// The copy constructor called CopyToEmpty() on an array whose m_ArrayNum and +// m_ArrayMax were still indeterminate, and CopyToEmpty routed through +// ResizeAllocation(), which short-circuits on `if (NewMax != m_ArrayMax)`. +// Whenever the uninitialised garbage happened to equal the computed NewMax the +// allocation was SKIPPED, leaving GetData() null for the ConstructItems memcpy +// that immediately followed -- a segfault writing to address 0. +// +// Small element types are the ones that expose it: their quantised capacities +// are small numbers that plausibly appear in recycled stack memory, whereas +// this engine's larger element types produce values that rarely collide. Hence +// char here. +TEST(TArrayRegression, CopyConstructAllocatesForSmallElementTypes) +{ + using namespace OloEngine; + + // Repeat: the bug depends on the garbage in the destination's storage, so + // a single copy can pass by luck. Reusing the same stack region across + // iterations is what makes this reliable. + for (i32 iteration = 0; iteration < 256; ++iteration) + { + TArray> source; + for (char c : std::string_view("Cube")) + source.Add(c); + source.Add('\0'); + + TArray> copy(source); + + ASSERT_NE(copy.GetData(), nullptr) << "copy ctor skipped its allocation on iteration " << iteration; + ASSERT_EQ(copy.Num(), source.Num()); + EXPECT_STREQ(copy.GetData(), "Cube"); + } +} + +// Copy-assignment passed m_ArrayMax as CopyToEmpty's third argument, which this +// port treats as ExtraSlack (`NewMax = Count + ExtraSlack`) rather than UE's +// PrevMax. Every assignment therefore requested Count + current capacity, so +// capacity grew without bound across repeated assignments to the same array. +TEST(TArrayRegression, CopyAssignmentDoesNotGrowCapacityWithoutBound) +{ + using namespace OloEngine; + + TArray> source; + for (i32 i = 0; i < 8; ++i) + source.Add(i); + + TArray> target; + target = source; + const auto capacityAfterFirst = target.Max(); + + // Assign the same contents many times. Capacity must settle, not compound. + for (i32 i = 0; i < 64; ++i) + target = source; + + EXPECT_EQ(target.Num(), source.Num()); + EXPECT_EQ(target.Max(), capacityAfterFirst) + << "capacity grew across repeated assignment (" << capacityAfterFirst << " -> " << target.Max() << ")"; + + for (i32 i = 0; i < 8; ++i) + EXPECT_EQ(target[i], i); +} + +// ============================================================================ +// TMap element relocation — a controlled three-case matrix. +// +// Material.h keeps a dozen uniform tables as TMap. TMap is +// built on TSet -> TSparseArray -> TArray, and TArray relocates bitwise. The +// question is whether that relocation reaches the element payload and corrupts +// std::string keys the way it corrupted TArray. +// +// Three cases isolate the mechanism: +// 1. std::string keys, too few to grow -> must pass (no relocation) +// 2. std::string keys, many (grows) -> corrupts (DISABLED, see below) +// 3. FString keys, many (grows) -> must pass (relocatable) +// +// Short keys throughout: only those live in std::string's SSO inline buffer and +// carry the self-referential pointer. Long keys point at separate heap blocks +// and survive relocation regardless, so a test using them proves nothing. +// ============================================================================ + +// Case 1 — control. Few enough entries that the backing storage never grows, +// so no element is ever relocated. If this failed, the problem would be +// something other than relocation. +TEST(TMapRelocation, StdStringKeysIntactWithoutGrowth) +{ + using namespace OloEngine; + + TMap uniforms; + constexpr i32 kFew = 4; + + for (i32 i = 0; i < kFew; ++i) + uniforms.Add("u" + std::to_string(i), static_cast(i)); + + ASSERT_EQ(uniforms.Num(), kFew); + for (i32 i = 0; i < kFew; ++i) + { + const f32* found = uniforms.Find("u" + std::to_string(i)); + ASSERT_NE(found, nullptr) << "key lost WITHOUT any growth — not a relocation problem"; + EXPECT_FLOAT_EQ(*found, static_cast(i)); + } +} + +// Case 2 — the defect. DISABLED because it fails by design: std::string is not +// trivially relocatable, so it must not be used as a TMap key at all. Kept as +// executable documentation of why, and as the reproduction if anyone doubts it. +// +// Observed: keys u0..u3 found intact, u4 lost, then the run hung walking the +// corrupted map. Partial, scattered loss is the signature of relocation damage +// — a logic error in Add/Find would fail uniformly, not from the fifth key on. +// +// Run with --gtest_also_run_disabled_tests to see it fail. +TEST(TMapRelocation, DISABLED_StdStringKeysCorruptAcrossGrowth) +{ + using namespace OloEngine; + + TMap uniforms; + constexpr i32 kCount = 512; + + for (i32 i = 0; i < kCount; ++i) + uniforms.Add("u" + std::to_string(i), static_cast(i)); + + for (i32 i = 0; i < kCount; ++i) + { + const std::string key = "u" + std::to_string(i); + const f32* found = uniforms.Find(key); + ASSERT_NE(found, nullptr) << "key '" << key << "' lost across TMap growth"; + EXPECT_FLOAT_EQ(*found, static_cast(i)); + } +} + +// Case 3 — the fix. Same shape as case 2 but with a relocatable key type. +// This is what Material's uniform tables must use. +TEST(TMapRelocation, FStringKeysSurviveGrowth) +{ + using namespace OloEngine; + + TMap uniforms; + constexpr i32 kCount = 512; + + for (i32 i = 0; i < kCount; ++i) + uniforms.Add(FString(("u" + std::to_string(i)).c_str()), static_cast(i)); + + ASSERT_EQ(uniforms.Num(), kCount); + for (i32 i = 0; i < kCount; ++i) + { + const FString key(("u" + std::to_string(i)).c_str()); + const f32* found = uniforms.Find(key); + ASSERT_NE(found, nullptr) << "key '" << *key << "' lost across TMap growth"; + EXPECT_FLOAT_EQ(*found, static_cast(i)); + } +} + +// Diagnostic: does the relocatability trait actually propagate through the +// types TMap is built from? The guard in ~TCompactSet checks the set's element +// type, which for TMap is TPair. +TEST(TMapRelocation, TraitPropagatesThroughPair) +{ + using namespace OloEngine; + EXPECT_FALSE(TIsTriviallyRelocatable_V) << "std::string must be non-relocatable"; + EXPECT_FALSE((TIsTriviallyRelocatable_V>)) + << "TPair must inherit non-relocatability from its key -- if this is TRUE, " + "the ~TCompactSet guard can never catch TMap"; + EXPECT_TRUE((TIsTriviallyRelocatable_V>)) << "FString pair should be relocatable"; +} + +// ============================================================================= +// FString self-aliasing appends (PR #701 review). +// +// AppendChars used to hold a raw `const char*` across +// Data.SetNumUninitialized(), which grows through FMemory::Realloc and MOVES +// the buffer. When the source pointed into that same buffer the copy then read +// freed heap: `s += s` spliced garbage into the middle of the result. Both +// cases below reproduce it if the aliasing guard is removed. +// ============================================================================= +TEST(FStringSelfAppend, SelfAppendAcrossGrowthKeepsContent) +{ + // Long enough that the append forces a reallocation rather than fitting in + // the existing slack. + std::string seed(64, 'A'); + seed += "-TAIL"; + + FString s(seed.c_str()); + const std::string expected = seed + seed; + + s += s; + + ASSERT_EQ(static_cast(s.Len()), expected.size()); + EXPECT_EQ(std::string(*s, static_cast(s.Len())), expected) + << "self-append read through a dangling pointer after the buffer moved"; +} + +TEST(FStringSelfAppend, AppendingOwnSuffixCopiesFromTheReDerivedSource) +{ + // A view over this string's own tail. This is the case the offset + // re-derivation exists for: the source pointer is INSIDE the buffer that + // SetNumUninitialized may move, so a pointer captured beforehand dangles. + // + // The ranges are adjacent, not overlapping — the source is a sub-range of + // [0, Len()] and the destination begins at Len() — so this does not + // require memmove over memcpy. It exercises that the source is re-derived + // correctly after the growth, which is the part that can actually break. + std::string seed(48, 'B'); + seed += "-SUFFIX"; + + FString s(seed.c_str()); + const std::string_view suffix(*s + (s.Len() - 7), 7); // "-SUFFIX" + const std::string expected = seed + std::string("-SUFFIX"); + + s.Append(suffix); + + ASSERT_EQ(static_cast(s.Len()), expected.size()); + EXPECT_EQ(std::string(*s, static_cast(s.Len())), expected); +} diff --git a/OloEngine/tests/Containers/StringTest.cpp b/OloEngine/tests/Containers/StringTest.cpp new file mode 100644 index 000000000..e0fb12a9c --- /dev/null +++ b/OloEngine/tests/Containers/StringTest.cpp @@ -0,0 +1,265 @@ +// OLO_TEST_LAYER: unit +// +// FString — the trivially-relocatable string ported from Unreal Engine. +// +// The headline test here is StoredInTArraySurvivesReallocation. FString exists +// because TArray relocates elements BITWISE (ResizeGrow -> ResizeAllocation -> +// FMemory::Realloc on the raw byte buffer), and libstdc++'s std::string cannot +// survive that: under SSO its internal pointer points into its own inline +// buffer, so relocating leaves that pointer aimed at the element's old address +// and the destructor frees a non-heap pointer — +// +// free(): invalid pointer +// +// — which is exactly how AssetSceneLoad aborted on the Linux GPU runner via +// ~TArray. FString is a single TArray member with no SSO, so its +// pointer always targets a separate heap block and byte-copying it is safe. + +#include "OloEnginePCH.h" +#include + +#include "OloEngine/Containers/Array.h" +#include "OloEngine/Containers/String.h" + +#include +#include + +namespace OloEngine::Tests +{ + namespace + { + TEST(FStringTest, DefaultConstructedIsEmpty) + { + const FString s; + EXPECT_TRUE(s.IsEmpty()); + EXPECT_EQ(s.Len(), 0); + // Must still yield a valid null-terminated buffer. + EXPECT_STREQ(*s, ""); + } + + TEST(FStringTest, ConstructsFromCStringAndStdString) + { + const FString a("hello"); + EXPECT_EQ(a.Len(), 5); + EXPECT_STREQ(*a, "hello"); + EXPECT_FALSE(a.IsEmpty()); + + const FString b(std::string("world")); + EXPECT_STREQ(*b, "world"); + + // Pointer + explicit length (does not require null termination). + const FString c("abcdef", 3); + EXPECT_EQ(c.Len(), 3); + EXPECT_STREQ(*c, "abc"); + } + + TEST(FStringTest, EmbeddedLengthNotBufferLength) + { + // The storage invariant: Data holds the characters PLUS a null + // terminator, so Len() must never be the raw array count. + const FString s("abc"); + EXPECT_EQ(s.Len(), 3); + EXPECT_EQ(s.GetCharArray().Num(), 4); + } + + TEST(FStringTest, AppendAndConcatenate) + { + FString s("foo"); + s += "bar"; + EXPECT_STREQ(*s, "foobar"); + + s.AppendChar('!'); + EXPECT_STREQ(*s, "foobar!"); + EXPECT_EQ(s.Len(), 7); + + const FString joined = FString("a") + FString("b") + "c"; + EXPECT_STREQ(*joined, "abc"); + } + + TEST(FStringTest, AppendToEmptyStartsClean) + { + // Appending to a default-constructed string must not read the + // (absent) terminator slot. + FString s; + s += "x"; + EXPECT_STREQ(*s, "x"); + EXPECT_EQ(s.Len(), 1); + } + + TEST(FStringTest, ComparisonRespectsCase) + { + const FString a("Hello"); + EXPECT_TRUE(a.Equals(FString("Hello"))); + EXPECT_FALSE(a.Equals(FString("hello"))); + EXPECT_TRUE(a.Equals(FString("hello"), FString::ESearchCase::IgnoreCase)); + EXPECT_TRUE(a == "Hello"); + } + + TEST(FStringTest, SearchOperations) + { + const FString s("the quick brown fox"); + EXPECT_EQ(s.Find("quick"), 4); + EXPECT_EQ(s.Find("QUICK"), FString::InvalidIndex); + EXPECT_EQ(s.Find("QUICK", FString::ESearchCase::IgnoreCase), 4); + EXPECT_EQ(s.Find("absent"), FString::InvalidIndex); + EXPECT_TRUE(s.Contains("brown")); + EXPECT_TRUE(s.StartsWith("the")); + EXPECT_TRUE(s.EndsWith("fox")); + EXPECT_FALSE(s.StartsWith("fox")); + + i32 idx = 0; + EXPECT_TRUE(s.FindChar('q', idx)); + EXPECT_EQ(idx, 4); + } + + TEST(FStringTest, Substrings) + { + const FString s("abcdef"); + EXPECT_STREQ(*s.Left(3), "abc"); + EXPECT_STREQ(*s.Right(2), "ef"); + EXPECT_STREQ(*s.Mid(2, 2), "cd"); + EXPECT_STREQ(*s.LeftChop(2), "abcd"); + EXPECT_STREQ(*s.RightChop(4), "ef"); + // Out-of-range must clamp, not read past the buffer. + EXPECT_STREQ(*s.Left(100), "abcdef"); + EXPECT_STREQ(*s.Mid(100), ""); + } + + TEST(FStringTest, CaseAndTrimming) + { + EXPECT_STREQ(*FString("MiXeD").ToUpper(), "MIXED"); + EXPECT_STREQ(*FString("MiXeD").ToLower(), "mixed"); + EXPECT_STREQ(*FString(" pad ").TrimStartAndEnd(), "pad"); + EXPECT_STREQ(*FString("\t x \n").TrimStartAndEnd(), "x"); + } + + // FString stores an explicit length, so the counted and string_view + // constructors can hold a NUL in the middle. Comparison must respect + // that: a terminator-driven strcmp stops at the first one and reported + // strings that differ AFTER it as equal, which Equals' length guard + // could not catch because the lengths matched. + TEST(FStringTest, ComparisonIsLengthAwareAcrossEmbeddedNuls) + { + const FString a(std::string_view("a\0b", 3)); + const FString b(std::string_view("a\0c", 3)); + + ASSERT_EQ(a.Len(), 3); + ASSERT_EQ(b.Len(), 3); + + EXPECT_FALSE(a.Equals(b)); + EXPECT_FALSE(a == b); + EXPECT_NE(a.Compare(b), 0); + EXPECT_TRUE(a.Equals(FString(std::string_view("a\0b", 3)))); + + // Case-insensitive must be length-aware too. + const FString upper(std::string_view("A\0B", 3)); + EXPECT_TRUE(a.Equals(upper, FString::ESearchCase::IgnoreCase)); + EXPECT_FALSE(b.Equals(upper, FString::ESearchCase::IgnoreCase)); + + // A prefix must not compare equal to the longer string, and must + // sort before it. + const FString prefix(std::string_view("a\0", 2)); + EXPECT_FALSE(prefix.Equals(a)); + EXPECT_LT(prefix.Compare(a), 0); + EXPECT_GT(a.Compare(prefix), 0); + } + + TEST(FStringTest, SplitFollowsUnrealSemantics) + { + FString left, right; + const FString path("folder/file.txt"); + EXPECT_TRUE(path.Split("/", &left, &right)); + EXPECT_STREQ(*left, "folder"); + EXPECT_STREQ(*right, "file.txt"); + + // No separator: returns false and leaves the outputs untouched. + FString l2("untouched"), r2("untouched"); + EXPECT_FALSE(path.Split("|", &l2, &r2)); + EXPECT_STREQ(*l2, "untouched"); + EXPECT_STREQ(*r2, "untouched"); + } + + TEST(FStringTest, Printf) + { + EXPECT_STREQ(*FString::Printf("%d-%s", 42, "x"), "42-x"); + EXPECT_STREQ(*FString::FromInt(-7), "-7"); + // Longer than any small-buffer guess, to exercise the sizing pass. + const FString big = FString::Printf("%0*d", 500, 1); + EXPECT_EQ(big.Len(), 500); + } + + TEST(FStringTest, StdInterop) + { + const FString s("round-trip"); + EXPECT_EQ(s.ToStdString(), std::string("round-trip")); + EXPECT_EQ(s.ToView(), std::string_view("round-trip")); + EXPECT_EQ(FString(s.ToStdString()).Len(), s.Len()); + } + + // ------------------------------------------------------------------ + // The reason this type exists. + // ------------------------------------------------------------------ + + TEST(FStringTest, IsMarkedTriviallyRelocatable) + { + static_assert(TIsTriviallyRelocatable_V, + "FString must be trivially relocatable — that is the whole point of it"); + static_assert(!TIsTriviallyRelocatable_V, + "std::string must remain marked NON-relocatable (libstdc++ SSO self-pointer)"); + SUCCEED(); + } + + TEST(FStringTest, StoredInTArraySurvivesReallocation) + { + // Force many reallocations, so the elements are relocated bitwise + // by FMemory::Realloc repeatedly. With std::string elements this is + // precisely the sequence that corrupts the heap on libstdc++ and + // aborts with "free(): invalid pointer" at array destruction. + // + // Deliberately SHORT strings: only those live in the SSO inline + // buffer and carry the self-referential pointer. Long strings point + // at a separate heap block and would survive relocation anyway, so + // a test using long strings would pass even against a broken type. + TArray> arr; + constexpr i32 kCount = 512; + + for (i32 i = 0; i < kCount; ++i) + arr.Add(FString::Printf("s%d", i)); // 2-4 chars => SSO range + + ASSERT_EQ(arr.Num(), kCount); + + // Every element must still read back correctly after all that + // relocation — a corrupted element typically shows up as garbage + // content well before it shows up as a crash. + for (i32 i = 0; i < kCount; ++i) + { + const FString expected = FString::Printf("s%d", i); + EXPECT_TRUE(arr[i].Equals(expected)) << "element " << i << " corrupted by relocation"; + } + + // Destruction of `arr` at scope exit is where the invalid free + // would fire; reaching the end of the test without aborting is + // itself part of the assertion. + } + + TEST(FStringTest, TArrayOfStringsSurvivesInsertAndRemove) + { + // The other relocation path: RelocateConstructItems memmoves during + // insert/remove shifting, distinct from realloc-based growth. + TArray> arr; + for (i32 i = 0; i < 32; ++i) + arr.Add(FString::Printf("e%d", i)); + + arr.Insert(FString("inserted"), 0); + EXPECT_STREQ(*arr[0], "inserted"); + EXPECT_STREQ(*arr[1], "e0"); + + arr.RemoveAt(0); + EXPECT_STREQ(*arr[0], "e0"); + EXPECT_EQ(arr.Num(), 32); + + for (i32 i = 0; i < 32; ++i) + EXPECT_TRUE(arr[i].Equals(FString::Printf("e%d", i))) << "element " << i << " corrupted by shift"; + } + } // namespace +} // namespace OloEngine::Tests diff --git a/OloEngine/tests/MathTest.cpp b/OloEngine/tests/MathTest.cpp index b25297dd9..2a284c2e1 100644 --- a/OloEngine/tests/MathTest.cpp +++ b/OloEngine/tests/MathTest.cpp @@ -101,20 +101,51 @@ namespace EXPECT_TRUE(BitwiseEqual(42, 42)); EXPECT_FALSE(BitwiseEqual(42, 43)); + // A struct with implicit padding after the trailing bool. struct Trivial { - f32 X; - i32 Y; - bool Z; - // Pad to force a struct with implicit padding bytes. Bit-exact - // comparison includes padding, so callers must zero-init for - // predictable equality — same rule as std::memcmp. + f32 m_X; + i32 m_Y; + bool m_Z; }; + static_assert(sizeof(Trivial) > sizeof(f32) + sizeof(i32) + sizeof(bool), + "this case only exercises anything while Trivial actually has padding"); + + // `BitwiseEqual` is `memcmp` over `sizeof(T)`, so it compares PADDING + // bytes as well as members — and the language does not guarantee that + // copying an object reproduces them. This case previously wrote + // `const Trivial a{ 1.0f, 7, true }; Trivial b = a;` and asserted the + // two compared equal. That holds on MSVC and Clang, which copy the + // whole object representation, but GCC's implicit copy constructor + // copies member-wise and leaves the destination's padding as whatever + // was on the stack — so every member matched and only bytes 9-11 + // differed. It went unnoticed until the suite first ran under GCC. + // + // Value-initialisation zero-initializes the whole object + // representation, padding included, so initialising both objects that + // way and assigning members gives a deterministic comparison. This is + // exactly the "zero-init for predictable equality" rule the helper's + // callers must follow. + // + // The spelling matters, and `Trivial a{}` is NOT it. `Trivial` is an + // aggregate, and for an aggregate the empty-brace form performs + // AGGREGATE initialization — each member is initialized from `{}`, + // which says nothing about the bytes between them. `Trivial()` is a + // value-initialized prvalue, and since C++17's guaranteed elision it + // initializes `a` directly with no intervening copy, so the padding + // guarantee actually reaches the object being compared. + Trivial a = Trivial(); + a.m_X = 1.0f; + a.m_Y = 7; + a.m_Z = true; + + Trivial b = Trivial(); + b.m_X = 1.0f; + b.m_Y = 7; + b.m_Z = true; - const Trivial a{ 1.0f, 7, true }; - Trivial b = a; EXPECT_TRUE(BitwiseEqual(a, b)); - b.Y = 8; + b.m_Y = 8; EXPECT_FALSE(BitwiseEqual(a, b)); } diff --git a/OloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cpp b/OloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cpp index 9797dc133..67d5b7186 100644 --- a/OloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cpp +++ b/OloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cpp @@ -134,6 +134,54 @@ namespace OloEngine::Tests return count ? std::sqrt(sumSq / static_cast(count)) : 0.0; } + // Goldens are baselined per GPU vendor. + // + // These captures are compared with an RMSE threshold of 8, and a real + // AMD-vs-NVIDIA difference blows straight through that: on radeonsi the + // three NIGHT captures drift 19-24 RMSE against NVIDIA-baselined images + // while every day capture passes, because the night sky is where the + // star-field/procedural-sky float precision diverges most. That is a + // genuine vendor difference, not a regression, so each vendor needs its + // own baseline set. + // + // Mirrors GoldenImageTests::GoldenBaselineDir so both golden mechanisms + // scope identically; previously only that one honoured the variable, so + // an AMD run silently compared against — and on a rebase would have + // OVERWRITTEN — the NVIDIA baselines. + [[nodiscard]] fs::path GoldenBaselineDir() + { + fs::path base = fs::path("assets") / "tests" / "visual"; + if (const char* vendor = std::getenv("OLOENGINE_GOLDEN_VENDOR"); vendor != nullptr && vendor[0] != '\0') + { + // The vendor must name ONE directory below the baseline root, + // nothing else. `base /= vendor` is not safe on its own: an + // absolute value REPLACES base outright (fs::path semantics), + // and "../.." walks out of the tree — so with + // OLOENGINE_GOLDEN_REBASE=1 a stray value would write goldens + // anywhere the process can reach. Accept only a plain name. + const std::string_view name(vendor); + // A separator/dot check alone is not enough on Windows: a + // DRIVE-RELATIVE value like "C:vendor" contains neither, yet + // fs::path treats it as rooted, so `base /= name` would discard + // base and resolve against C:'s current directory. Reject + // anything fs::path considers rooted at all. + const fs::path vendorPath(name); + const bool safe = name.find('/') == std::string_view::npos && + name.find('\\') == std::string_view::npos && + name != "." && name != ".." && + !vendorPath.has_root_name() && !vendorPath.has_root_directory(); + if (!safe) + { + ADD_FAILURE() << "OLOENGINE_GOLDEN_VENDOR must be a single directory name " + "(no separators, '.', '..', drive letter or root) — got '" + << name << "'"; + return base; + } + base /= name; + } + return base; + } + [[nodiscard]] bool GoldenRebaseRequested() { const char* v = std::getenv("OLOENGINE_GOLDEN_REBASE"); @@ -329,7 +377,7 @@ namespace OloEngine::Tests m_Horizon[name] = MeanBand(pixels, kHeight * 38u / 100u, kHeight * 46u / 100u); m_Ground[name] = MeanBand(pixels, kHeight * 75u / 100u, kHeight); - const fs::path dir = fs::path("assets") / "tests" / "visual"; + const fs::path dir = GoldenBaselineDir(); const std::string path = (dir / ("Atmosphere_" + name + ".png")).string(); if (GoldenRebaseRequested()) diff --git a/OloEngine/tests/Rendering/PropertyTests/GLStateGuardTest.cpp b/OloEngine/tests/Rendering/PropertyTests/GLStateGuardTest.cpp index d4a6b2f35..51eddfaaa 100644 --- a/OloEngine/tests/Rendering/PropertyTests/GLStateGuardTest.cpp +++ b/OloEngine/tests/Rendering/PropertyTests/GLStateGuardTest.cpp @@ -37,6 +37,18 @@ #include #include +namespace +{ + // See the call sites: glGetIntegerv marshals an all-bits stencil write mask + // through a signed GLint, and vendors differ on whether that clamps. + [[nodiscard]] inline bool IsAllBitsStencilMask(GLint value) + { + const unsigned int asUnsigned = static_cast(value); + return asUnsigned == 0xFFFFFFFFu // raw bit pattern (NVIDIA) + || asUnsigned == 0x7FFFFFFFu; // clamped to INT_MAX (Mesa/radeonsi) + } +} // namespace + namespace OloEngine::Tests { // Helper: does the diff list contain an entry whose field name starts @@ -414,13 +426,25 @@ namespace OloEngine::Tests ::glGetIntegerv(GL_STENCIL_BACK_VALUE_MASK, &stencilBackValueMask); EXPECT_EQ(static_cast(stencilBackValueMask), 0xFFu) << "StencilBackValueMask not restored"; + // An all-bits-set stencil write mask cannot be represented in the + // SIGNED GLint that glGetIntegerv fills, and the GL spec says + // out-of-range values are clamped. Mesa/radeonsi clamps to INT_MAX + // (0x7FFFFFFF); NVIDIA hands back the raw bit pattern (-1, which casts + // to 0xFFFFFFFF). Both are defensible readings, so accept either rather + // than pinning the test to one vendor's marshalling. + // + // The assertion still means what it meant: the guard restored the mask + // to "every bit the driver will report". GLint stencilWriteMask = 0; ::glGetIntegerv(GL_STENCIL_WRITEMASK, &stencilWriteMask); - EXPECT_EQ(static_cast(stencilWriteMask), 0xFFFFFFFFu) << "StencilWriteMask not restored"; + EXPECT_TRUE(IsAllBitsStencilMask(stencilWriteMask)) + << "StencilWriteMask not restored (got 0x" << std::hex << static_cast(stencilWriteMask) << ")"; GLint stencilBackWriteMask = 0; ::glGetIntegerv(GL_STENCIL_BACK_WRITEMASK, &stencilBackWriteMask); - EXPECT_EQ(static_cast(stencilBackWriteMask), 0xFFFFFFFFu) << "StencilBackWriteMask not restored"; + EXPECT_TRUE(IsAllBitsStencilMask(stencilBackWriteMask)) + << "StencilBackWriteMask not restored (got 0x" << std::hex + << static_cast(stencilBackWriteMask) << ")"; GLint stencilFail = 0; ::glGetIntegerv(GL_STENCIL_FAIL, &stencilFail); diff --git a/OloEngine/tests/Rendering/PropertyTests/RenderPropertyTest.cpp b/OloEngine/tests/Rendering/PropertyTests/RenderPropertyTest.cpp index 0fb330a22..9b3767ee6 100644 --- a/OloEngine/tests/Rendering/PropertyTests/RenderPropertyTest.cpp +++ b/OloEngine/tests/Rendering/PropertyTests/RenderPropertyTest.cpp @@ -90,6 +90,7 @@ namespace OloEngine::Tests #if defined(OLO_TESTS_HAVE_EGL) EGLDisplay m_EglDisplay = EGL_NO_DISPLAY; EGLContext m_EglContext = EGL_NO_CONTEXT; + EGLSurface m_EglSurface = EGL_NO_SURFACE; #endif static GpuContext& Get() @@ -243,17 +244,42 @@ namespace OloEngine::Tests if (context == EGL_NO_CONTEXT) return false; - // Surfaceless on purpose: every pixel the suite inspects is read - // back from an FBO, so a default framebuffer would go unused. - if (!::eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, context)) + // Bind a small pbuffer rather than going surfaceless. + // + // A surfaceless context has NO default framebuffer, and the + // renderer does touch framebuffer 0 — every RendererAttachedTest + // render tick then failed with + // + // GL_INVALID_FRAMEBUFFER_OPERATION + // + // which is how the whole visual/golden layer failed on the + // headless runner while the non-rendering tests sailed past. The + // pbuffer is 1x1 because nothing is ever presented to it; its + // only job is to give framebuffer 0 something complete to be. + constexpr EGLint pbufferAttribs[] = { + EGL_WIDTH, 1, + EGL_HEIGHT, 1, + EGL_NONE + }; + EGLSurface surface = ::eglCreatePbufferSurface(display, config, pbufferAttribs); + if (surface == EGL_NO_SURFACE) + { + ::eglDestroyContext(display, context); + return false; + } + + if (!::eglMakeCurrent(display, surface, surface, context)) { + ::eglDestroySurface(display, surface); ::eglDestroyContext(display, context); return false; } + m_EglSurface = surface; if (!LoadGladAndCheckVersion(reinterpret_cast(::eglGetProcAddress))) { ::eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); + ::eglDestroySurface(display, surface); ::eglDestroyContext(display, context); return false; } diff --git a/OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp b/OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp index 0c9bad795..20d880e57 100644 --- a/OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp +++ b/OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp @@ -355,6 +355,11 @@ namespace OloEngine::Tests::TestFailureCapture { m_Captured = false; m_FirstMessage.clear(); + // Remember the identity here, where gtest hands it to us + // directly. OnTestPartResult must NOT ask UnitTest for it — + // see the deadlock note there. + m_SuiteName = info.test_suite_name(); + m_TestName = info.name(); // Clear any stale capture directory from a previous run. Per // test rather than per binary so parallel invocations don't // race on the root directory. @@ -362,21 +367,59 @@ namespace OloEngine::Tests::TestFailureCapture fs::remove_all(DirectoryFor(info.test_suite_name(), info.name()), ec); } + void OnTestEnd(const ::testing::TestInfo&) override + { + // Drop the identity as soon as the test is over. A failure in + // a suite-level or global fixture (SetUpTestSuite, or an + // environment's SetUp/TearDown) still reaches + // OnTestPartResult, but with NO test active — and a stale + // identity here would file those diagnostics under whichever + // test happened to run last, which is worse than not + // capturing them: it looks like evidence about that test. + m_SuiteName.clear(); + m_TestName.clear(); + } + void OnTestPartResult(const ::testing::TestPartResult& result) override { if (!result.failed() || m_Captured) return; m_Captured = true; m_FirstMessage = result.summary(); - const auto* info = ::testing::UnitTest::GetInstance()->current_test_info(); - if (info == nullptr) + + // DO NOT call ::testing::UnitTest::GetInstance()->current_test_info() + // here. gtest holds its internal mutex while dispatching this + // callback, and current_test_info() takes that same mutex — + // so asking for the test's identity from inside the callback + // SELF-DEADLOCKS: + // + // __lll_lock_wait + // testing::internal::MutexBase::lock + // testing::UnitTest::current_test_info + // FailureListener::OnTestPartResult + // testing::UnitTest::AddTestPartResult + // + // + // The effect is that the FIRST failing assertion anywhere in + // the binary hangs the whole run instead of reporting — the + // failure reporter deadlocking while reporting a failure. It + // stalled a nightly for an hour on a test whose only crime was + // to fail, and it masks the real failure completely. + // + // OnTestStart already gave us the identity; use that. Both + // halves must be present — OnTestEnd clears them, so an empty + // pair means no test is active and there is nothing sensible + // to file the capture under. + if (m_SuiteName.empty() || m_TestName.empty()) return; - CaptureAll(info->test_suite_name(), info->name(), m_FirstMessage); + CaptureAll(m_SuiteName, m_TestName, m_FirstMessage); } private: bool m_Captured = false; std::string m_FirstMessage; + std::string m_SuiteName; + std::string m_TestName; }; static bool s_Registered = false; diff --git a/docs/ops/self-hosted-gpu-runner.md b/docs/ops/self-hosted-gpu-runner.md index b0aa0fc4e..1a10a4e13 100644 --- a/docs/ops/self-hosted-gpu-runner.md +++ b/docs/ops/self-hosted-gpu-runner.md @@ -106,17 +106,32 @@ If you add a third GPU gate with new wording, add its phrase there too. Most of the toolchain is already present. Mirroring `asan.yml`'s Linux dependency set, on Rocky 10: +**Everything the build needs must be installed system-wide** (`/usr`, `/opt`) — +never in a person's home directory. `/home/obueker` is mode `0700`, so anything +under it is invisible to `gh-runner-olo`. This bit three separate times during +bring-up: the Vulkan SDK, the runner tarball, and finally `cmake`/`ninja`, which +were pip installs in `/home/obueker/.local/bin` and produced a bare `cmake: +command not found`. The workflow now preflights the toolchain so the error names +the cause. + ```bash sudo dnf install -y \ - ccache \ + cmake ninja-build ccache gcc gcc-c++ \ vulkan-loader-devel vulkan-headers \ mesa-libGL-devel mesa-libEGL-devel libglvnd-devel \ libX11-devel libXrandr-devel libXinerama-devel libXcursor-devel libXi-devel libXext-devel \ wayland-devel wayland-protocols-devel libxkbcommon-devel \ - glslang-devel spirv-tools -python3 -m pip install --user jinja2 # OloHeaderTool codegen + glslang-devel spirv-tools \ + python3-jinja2 ``` +**`python3-jinja2` from dnf, not `pip install --user`.** glad2's code generation +imports jinja2, and `--user` installs it into the *installing* user's home — +invisible to `gh-runner-olo` for the same `0700` reason as everything else. That +mistake cost a run: all three preflights passed and the build died four minutes +in on `ModuleNotFoundError: No module named 'jinja2'`. The toolchain preflight +now checks importable modules as well as binaries. + The X11/Wayland `-devel` packages are needed to *build* GLFW even though no display server runs — GLFW compiles its backends unconditionally.