fix(ci): preflight the build toolchain on the self-hosted runner - #701
Open
drsnuggles8 wants to merge 10 commits into
Open
fix(ci): preflight the build toolchain on the self-hosted runner#701drsnuggles8 wants to merge 10 commits into
drsnuggles8 wants to merge 10 commits into
Conversation
The first real run died at Configure CMake with `cmake: command not
found`. cmake and ninja were pip installs under /home/obueker/.local/bin
and that home is mode 0700, so gh-runner-olo could not see them. ccache
was not installed at all, yet the configure passes
-DCMAKE_{C,CXX}_COMPILER_LAUNCHER=ccache.
This is the same root cause as the Vulkan SDK relocation and the runner
tarball extraction: the runner user's environment is not the admin's,
and anything under a 0700 home is invisible to it. Both preflights that
already existed passed -- hardware GL 4.6 and the Vulkan SDK were fine --
so the runner itself is sound; only the toolchain was unreachable.
Add a toolchain preflight that names every missing tool and prints the
dnf line to fix it, so this class of failure reads as "provision the
runner" instead of a bare command-not-found after a 39 s checkout.
Document that build dependencies must live system-wide, never in a
person's home, and add cmake/ninja-build/gcc/gcc-c++ to the package list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds ChangesEngine string and container changes
AMD runner toolchain
Rendering test portability
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/gpu-conformance-amd.yml:
- Around line 220-227: Move the “Preflight — build toolchain” step before
“Preflight — require hardware GL 4.6 (reject llvmpipe)” in the workflow, so its
toolchain validation and remediation message run before the GL probe invokes
gcc. Preserve both steps’ existing checks and commands.
In `@docs/ops/self-hosted-gpu-runner.md`:
- Around line 109-115: Update the Jinja2 installation instructions near the
provisioning steps to remove the per-user `python3 -m pip install --user jinja2`
command. Install Jinja2 through the system package manager or another shared,
runner-readable environment so `gh-runner-olo` can import it outside
`/home/obueker`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b48a1152-59c6-41a3-9285-a3beea89b59c
📒 Files selected for processing (2)
.github/workflows/gpu-conformance-amd.ymldocs/ops/self-hosted-gpu-runner.md
Configure failed 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. The LunarG SDK does not put the loader in lib/ -- it nests it under lib/VulkanLoader/lib/ -- so CMAKE_PREFIX_PATH resolves the headers and the version but never the library. This slipped through local verification because that check was not clean-slate: the build-gpu cache already held Vulkan_LIBRARY from an earlier explicit -DVulkan_LIBRARY= attempt, and the later CMAKE_PREFIX_PATH-only configure silently reused the cached value. A fresh build directory reproduces the CI failure exactly. Have the Vulkan preflight locate libvulkan.so and export the path, so the configure step gets a verified location instead of guessing at a layout that varies between SDK versions. Re-verified from a clean build directory under BOTH cmake 4.4 and the runner's distro cmake 3.31.8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stall The build died four minutes in with: ModuleNotFoundError: No module named 'jinja2' glad2's code generation imports jinja2. The ops doc told you to install it with `python3 -m pip install --user jinja2`, which puts it in the INSTALLING user's home -- invisible to gh-runner-olo for the same 0700 reason as the Vulkan SDK, the runner tarball and cmake/ninja before it. That instruction was wrong; use the distro package. The toolchain preflight checked binaries only, so it passed while the build needed something it never looked at -- the same shape as the Vulkan preflight passing while Vulkan_LIBRARY was unresolvable. Check importable modules as well, so a missing dependency is named up front instead of surfacing minutes into a cold build. Configure now succeeds, so the Vulkan loader fix holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The GL 4.6 preflight compiles its probe with gcc, but ran before the toolchain preflight that exists to diagnose a missing compiler. With that order a box without gcc fails on a bare `gcc: command not found` instead of the remediation message listing every missing tool and the dnf line to install them. Pure reordering -- both steps keep their checks verbatim -- plus a comment recording the dependency so the two are not swapped back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… test
MathBitwiseEqualTest.IntegerTypesAlsoWork failed on the self-hosted
Linux runner:
MathTest.cpp:116: Value of: BitwiseEqual(a, b)
Actual: false Expected: true
BitwiseEqual is memcmp over sizeof(T), so it compares PADDING as well as
members, and the language does not guarantee that copying an object
reproduces padding. The case built `const Trivial a{1.0f, 7, true};
Trivial b = a;` and asserted bit equality. MSVC and Clang copy the whole
object representation so it held there; GCC's implicit copy constructor
copies member-wise and leaves the destination's padding as whatever was
on the stack. Every member matched -- only bytes 9-11 differed.
Pre-existing; it surfaced now only because this is the first time the
suite has run under GCC. Value-initialise both objects (guaranteed to
zero padding bits since C++20) and assign members, which is the same
"zero-init for predictable equality" rule the helper's callers must
follow, and which the case's own comment already stated without doing.
Verified: the old form fails under gcc -O0/-O2/-O3 and passes under
clang -O2 (explaining why it went unnoticed); the new form passes under
all four, with the padding bytes confirmed zero.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both bugs are pre-existing and unrelated to strings; they surfaced while giving Submesh a relocatable string type, because FString introduced the engine's first TArray<char>. 1. The copy constructors call CopyToEmpty() on an array whose m_ArrayNum and m_ArrayMax are still indeterminate. CopyToEmpty sets m_ArrayNum but not m_ArrayMax, then calls ResizeAllocation(), which short-circuits on `if (NewMax != m_ArrayMax)`. When the uninitialised garbage happened to equal the computed NewMax the allocation was SKIPPED, leaving GetData() null for the ConstructItems memcpy that follows -- a write to address 0. Small element types expose it: their quantised capacities are small numbers that plausibly appear in recycled stack memory, where this engine's larger element types produce values that rarely collide. 2. Copy-assignment passed m_ArrayMax as CopyToEmpty's third argument. UE's signature takes PrevMax there and uses it to decide whether the existing allocation can be reused; this port takes ExtraSlack and computes `NewMax = Count + ExtraSlack`. Assignment therefore asked for Count + current capacity, growing the allocation without bound across repeated assignments to the same array. Also ports UE's relocatability guard (UE_STATIC_ASSERT_WARN in ~TArray, Array.h:1238) to TArray, TSparseArray, TDeque and TCompactSet, along with the [[deprecated]]-based warning macro it needs. Warning rather than hard assert, matching UE: the trait defaults to true and is opt-out, so this only fires for types someone has explicitly marked non-relocatable. Verified: the guard fires on TCompactSet<std::string> naming the culprit type, and is silent across the engine. NOTE it does NOT catch TMap<std::string, T> -- the TMap path evades instantiation of the guarded destructor and I have not established why, so that gap remains open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TArray relocates its elements BITWISE -- ResizeGrow goes through the
allocator's ResizeAllocation -> FMemory::Realloc on the raw byte buffer,
and insert/remove memmove via RelocateConstructItems. UE documents this
as a contract: "TArray (like many Unreal Engine containers) assumes that
the element type is trivially relocatable."
libstdc++'s std::string violates it. Under SSO its internal pointer aims
into its OWN inline buffer, so relocating leaves that pointer at the
element's old address and the destructor frees a non-heap pointer:
free(): invalid pointer (SIGABRT, ~TArray<Submesh> via ~MeshSource)
MSVC's std::string keeps no self-pointer, which is why this only ever
aborted against libstdc++ -- it took the first Linux GPU CI run to find.
UE's own string type has no such problem BY CONSTRUCTION: FString is a
single TArray<CharType> member with no small-string optimisation, so its
pointer always targets a separate heap block. That is precisely why UE
can assume relocatability engine-wide. This adds an FString with that
property, following UE's FUtf8String instantiation (char element) since
OloEngine's string surface is UTF-8 std::string throughout.
SCOPE: this is FString's design, storage layout, invariant and ~40 of
UE's ~140 methods -- NOT a transplant of Epic's 2298-line
UnrealString.h.inl. CString.h, StringConv.h, Char.h, Crc.h, VarArgs,
OutputDevice and the AutoRTFM coupling are not ported; none of them
affect the relocatability property this type exists to provide.
Converted:
* Submesh::m_NodeName / m_MeshName -- the crash site.
* Material's 28 TMap<std::string, T> uniform tables. TMap corrupts
std::string keys the same way; proven with a three-case control
matrix (std::string without growth passes, std::string with growth
corrupts, FString with growth passes), so relocation is the
mechanism and FString is the remedy.
The asset-pack and mesh-cache formats are byte-identical: reads go
through std::string temporaries and writes through ToStdString(), so no
cached .omesh or shipped pack is invalidated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vendor goldens
Five problems that between them made the suite unable to report anything
useful on the self-hosted AMD runner.
1. TestFailureCapture's listener SELF-DEADLOCKED. gtest holds its
internal mutex while dispatching OnTestPartResult, and the listener
called UnitTest::current_test_info(), which takes that same mutex:
__lll_lock_wait
testing::internal::MutexBase::lock
testing::UnitTest::current_test_info
FailureListener::OnTestPartResult
testing::UnitTest::AddTestPartResult
<the failing EXPECT_/ASSERT_>
The failure reporter deadlocked while reporting a failure, so the
FIRST failing assertion anywhere in the binary hung the whole run
instead of reporting -- and hid the real failure completely. That is
what stalled the nightly. OnTestStart already receives the TestInfo;
stash it there instead of asking for it under the lock.
2. The headless EGL context was SURFACELESS, so it had no default
framebuffer -- and the renderer does touch framebuffer 0. Every
RendererAttachedTest render tick failed with
GL_INVALID_FRAMEBUFFER_OPERATION, which silently disabled the entire
visual/golden layer: ZERO visual tests ran. Bind a 1x1 pbuffer.
3. AtmosphereVisualEvidenceTest ignored OLOENGINE_GOLDEN_VENDOR -- only
GoldenImageTests honoured it. An AMD run compared against, and on a
rebase would have OVERWRITTEN, the NVIDIA baselines.
4. GLStateGuardTest pinned the stencil write mask to NVIDIA's
marshalling. glGetIntegerv returns a signed GLint and the spec says
out-of-range values are clamped, so Mesa returns INT_MAX where NVIDIA
returns the raw 0xFFFFFFFF bit pattern. Mesa is arguably the more
spec-correct of the two; accept either.
5. The workflow now excludes NetworkIntegrationTest, matching every
other job in the repo (asan.yml excludes it on all three Linux
sanitiser jobs) -- it needs socket plumbing CI does not provide and is
not a GPU test.
Also fixes a REAL rendering bug the AMD run exposed (InfiniteGrid.glsl):
the grid lies on Y=0 and scenes routinely put a ground plane there too.
The ground gets the rasteriser's interpolated depth while the grid writes
gl_FragDepth from an unprojected-then-reprojected position; the two
disagree by ~1 ULP and the depth test resolved the tie on float noise.
NVIDIA resolved it consistently, Mesa did not, and the grid shattered
into dashes toward the horizon. A 1e-5 depth bias makes the grid win
deterministically on every vendor. Verified visually -- the grid renders
clean and continuous -- and the night captures' RMSE against the NVIDIA
goldens dropped 24.4->13.1, 23.0->9.4, and 19.4->passing. The residual
is star-field hash divergence, which is genuine vendor variance.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/OloEngine/Containers/String.h`:
- Around line 230-260: Update AppendChars to safely handle source pointers that
overlap the string’s current Data buffer before Data.SetNumUninitialized can
reallocate it. Preserve existing no-op and append behavior for non-overlapping
inputs, and ensure both Append(const FString& other) and Append(std::string_view
sv) remain safe when their sources alias this string.
- Around line 70-72: Update the String class member naming to follow project
conventions: rename the Data member to m_Data and the static constexpr
InvalidIndex constant to kInvalidIndex, updating all references accordingly. If
retaining Data to mirror UE’s layout is required, add a concise class-level note
documenting the intentional naming exception instead.
- Line 350: Update the default search-case behavior in the Find API to use
ESearchCase::IgnoreCase, ensuring Contains inherits the same default through its
Find delegation. If retaining case-sensitive behavior, add a clear deviation
note next to the ported API instead.
In `@OloEngine/src/OloEngine/Renderer/Material.h`:
- Around line 413-465: Update Material’s uniform Set/Get lookup paths to avoid
constructing a temporary FString from each const std::string& name; use
std::string_view or const char*-compatible TMap lookup APIs, or add
corresponding overloads, while preserving existing FindOrAdd and Find behavior
across all uniform maps and texture maps.
In `@OloEngine/tests/MathTest.cpp`:
- Around line 105-110: Update the local Trivial struct by renaming its data
members X, Y, and Z to m_X, m_Y, and m_Z, and adjust all corresponding member
assignments and accesses in the surrounding test code.
- Around line 124-136: Update the padded-object setup in the test to use
explicit value-initialization for both objects, replacing the current brace
declarations with the requested `Trivial()` form. Revise the surrounding comment
to accurately describe value-initialization and its role in deterministic
`BitwiseEqual(a, b)` comparisons.
In `@OloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cpp`:
- Around line 151-157: Update GoldenBaselineDir so OLOENGINE_GOLDEN_VENDOR is
accepted only as a single safe directory name: reject empty, absolute/rooted
paths, path separators, and "." or ".." components before appending it to the
base path. Preserve the default assets/tests/visual directory when the value is
invalid.
In `@OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp`:
- Around line 397-406: Update OnTestEnd to clear both m_SuiteName and m_TestName
after each test, preventing stale identities from being reused. In
OnTestPartResult, require both identity fields to be non-empty before calling
CaptureAll, while preserving the existing m_FirstMessage and m_Captured
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6295654a-55f5-4523-a502-4402ecca4541
⛔ Files ignored due to path filters (1)
OloEditor/assets/shaders/InfiniteGrid.glslis excluded by!**/*.glsl
📒 Files selected for processing (23)
.github/workflows/gpu-conformance-amd.ymlOloEngine/src/OloEngine/Asset/AssetSerializer.cppOloEngine/src/OloEngine/Asset/Interchange/AssimpMeshExporter.cppOloEngine/src/OloEngine/Containers/Array.hOloEngine/src/OloEngine/Containers/CompactSet.hOloEngine/src/OloEngine/Containers/Deque.hOloEngine/src/OloEngine/Containers/SparseArray.hOloEngine/src/OloEngine/Containers/String.hOloEngine/src/OloEngine/Core/YAMLConverters.hOloEngine/src/OloEngine/Renderer/Material.hOloEngine/src/OloEngine/Renderer/MeshSource.hOloEngine/src/OloEngine/Renderer/Model.cppOloEngine/src/OloEngine/Renderer/Renderer3DMeshSubmission.cppOloEngine/src/OloEngine/Serialization/MeshBinarySerializer.cppOloEngine/src/OloEngine/Templates/UnrealTypeTraits.hOloEngine/tests/CMakeLists.txtOloEngine/tests/ContainerTest.cppOloEngine/tests/Containers/StringTest.cppOloEngine/tests/MathTest.cppOloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cppOloEngine/tests/Rendering/PropertyTests/GLStateGuardTest.cppOloEngine/tests/Rendering/PropertyTests/RenderPropertyTest.cppOloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp
Comment on lines
+70
to
+72
| // Array holding the character data (UE's member name and layout). | ||
| using DataType = TArray<ElementType, AllocatorType>; | ||
| DataType Data; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Naming does not follow the project's member/constant conventions.
Data (line 72) lacks the m_ prefix required for members, and InvalidIndex (line 350) is static constexpr but lacks the k prefix used elsewhere in the codebase for compile-time constants. The comment at lines 70-71 explains Data deliberately mirrors UE's member name; if that tradeoff is intentional, consider a short note next to the class stating it's an accepted naming exception. Otherwise, rename to m_Data and kInvalidIndex for consistency.
Based on learnings, "use a kPascalCase/k-prefix naming style for static constexpr compile-time constants... Reserve s_PascalCase exclusively for mutable static member variables," and as per coding guidelines, "naming conventions of PascalCase classes, m_PascalCase members, s_PascalCase statics."
Also applies to: 350-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/OloEngine/Containers/String.h` around lines 70 - 72, Update the
String class member naming to follow project conventions: rename the Data member
to m_Data and the static constexpr InvalidIndex constant to kInvalidIndex,
updating all references accordingly. If retaining Data to mirror UE’s layout is
required, add a concise class-level note documenting the intentional naming
exception instead.
Sources: Coding guidelines, Learnings
Comment on lines
+230
to
+260
| FString& AppendChars(const char* str, SizeType count) | ||
| { | ||
| if (!str || count <= 0) | ||
| return *this; | ||
|
|
||
| const SizeType oldLen = Len(); | ||
| // +1 for the terminator; Data may be completely empty here. | ||
| Data.SetNumUninitialized(oldLen + count + 1); | ||
| std::memcpy(Data.GetData() + oldLen, str, static_cast<sizet>(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<SizeType>(std::strlen(str)) : 0); | ||
| } | ||
| FString& Append(std::string_view sv) | ||
| { | ||
| return AppendChars(sv.data(), static_cast<SizeType>(sv.size())); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix use-after-free on self-append (s.Append(s) / s += s).
Append(const FString& other) passes *other (a raw pointer into other.Data) to AppendChars before that call runs. If other is *this, AppendChars then calls Data.SetNumUninitialized(oldLen + count + 1), which can reallocate Data's buffer (the header comment for this file documents that growth goes through FMemory::Realloc, which "moves the raw byte buffer"). The memcpy that follows then reads from the now-freed old buffer through the dangling str pointer. The same hazard applies to Append(std::string_view sv) if the view aliases this string's own storage.
Guard AppendChars against a source pointer that overlaps Data's current buffer, or make a defensive copy at the Append(const FString&) call site before mutating Data.
🐛 Proposed fix for self-aliasing in AppendChars
FString& AppendChars(const char* str, SizeType count)
{
if (!str || count <= 0)
return *this;
const SizeType oldLen = Len();
- // +1 for the terminator; Data may be completely empty here.
- Data.SetNumUninitialized(oldLen + count + 1);
- std::memcpy(Data.GetData() + oldLen, str, static_cast<sizet>(count));
- Data.GetData()[oldLen + count] = '\0';
- CheckInvariants();
- return *this;
+
+ // Self-aliasing guard: if `str` points inside this string's own buffer
+ // (e.g. `s.Append(s)` / `s += s`), SetNumUninitialized below can
+ // reallocate and free that buffer before the memcpy reads from it.
+ const char* selfBegin = Data.GetData();
+ const char* selfEnd = selfBegin + Data.Num();
+ if (str >= selfBegin && str < selfEnd)
+ {
+ std::vector<char> temp(str, str + count);
+ Data.SetNumUninitialized(oldLen + count + 1);
+ std::memcpy(Data.GetData() + oldLen, temp.data(), static_cast<sizet>(count));
+ Data.GetData()[oldLen + count] = '\0';
+ CheckInvariants();
+ return *this;
+ }
+
+ // +1 for the terminator; Data may be completely empty here.
+ Data.SetNumUninitialized(oldLen + count + 1);
+ std::memcpy(Data.GetData() + oldLen, str, static_cast<sizet>(count));
+ Data.GetData()[oldLen + count] = '\0';
+ CheckInvariants();
+ return *this;
}Also applies to: 262-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/OloEngine/Containers/String.h` around lines 230 - 260, Update
AppendChars to safely handle source pointers that overlap the string’s current
Data buffer before Data.SetNumUninitialized can reallocate it. Preserve existing
no-op and append behavior for non-overlapping inputs, and ensure both
Append(const FString& other) and Append(std::string_view sv) remain safe when
their sources alias this string.
| // Search | ||
| // ------------------------------------------------------------------ | ||
|
|
||
| static constexpr SizeType InvalidIndex = -1; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Unreal Engine 5 FString::Find default ESearchCase parameter
💡 Result:
In Unreal Engine 5, the default value for the SearchCase parameter (which uses the ESearchCase::Type enumeration) in the FString::Find function is ESearchCase::IgnoreCase [1]. The function signature typically appears as: FORCEINLINE int32 Find(const FString& SubStr, ESearchCase::Type SearchCase = ESearchCase::IgnoreCase, ESearchDir::Type SearchDir = ESearchDir::FromStart, int32 StartPosition = INDEX_NONE) const [1] When using ESearchCase::IgnoreCase, the string search will not distinguish between uppercase and lowercase characters [2][3]. If you require a case-sensitive search, you must explicitly pass ESearchCase::CaseSensitive as the argument [2].
Citations:
- 1: https://github.com/Michidu/ARK-Server-API/blob/master/version/Core/Public/API/UE/Containers/FString.h
- 2: https://www.programmersought.com/article/69987165241/
- 3: https://dev.epicgames.com/documentation/en-us/unreal-engine/API/Runtime/Core/ESearchCase__Type
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant string containers header around the reviewed symbols.
fd -a 'String.h' . | sed 's#^\./##'
file="$(fd 'String.h' . | head -n 1)"
if [ -n "$file" ]; then
echo "---- $file lines 320-420 ----"
sed -n '320,420p' "$file" | nl -ba -v320
echo "---- definitions/usages of Contains/Find/ESearchCase in $file ----"
rg -n "Contains|Find|enum class ESearchCase|struct ESearchCase|CaseSensitive|IgnoreCase" "$file"
fi
echo "---- repository-wide ESearchCase/Contains/Find context ----"
rg -n "struct ESearchCase|enum class ESearchCase|ESearchCase::CaseSensitive|ESearchCase::IgnoreCase|Contains\\([^\\n]*E?SearchCase|Find\\([^\\n]*E?SearchCase" .Repository: drsnuggles8/OloEngineBase
Length of output: 336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./OloEngine/src/OloEngine/Containers/String.h"
echo "---- $file lines 320-420 ----"
python3 - <<'PY'
from pathlib import Path
p = Path("./OloEngine/src/OloEngine/Containers/String.h")
lines = p.read_text().splitlines()
for i in range(319, min(421, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PY
echo "---- definitions/usages of Contains/Find/ESearchCase in $file ----"
rg -n "Contains|Find|ESearchCase|CaseSensitive|IgnoreCase" "$file" || true
echo "---- repository-wide ESearchCase/Contains/Find context ----"
rg -n "struct ESearchCase|enum class ESearchCase|ESearchCase::CaseSensitive|ESearchCase::IgnoreCase|Contains\\([^\\n]*E?SearchCase|Find\\([^\\n]*E?SearchCase" . || trueRepository: drsnuggles8/OloEngineBase
Length of output: 7871
Align Find defaults with UE FString semantics.
UE 5 FString::Find defaults ESearchCase to IgnoreCase; Contains delegates to Find here and is also case-sensitive by default. Use ESearchCase::IgnoreCase for Find, or add a clear deviation note near the ported UE API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/OloEngine/Containers/String.h` at line 350, Update the default
search-case behavior in the Find API to use ESearchCase::IgnoreCase, ensuring
Contains inherits the same default through its Find delegation. If retaining
case-sensitive behavior, add a clear deviation note next to the ported API
instead.
Comment on lines
+413
to
+465
| const TMap<FString, float>& GetFloatUniforms() const | ||
| { | ||
| return m_FloatUniforms; | ||
| } | ||
| const TMap<std::string, int>& GetIntUniforms() const | ||
| const TMap<FString, int>& GetIntUniforms() const | ||
| { | ||
| return m_IntUniforms; | ||
| } | ||
| const TMap<std::string, u32>& GetUIntUniforms() const | ||
| const TMap<FString, u32>& GetUIntUniforms() const | ||
| { | ||
| return m_UIntUniforms; | ||
| } | ||
| const TMap<std::string, bool>& GetBoolUniforms() const | ||
| const TMap<FString, bool>& GetBoolUniforms() const | ||
| { | ||
| return m_BoolUniforms; | ||
| } | ||
| const TMap<std::string, glm::vec2>& GetVec2Uniforms() const | ||
| const TMap<FString, glm::vec2>& GetVec2Uniforms() const | ||
| { | ||
| return m_Vec2Uniforms; | ||
| } | ||
| const TMap<std::string, glm::vec3>& GetVec3Uniforms() const | ||
| const TMap<FString, glm::vec3>& GetVec3Uniforms() const | ||
| { | ||
| return m_Vec3Uniforms; | ||
| } | ||
| const TMap<std::string, glm::vec4>& GetVec4Uniforms() const | ||
| const TMap<FString, glm::vec4>& GetVec4Uniforms() const | ||
| { | ||
| return m_Vec4Uniforms; | ||
| } | ||
| const TMap<std::string, glm::ivec2>& GetIVec2Uniforms() const | ||
| const TMap<FString, glm::ivec2>& GetIVec2Uniforms() const | ||
| { | ||
| return m_IVec2Uniforms; | ||
| } | ||
| const TMap<std::string, glm::ivec3>& GetIVec3Uniforms() const | ||
| const TMap<FString, glm::ivec3>& GetIVec3Uniforms() const | ||
| { | ||
| return m_IVec3Uniforms; | ||
| } | ||
| const TMap<std::string, glm::ivec4>& GetIVec4Uniforms() const | ||
| const TMap<FString, glm::ivec4>& GetIVec4Uniforms() const | ||
| { | ||
| return m_IVec4Uniforms; | ||
| } | ||
| const TMap<std::string, glm::mat3>& GetMat3Uniforms() const | ||
| const TMap<FString, glm::mat3>& GetMat3Uniforms() const | ||
| { | ||
| return m_Mat3Uniforms; | ||
| } | ||
| const TMap<std::string, glm::mat4>& GetMat4Uniforms() const | ||
| const TMap<FString, glm::mat4>& GetMat4Uniforms() const | ||
| { | ||
| return m_Mat4Uniforms; | ||
| } | ||
| const TMap<std::string, Ref<Texture2D>>& GetTexture2DUniforms() const | ||
| const TMap<FString, Ref<Texture2D>>& GetTexture2DUniforms() const | ||
| { | ||
| return m_Texture2DUniforms; | ||
| } | ||
| const TMap<std::string, Ref<TextureCubemap>>& GetTextureCubeUniforms() const | ||
| const TMap<FString, Ref<TextureCubemap>>& GetTextureCubeUniforms() const |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Material::Set/Get implementations for TMap<FString,...> lookup patterns and call frequency.
ast-grep outline OloEngine/src/OloEngine/Renderer/Material.cpp --items all --type function --match 'Set|Get' 2>/dev/null || true
rg -n -C5 '\bMaterial::(Set|Get)(Float|Int|UInt|Bool|Vector|IntVector|Matrix|Texture)' OloEngine/src/OloEngine/Renderer/Material.cppRepository: drsnuggles8/OloEngineBase
Length of output: 7492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Material uniform API call sites =="
rg -n -C3 '(\bMaterial::Set\b|->Set\(|\.Set\(|\\bMaterial::GetFloat\b|\\bMaterial::Get(Bool|Int|UInt|IntVector|Vector|Matrix)4?\\b|->GetFloat\\(|->Get(Bool|Int|UInt|IntVector|Vector|Matrix)4?\\(|\.Get(Float|Bool|Int|UInt|IntVector|Vector|Matrix)4?\()' OloEngine/src | sed -n '1,240p'
echo
echo "== FString key constructor implementation candidates =="
rg -n -C4 'class FString|struct FString|FString\(const std::string|FString\(&&|operator=|small|Small-string|allocat|Small allocat' OloEngine/src -g '*.{h,hpp,cpp}' | sed -n '1,260p'
echo
echo "== TMap key lookup signatures =="
rg -n -C4 'class TMap|struct TMap|Find\(|Insert\(|operator\\[' OloEngine/src -g '*.{h,hpp,cpp}' | sed -n '1,220p'Repository: drsnuggles8/OloEngineBase
Length of output: 503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Material uniform API call sites =="
rg -n -C3 '(\bMaterial::Set\b|->Set\(|\.Set\(|Material::GetFloat\b|GetFloat\(|GetBoolean|GetFloat|GetBool|GetInt|GetUInt|GetIntVector|GetVector|GetMatrix|->GetFloat\(|->GetBoolean|->GetFloat|->GetBool|->GetInt|->GetUInt|->GetIntVector|->GetVector|->GetMatrix|\.GetFloat\(|\.GetBoolean|\.GetFloat|\.GetBool|\.GetInt|\.GetUInt|\.GetIntVector|\.GetVector|\.GetMatrix)' OloEngine/src | sed -n '1,280p'
echo
echo "== FString implementation candidates =="
rg -n -C4 'class FString|struct FString|FString\(const std::string|FString\(&&|operator=|small|Small-string|allocat|heap|stack' OloEngine/src -g '*.{h,hpp,cpp}' | sed -n '1,280p'
echo
echo "== TMap/FString key lookup signatures =="
rg -n -C4 'class TMap|struct TMap|Find\(|FindAndRemove|Insert\(|Emplace\(|operator\\[[' OloEngine/src -g '*.{h,hpp,cpp}' | sed -n '1,240p'Repository: drsnuggles8/OloEngineBase
Length of output: 49351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact Material Set/Get call sites =="
rg -n -C3 '\.Set\(|->Set\(|Material::Set\(|Material::(GetFloat|GetBool|GetInt|GetUInt|GetIntVector|GetVector|GetMatrix)\(' OloEngine/src | sed -n '1,260p'
echo
echo "== exact FString definitions =="
rg -n -C8 'class[[:space:]]+FString|struct[[:space:]]+FString|class FString|struct FString' OloEngine/src -g '*.{h,hpp,cpp}' | sed -n '1,300p'
echo
echo "== exact TMap declarations/signatures =="
rg -n -C8 'class[[:space:]]+TMap|struct[[:space:]]+TMap|class TMap|struct TMap' OloEngine/src -g '*.{h,hpp,cpp}' | sed -n '1,320p'
echo
echo "== exact TMap operations used by Material implementation =="
rg -n -C3 'FindOrAdd\(|Find\(|GenerateArrayKey\(' OloEngine/src/OloEngine/Renderer/Material.cpp | sed -n '1,220p'Repository: drsnuggles8/OloEngineBase
Length of output: 36084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FString full body =="
sed -n '61,180p' OloEngine/src/OloEngine/Containers/String.h
echo
echo "== Map operation signatures and key-lookup templating =="
sed -n '199,360p' OloEngine/src/OloEngine/Containers/Map.h
sed -n '1220,1435p' OloEngine/src/OloEngine/Containers/Map.h
sed -n '1441,1640p' OloEngine/src/OloEngine/Containers/Map.h
echo
echo "== TSortableMapBase relevant operations =="
rg -n -C5 'class TSortableMapBase|FindOrAdd\(|FindNonConst\(|FindConst|AddToMap|RemoveFromMap|Sort|KeyFuncs::' OloEngine/src/OloEngine/Containers/Map.h | sed -n '1,260p'Repository: drsnuggles8/OloEngineBase
Length of output: 36559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TArray allocator and construction details =="
rg -n -C6 'class TSizedDefaultAllocator|struct TSizedDefaultAllocator|class TArray|TSizedDefaultAllocator<32>|constexpr inline.*Alloc|RAlloc|ConstructFromPtrSize|operator new|Delete(void' OloEngine/src/OloEngine/Containers/String.h OloEngine/src/OloEngine/Containers -g '*.{h,hpp,cpp}' | sed -n '1,260p'
echo
echo "== exact FString Data type constructors in String.h =="
sed -n '1,45p' OloEngine/src/OloEngine/Containers/String.h
rg -n -C12 'ConstructFromPtrSize|Alloc|Allocate|ConstructElements|NewMemory|RAlloc|TSizedDefaultAllocator' OloEngine/src/OloEngine/Containers/String.h OloEngine/src/OloEngine/Containers/Array.h OloEngine/src/OloEngine/Algo -g '*.{h,hpp,cpp}' | sed -n '1,260p'
echo
echo "== exact Map lookup key template definitions =="
sed -n '723,850p' OloEngine/src/OloEngine/Containers/Map.hRepository: drsnuggles8/OloEngineBase
Length of output: 446
Reduce per-call FString allocation in uniform map access.
Material::Set/Get still take const std::string& while the maps are TMap<FString, T>. These calls pass name directly into FindOrAdd/Find, which materializes an FString for every lookup, and FString backs data in a heap-allocated TArray. Switch Material to look up with std::string_view or const char*-compatible methods, or add overloads that take std::string_view, to avoid construction for short uniform-namex.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/OloEngine/Renderer/Material.h` around lines 413 - 465, Update
Material’s uniform Set/Get lookup paths to avoid constructing a temporary
FString from each const std::string& name; use std::string_view or const
char*-compatible TMap lookup APIs, or add corresponding overloads, while
preserving existing FindOrAdd and Find behavior across all uniform maps and
texture maps.
Comment on lines
105
to
110
| 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. | ||
| }; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the required member naming convention.
Rename Trivial::X, Trivial::Y, and Trivial::Z to m_X, m_Y, and m_Z. Update the member assignments accordingly.
As per coding guidelines, C++ members use m_PascalCase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/MathTest.cpp` around lines 105 - 110, Update the local
Trivial struct by renaming its data members X, Y, and Z to m_X, m_Y, and m_Z,
and adjust all corresponding member assignments and accesses in the surrounding
test code.
Source: Coding guidelines
Comment on lines
+124
to
+136
| // Value-initialisation zeroes padding bits (guaranteed since C++20), | ||
| // 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. | ||
| Trivial a{}; | ||
| a.X = 1.0f; | ||
| a.Y = 7; | ||
| a.Z = true; | ||
|
|
||
| Trivial b{}; | ||
| b.X = 1.0f; | ||
| b.Y = 7; | ||
| b.Z = true; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="OloEngine/tests/MathTest.cpp"
! rg -n 'Trivial [ab]\{\}' "$file"
rg -n 'Trivial [ab] = Trivial\(\)' "$file"Repository: drsnuggles8/OloEngineBase
Length of output: 213
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== MathTest.cpp relevant section =="
sed -n '100,150p' OloEngine/tests/MathTest.cpp
echo "== BitwiseEqual usages/definition =="
rg -n "BitwiseEqual|struct Trivial|class Trivial|union Trivial" OloEngine/tests/OloEngineRepository: drsnuggles8/OloEngineBase
Length of output: 2638
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Locate BitwiseEqual and related test files =="
fd -a '.*\.(cpp|hpp|h)$' OloEngine | sed 's#^\./##' | head -200
rg -n "BitwiseEqual|struct Trivial|class Trivial|union Trivial" OloEngine/tests OloEngine -g '*.{cpp,h,hpp}' 2>/dev/null || true
echo "== C++ syntax probe for aggregate initialization vs value initialization with padding context =="
python3 - <<'PY'
# Read-only behavioral/logic model: this is not a compiler.
# It models whether the source form matches the claim in the review comment,
# and maps the C++20/23 value-initialization path to full object zero-initialization.
cases = [
("aggregate form", "Trivial a{};", "a{} uses brace-or-equal member initialization if declared with no constructor and no brace initialization occurs, leaving enclosing unused padding unspecified."),
("explicit value-init form", "Trivial a = Trivial();", "a = Trivial() performs copy-initialization of a value-initialized temporary, guaranteeing full object zero-initialization for a literal type aggregate."),
]
for label, src, note in cases:
print(f"{label}\n src={src}\n note={note}")
PYRepository: drsnuggles8/OloEngineBase
Length of output: 13526
Use explicit value-initialization for the padded objects.
Trivial a{} and Trivial b{} are direct list-initialization, not full value-initialization of the enclosing object under the standard wording. Padding can remain unspecified, so BitwiseEqual(a, b) can still depend on the compiler. Use Trivial a = Trivial(); and Trivial b = Trivial();, then update the comment to describe value-initialization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/MathTest.cpp` around lines 124 - 136, Update the
padded-object setup in the test to use explicit value-initialization for both
objects, replacing the current brace declarations with the requested `Trivial()`
form. Revise the surrounding comment to accurately describe value-initialization
and its role in deterministic `BitwiseEqual(a, b)` comparisons.
Source: Coding guidelines
Comment on lines
+151
to
+157
| [[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') | ||
| { | ||
| base /= vendor; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Restrict OLOENGINE_GOLDEN_VENDOR to one safe directory name.
Line 156 accepts absolute paths and traversal components such as ../../captures. When OLOENGINE_GOLDEN_REBASE=1, this can write a golden outside assets/tests/visual. Reject separators, . and .., and root paths before appending the vendor value.
Proposed fix
if (const char* vendor = std::getenv("OLOENGINE_GOLDEN_VENDOR"); vendor != nullptr && vendor[0] != '\0')
{
+ const std::string_view vendorName{ vendor };
+ if (vendorName == "." || vendorName == ".." ||
+ vendorName.find_first_of("/\\") != std::string_view::npos)
+ {
+ throw std::runtime_error("OLOENGINE_GOLDEN_VENDOR must be a single directory name");
+ }
base /= vendor;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [[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') | |
| { | |
| base /= vendor; | |
| } | |
| [[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') | |
| { | |
| const std::string_view vendorName{ vendor }; | |
| if (vendorName == "." || vendorName == ".." || | |
| vendorName.find_first_of("/\\") != std::string_view::npos) | |
| { | |
| throw std::runtime_error("OLOENGINE_GOLDEN_VENDOR must be a single directory name"); | |
| } | |
| base /= vendor; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/Rendering/PropertyTests/AtmosphereVisualEvidenceTest.cpp`
around lines 151 - 157, Update GoldenBaselineDir so OLOENGINE_GOLDEN_VENDOR is
accepted only as a single safe directory name: reject empty, absolute/rooted
paths, path separators, and "." or ".." components before appending it to the
base path. Preserve the default assets/tests/visual directory when the value is
invalid.
Comment on lines
+397
to
+406
| if (m_SuiteName.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; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect listener lifecycle handling and any fixture-level failure coverage.
rg -n -C 4 --glob '*.{cc,cpp,h,hpp}' \
'OnTestStart|OnTestEnd|OnTestSuiteStart|OnTestSuiteEnd|OnTestPartResult|SetUpTestSuite|TearDownTestSuite' \
OloEngineRepository: drsnuggles8/OloEngineBase
Length of output: 27914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp"
echo "---- file outline ----"
ast-grep outline "$file" --view expanded || true
echo "---- relevant source ----"
sed -n '320,425p' "$file" | cat -n
echo "---- declaration ----"
sed -n '1,80p' OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.h | cat -n
# Behavioral probe: model the current listener state transitions and the proposed guard.
python3 - <<'PY'
def current(suite, test, failed, first, captured, m_suite_name, m_test_name):
# OnTestStart
captured = False
first = ""
m_suite_name = suite
m_test_name = test
# No-op failure in active test
if failed and not captured and suite and test:
captured = True
# Listener state reaches another non-active failure: no new state for current code when m_suite_name is non-empty
return m_suite_name, m_test_name, captured, first
def proposed(s, t, failed, captured, first, m_suite_name, m_test_name, listener_count=1, on_fail=None):
captures = []
# OnTestStart
captured = False
first = ""
m_suite_name = s
m_test_name = t
captured = captured or (failed and not captured and s and t and (on_fail and on_fail("active")))
for listener_idx in range(listener_count):
# OnTestEnd only clears names in the proposed fix
m_suite_name = ""
m_test_name = ""
# Fail after active test
captures.append((failed and not captured and m_suite_name and m_test_name, m_suite_name, m_test_name))
return captures, m_suite_name, m_test_name
current_state = current("PreviousFixSuite", "PreviousFixTest", True, "First", False, "", "")
print("current_active_then_external_failure_state:", current_state)
try:
proposed_state = proposed("", "", True, False, "", "", "")
print("proposed_after_test_clear_for_external_failure:", proposed_state)
except Exception as e:
print("proposed_after_test_clear_error:", repr(e))
PYRepository: drsnuggles8/OloEngineBase
Length of output: 11358
Clear the cached identity after each test.
A suite or global fixture failure can reach OnTestPartResult when no test is active. If a prior test populated these fields, CaptureAll can write diagnostics under the prior test’s directory. Clear m_SuiteName and m_TestName in OnTestEnd, and require both identities before capture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/Rendering/PropertyTests/TestFailureCapture.cpp` around lines
397 - 406, Update OnTestEnd to clear both m_SuiteName and m_TestName after each
test, preventing stale identities from being reused. In OnTestPartResult,
require both identity fields to be non-empty before calling CaptureAll, while
preserving the existing m_FirstMessage and m_Captured behavior.
Closes the gap noted in 6d465dd: the ~TArray/~TSparseArray guard caught direct container misuse but never fired for TMap<std::string, T> -- the exact case that corrupted Material's uniform tables. The chain broke in four steps, each individually reasonable: 1. TSet resolves to TSparseSet, NOT TCompactSet. Set.h's OLO_USE_COMPACT_SET_AS_DEFAULT defaults to 1, which reads as though TCompactSet wins; the type only revealed itself by making the compiler print it. 2. TSparseSet stores TSparseArray<TSparseSetElement<T>>. 3. TSparseSetElement<T> had NO relocatability specialisation, so it fell through to the permissive default and reported itself relocatable regardless of T. 4. The guard on TSparseArray therefore saw a "safe" wrapper. Every other layer was already correct -- std::string is marked non-relocatable and TTuple propagates that to TPair -- and the wrapper discarded the information one layer before the guard could act on it. With the specialisation the guard names the whole chain: InElementType = TSparseSetElement<TTuple<std::string, float>> "This container can only be used with trivially relocatable types" It immediately flagged one more instance, ContainerSmoke.TMapAddFindRemove (TMap<i32, std::string>), now converted to FString. That was a test, not engine code; the engine has no remaining string-keyed UE containers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng a live attachment
Two independent correctness fixes found while investigating the AMD-only
OceanFFT and GTAO failures. NEITHER fixes its test -- both are real bugs
that turned out to sit next to the actual causes. Recorded as such rather
than claimed as fixes.
1. GTAO.comp decoded the (-2,-2) "no normal here" sentinel as if it were
a real octahedral normal. Ten shaders write that sentinel when they
draw into the scene normal RT without a meaningful surface normal:
InfiniteGrid, the four Particle_* shaders, and the Renderer2D_*
quad/circle/line/text/polygon shaders. SSAO.glsl has always guarded it
("sentinel value (-2,-2) means no normal", SSAO.glsl:109); GTAO.comp
did not, so it fabricated occlusion wherever those shaders drew.
Match SSAO: no normal means full visibility.
This does NOT close the forward-vs-deferred GTAO gap -- GTAO also
samples DEPTH, and neighbouring samples still read the contaminated
depth even when the centre pixel is skipped. See the handoff note in
the PR for the real cause.
2. WaterRenderPass read board.Scene.SceneDepthAttachment -- the live depth
attachment of the very framebuffer it renders into (it WriteNewVersions
SceneColor). Sampling a texture attached to the bound framebuffer is a
GL feedback loop: undefined behaviour, not merely discouraged.
ContactShadowRenderPass and FogRenderPass both read the
Scene.SceneDepth snapshot instead; Water was the odd one out.
Verified the change actually takes effect (the pass now resolves
texture 51, the snapshot, rather than 56, the attachment) -- and
verified it does NOT change the render, so the feedback loop was not
the cause of the AMD magenta-seafloor failure. Kept because sampling a
bound attachment is UB regardless of whether this GPU tolerated it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first run died at Configure CMake with
cmake: command not found— cmake and ninja were pip installs under /home/obueker/.local/bin, and that home is mode 0700, so gh-runner-olo could not see them. ccache was not installed at all despite the configure passing -DCMAKE_{C,CXX}_COMPILER_LAUNCHER=ccache.Same root cause as the Vulkan SDK relocation and the tarball extraction: the runner user's environment is not the admin's. Both existing preflights passed — hardware GL 4.6 and the Vulkan SDK were fine — so the runner itself is sound; only the toolchain was unreachable.
Adds a toolchain preflight that names every missing tool and prints the dnf line, so this reads as "provision the runner" rather than a bare command-not-found after a 39s checkout. Documents that build deps must be system-wide, and adds cmake/ninja-build/gcc/gcc-c++ to the package list.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation