Port UE5.7 task system to OloEngine - #93
Conversation
Complete port of Unreal Engine 5.7.1 low-level and high-level task system with all supporting infrastructure: ## Task System (LowLevelTasks namespace) - Scheduler: Work-stealing scheduler with per-thread local queues - LowLevelTask: Minimal task abstraction with priority and affinity - WaitingQueue: Parking lot pattern for efficient thread synchronization - LocalQueue: Lock-free MPSC queue for work stealing - TaskDelegate: Type-erased callable wrapper - TaskShared: Shared types (priorities, oversubscription callbacks) - Oversubscription: Dynamic thread scaling for blocking operations ## Task System (Tasks namespace - high-level API) - Task/TaskPrivate: FTaskBase with pipe, prereqs, completion events - ExtendedTaskPriority: Normal/blocking priority variants - Pipe: Serialized task execution - CancellationToken: Cooperative cancellation - ParallelFor: Parallel iteration patterns - LocalWorkQueue: Work-stealing parallel work pattern - SmallTaskAllocator: Pooled allocation for small tasks - InheritedContext: Task context propagation - NamedThreads: Game/RenderThread affinity (OloEngine standalone impl) ## Threading Primitives - FMutex, FRecursiveMutex, FWordMutex, FRecursiveWordMutex - FSharedMutex, FSharedRecursiveMutex (reader-writer) - TIntrusiveMutex, TExternalMutex (external state) - ParkingLot: Wait/wake infrastructure - TUniqueLock, TDynamicUniqueLock, TSharedLock, TDynamicSharedLock ## Supporting Infrastructure - TFunction, TFunctionRef, TUniqueFunction: Callable wrappers - TFunctionWithContext: Separated function+context - TRefCountPtr, TRefCountingMixin: Reference counting - Event/FEventRef: Manual/auto-reset events - FPlatformManualResetEvent: Lightweight OS event - WindowsHWrapper: Clean Windows.h include ## Changes from UE5.7 - Namespace: LowLevelTasks -> OloEngine::LowLevelTasks - Namespace: UE::Tasks -> OloEngine::Tasks - Types: SIZE_T->sizet, uint32->u32, int32->i32 - Members: m_ prefix convention - Excluded: AutoRTFM, NUMA (out of scope) - LocalWorkQueue: Changed std::function to TFunctionRef per review
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a large platform and concurrency expansion to OloEngine: an asynchronous/future framework, promise types and continuations, queued thread-pool and scheduler, low-level task scheduling, thread and runnable primitives, parking-lot wait/wake system, event/semaphore primitives and pooling, hazard-pointer and concurrent allocators, low-level memory tracker (LLM), numerous containers (deque, queue, linked lists, static arrays), task tagging and thread-manager, platform time/affinity/process utilities, and many auxiliary utilities. CMake and tooling files wire these sources into the build and add style/formatting/configuration updates. Many new public headers and symbols are introduced across Core, HAL, Task, Memory, Containers, Threading, and Async. Sequence Diagram(s)mermaid Caller->>AsyncLib: Async(Execution, Callable, CompletionCallback?) Note right of Promise: Future returned to Caller; continuations (Then/Next) run when Promise is set. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR completes the port of Unreal Engine 5.7.1's task system to OloEngine, providing a comprehensive work-stealing scheduler with supporting threading infrastructure. The implementation includes both low-level task primitives (scheduler, work-stealing queues, parking lot synchronization) and high-level APIs (parallel-for, task pipes, cancellation tokens), along with extensive threading support (mutexes, events, thread pools). Key additions include callable wrappers (TFunction/TFunctionRef), reference counting utilities, and a complete type layout system for memory image serialization.
Reviewed changes
Copilot reviewed 68 out of 117 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| NamedThreads.cpp | Thread-local storage for named thread dispatch (Game/Render/RHI thread affinity) |
| LowLevelTask.h | Core task primitive with state machine, priority, and cancellation support |
| LocalWorkQueue.h | Work-stealing parallel task pattern with Y-combinator for recursive lambdas |
| LocalQueue.h | Lock-free MPSC queue implementation for work stealing across thread-local queues |
| InheritedContext.h | Task context propagation for memory tagging and profiling integration |
| ExtendedTaskPriority.h | Extended priority system including inline execution and named thread priorities |
| CancellationToken.h | Cooperative cancellation with thread-local scope management |
| MemoryLayout.h | Complete type layout system for memory image serialization with field metadata |
| Archive.h | Removed duplicate memory image types (now in MemoryLayout.h) |
| LazySingleton.h | Lazy singleton pattern with explicit teardown support |
| Launder.h | std::launder wrapper for type-punning safety |
| Fork.h | Process forking support with forkable threads and multithread conversion |
| EnumClassFlags.h | Bitwise operators for enum class flags with helper functions |
| UnrealMemory.cpp | Implemented purgatory and poison proxy malloc wrappers |
| PlatformMallocCrash.h | Emergency crash allocator with pool-based allocation |
| PlatformMallocCrash.cpp | Crash allocator implementation with 14 fixed-size pools and bump allocator |
| PageAllocator.h | Added trim support and protected mode latching |
| MemoryOps.h | Fixed trait member access (Value → value) |
| WindowsEvent.h | Windows FEvent implementation using Win32 event handles |
| ThreadManager.h | Global thread registry with enumeration and stack trace support |
| Thread.h | High-level thread wrapper with RAII lifetime management |
| RunnableThread.h | Base thread class with TLS-based access and platform-specific implementations |
| Runnable.h | FRunnable interface for thread work with single-thread fallback |
| PlatformProcess.h | Platform-specific process/thread utilities (priority, affinity, naming) |
| PlatformMisc.h | Asymmetric fences, memory barriers, and processor group detection |
| ParkingLot.h | Global hash table of wait queues for efficient thread synchronization |
| LowLevelMemTrackerDefines.h | Compile-time configuration for Low-Level Memory tracker |
| MonotonicTime.cpp | Monotonic time implementation using platform time API |
| FAAArrayQueue.h | Changed std::this_thread::yield() to FPlatformProcess::YieldThread() |
| template <> struct THasTypeLayout<char> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<signed char> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<short> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<int> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<long> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<long long> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<unsigned char> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<unsigned short> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<unsigned int> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<unsigned long> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<unsigned long long> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<float> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<double> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<wchar_t> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<char16_t> { static constexpr bool Value = true; }; | ||
| template <> struct THasTypeLayout<void*> { static constexpr bool Value = true; }; |
There was a problem hiding this comment.
Inconsistent naming convention detected. The existing specializations use Value (uppercase), but the porting guide specifies lowercase member names (value). The changed line 71 in MemoryOps.h correctly uses value, but these THasTypeLayout specializations still use Value.
| */ | ||
| enum class ECancellationFlags : i8 | ||
| { | ||
| None = 0 << 0, |
There was a problem hiding this comment.
Enum value assignment uses 0 << 0 instead of just 0. While functionally identical, this is inconsistent with standard practice for the zero/none value in flag enums.
| */ | ||
| enum class ETaskFlags : i8 | ||
| { | ||
| AllowNothing = 0 << 0, |
There was a problem hiding this comment.
Enum value assignment uses 0 << 0 instead of just 0. While functionally identical, this is inconsistent with standard practice for the zero/none value in flag enums.
| * This class is used internally by CreateForkableThread and generally | ||
| * should not be instantiated directly. | ||
| */ | ||
| class FForkableThread : public FRunnableThread |
There was a problem hiding this comment.
The FForkableThread class implements process fork survival logic but lacks corresponding test coverage. Since the repo uses comprehensive testing for HAL components, this critical functionality should have tests verifying the fake-to-real thread conversion and fork behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 102
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
OloEngine/src/OloEngine/Containers/HazardPointer.h (1)
518-528: Critical bug: Move constructor releases the hazard slot it just acquired.The move constructor copies
Other.m_Recordintom_Record, then immediately callsm_Record->Release(). This releases the hazard protection that was just transferred, leaving the pointer unprotected and causing a use-after-free risk.THazardPointer(THazardPointer&& Other) : m_Hazard(Other.m_Hazard) , m_Record(Other.m_Record) { - if (m_Record) - { - m_Record->Release(); - } Other.m_Hazard = nullptr; Other.m_Record = nullptr; }The move constructor should simply transfer ownership without releasing. The moved-from object's destructor will be a no-op since its
m_Recordis nulled.OloEngine/src/OloEngine/Memory/MemoryOps.h (1)
350-358: Move operations incorrectly takeconstsource and cast away constness.
MoveConstructItemsandMoveAssignItems(lines 350, 388) acceptconst ElementType* Sourcebut then cast away constness viaconst_castto perform a move. This is semantically incorrect—move operations should take non-const source since they modify the source object.Apply this diff to fix the signature:
template <typename ElementType, typename SizeType> requires (sizeof(ElementType) > 0 && !std::is_trivially_copy_constructible_v<ElementType>) -OLO_NOINLINE void MoveConstructItems(void* Dest, const ElementType* Source, SizeType Count) +OLO_NOINLINE void MoveConstructItems(void* Dest, ElementType* Source, SizeType Count) { while (Count) { - ::new (static_cast<void*>(Dest)) ElementType(static_cast<ElementType&&>(*const_cast<ElementType*>(Source))); + ::new (static_cast<void*>(Dest)) ElementType(std::move(*Source)); ++(ElementType*&)Dest; ++Source; --Count; } }Similarly for
MoveAssignItemsat lines 386-397.
♻️ Duplicate comments (2)
OloEngine/src/OloEngine/Memory/PageAllocator.h (2)
158-164: Data race already flagged in LatchProtectedMode() review.The unsynchronized read of
m_bProtectedModeLatchedis part of the same data race issue identified in theLatchProtectedMode()method review above.
174-174: Change to std::atomic as flagged above.This member variable requires atomic access to prevent data races. See the fix proposed in the
LatchProtectedMode()review.
| "OloEngine/HAL/Event.cpp" | ||
| "OloEngine/HAL/Event.h" | ||
| "OloEngine/HAL/EventPool.h" | ||
| "OloEngine/HAL/ManualResetEvent.cpp" | ||
| "OloEngine/HAL/ManualResetEvent.h" | ||
| "OloEngine/HAL/ParkingLot.cpp" | ||
| "OloEngine/HAL/ParkingLot.h" | ||
| "OloEngine/HAL/Windows/WindowsEvent.h" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, locate the CMakeLists.txt file
fd -n "CMakeLists.txt" | head -20Repository: drsnuggles8/OloEngineBase
Length of output: 299
🏁 Script executed:
# Get the content at lines 246-253
fd -n "CMakeLists.txt" -x cat -n {} | sed -n '240,260p'Repository: drsnuggles8/OloEngineBase
Length of output: 299
🏁 Script executed:
# Search for platform-specific directories under HAL
find OloEngine -type d -name "Windows" -o -name "Linux" -o -name "Mac" -o -name "Unix" 2>/dev/null | grep -i halRepository: drsnuggles8/OloEngineBase
Length of output: 104
🏁 Script executed:
# Check CMake conditional patterns for platform-specific files
rg -n "if\(WIN32\)|if\(UNIX\)|if\(APPLE\)|elseif|else\(\)" OloEngine/src/CMakeLists.txt -A 3 -B 1 | head -50Repository: drsnuggles8/OloEngineBase
Length of output: 51
🏁 Script executed:
# Find and read the CMakeLists.txt file at the specified lines
find . -name "CMakeLists.txt" -path "*/src/*" | head -1 | xargs -I {} sed -n '240,260p' {}Repository: drsnuggles8/OloEngineBase
Length of output: 51
🏁 Script executed:
# Find all platform-specific directories under OloEngine/HAL
find OloEngine -type d | grep -E "(Windows|Linux|Mac|Unix)" | sortRepository: drsnuggles8/OloEngineBase
Length of output: 135
🏁 Script executed:
# Check how platform-specific files are included in CMakeLists.txt
rg -n "if\(WIN32\)|if\(UNIX\)|if\(APPLE\)|if\(CMAKE_SYSTEM" OloEngine/src/CMakeLists.txt -A 5 -B 1Repository: drsnuggles8/OloEngineBase
Length of output: 51
🏁 Script executed:
# Search for WindowsEvent references and how they're currently handled
rg -n "WindowsEvent" OloEngine/Repository: drsnuggles8/OloEngineBase
Length of output: 536
🏁 Script executed:
# Read Event.cpp around the WindowsEvent include (lines 10-30)
sed -n '10,30p' OloEngine/src/OloEngine/HAL/Event.cppRepository: drsnuggles8/OloEngineBase
Length of output: 545
🏁 Script executed:
# Check the full HAL directory structure to see what platform implementations exist
find OloEngine/src/OloEngine/HAL -type f -name "*.h" -o -name "*.cpp" | sortRepository: drsnuggles8/OloEngineBase
Length of output: 1011
🏁 Script executed:
# Check if WindowsEvent.h has preprocessor guards or conditional compilation
head -50 OloEngine/src/OloEngine/HAL/Windows/WindowsEvent.hRepository: drsnuggles8/OloEngineBase
Length of output: 1134
🏁 Script executed:
# Look at CMakeLists.txt around line 246-260 more carefully
sed -n '240,265p' OloEngine/src/CMakeLists.txtRepository: drsnuggles8/OloEngineBase
Length of output: 878
Conditional inclusion in CMakeLists.txt is a best-practice improvement, not a critical issue.
While WindowsEvent.h is listed unconditionally in CMakeLists.txt (line 253), the source code includes are already properly guarded with #ifdef OLO_PLATFORM_WINDOWS, preventing cross-platform build issues. For consistency and cleaner CMake practices, consider conditionally adding platform-specific files:
if(WIN32)
"OloEngine/HAL/Windows/WindowsEvent.h"
endif()🤖 Prompt for AI Agents
In OloEngine/src/CMakeLists.txt around lines 246 to 253, Windows-specific header
WindowsEvent.h is added unconditionally; update the CMake file to add that file
only when building on Windows (wrap the file entry in a WIN32 or similar
platform check) so platform-specific sources are included conditionally and
match the source-level #ifdef guards.
| ~TConsumeAllMpmcQueue() | ||
| { | ||
| static_assert(std::is_trivially_destructible_v<FNode>); | ||
| if (m_Head.load(std::memory_order_acquire) != nullptr) | ||
| { | ||
| ConsumeAllLifo([](T&&) {}); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the static_assert assumption.
The static_assert checks that FNode is trivially destructible, which assumes TTypeCompatibleBytes<T> is just aligned storage. This is correct because the contained T is manually destroyed via DestructItem. However, consider adding a comment explaining why this is valid.
~TConsumeAllMpmcQueue()
{
- static_assert(std::is_trivially_destructible_v<FNode>);
+ // FNode is trivially destructible because TTypeCompatibleBytes<T> is just aligned storage.
+ // The actual T object is manually destroyed in ConsumeAll via DestructItem.
+ static_assert(std::is_trivially_destructible_v<FNode>, "FNode must be trivially destructible");
if (m_Head.load(std::memory_order_acquire) != nullptr)
{
ConsumeAllLifo([](T&&) {});
}
}📝 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.
| ~TConsumeAllMpmcQueue() | |
| { | |
| static_assert(std::is_trivially_destructible_v<FNode>); | |
| if (m_Head.load(std::memory_order_acquire) != nullptr) | |
| { | |
| ConsumeAllLifo([](T&&) {}); | |
| } | |
| } | |
| ~TConsumeAllMpmcQueue() | |
| { | |
| // FNode is trivially destructible because TTypeCompatibleBytes<T> is just aligned storage. | |
| // The actual T object is manually destroyed in ConsumeAll via DestructItem. | |
| static_assert(std::is_trivially_destructible_v<FNode>, "FNode must be trivially destructible"); | |
| if (m_Head.load(std::memory_order_acquire) != nullptr) | |
| { | |
| ConsumeAllLifo([](T&&) {}); | |
| } | |
| } |
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Containers/ConsumeAllMpmcQueue.h around lines 44 to
51, the static_assert only checks that FNode is trivially destructible but lacks
an explanatory comment; add a short comment immediately above the static_assert
that states FNode is expected to be TTypeCompatibleBytes<T> (i.e. aligned
storage) and that the contained T is explicitly destroyed via DestructItem, so
the trivial destructor requirement is safe and intentional.
| if constexpr (bReverse) // Reverse the links to FIFO order if requested | ||
| { | ||
| FNode* Prev = nullptr; | ||
| while (Node) | ||
| { | ||
| FNode* Tmp = Node; | ||
| Node = Node->Next.exchange(Prev, std::memory_order_relaxed); | ||
| Prev = Tmp; | ||
| } | ||
| Node = Prev; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using non-atomic operations in the reversal loop.
After the list is detached from the queue via exchange, it's exclusively owned by the current thread. The atomic exchange on line 133 is unnecessary overhead; a simple load and store would suffice.
if constexpr (bReverse) // Reverse the links to FIFO order if requested
{
FNode* Prev = nullptr;
while (Node)
{
FNode* Tmp = Node;
- Node = Node->Next.exchange(Prev, std::memory_order_relaxed);
+ FNode* Next = Node->Next.load(std::memory_order_relaxed);
+ Node->Next.store(Prev, std::memory_order_relaxed);
+ Node = Next;
Prev = Tmp;
}
Node = Prev;
}🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Containers/ConsumeAllMpmcQueue.h around lines
127-137, the reversal loop uses an unnecessary atomic exchange on Node->Next
even though the list has been detached and is exclusively owned; replace the
atomic exchange with plain non-atomic pointer operations (load the Next pointer
into Tmp, assign Prev into the node's Next field using a plain pointer store) to
avoid atomic overhead, ensuring you access the underlying raw pointer/member
directly (or use .store on a non-atomic backing field removed of atomic
semantics) and keep the same loop logic (Tmp/Prev updates) and
memory_order_relaxed semantics are not needed for non-atomic stores.
| if (TlsData->ReclamationList.Num() >= Limit) | ||
| // Maybe scan the list - use time and count based triggers (matches UE5.7) | ||
| // Note: Using a simple time approximation since we don't have FApp::GetGameTime() | ||
| static thread_local double s_LastCollectionTime = 0.0; |
There was a problem hiding this comment.
Remove unused variable s_LastCollectionTime.
This static thread_local variable is declared but never used. The code correctly uses TlsData->TimeOfLastCollection instead.
- static thread_local double s_LastCollectionTime = 0.0;📝 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.
| static thread_local double s_LastCollectionTime = 0.0; |
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Containers/HazardPointer.h around line 396, remove
the unused static thread_local variable declaration `s_LastCollectionTime` since
the code uses `TlsData->TimeOfLastCollection` instead; delete that line and run
a build to ensure no remaining references depend on it.
| void Tick() | ||
| { | ||
| if (m_bIsRealThread || !m_Runnable) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // In fake thread mode, we execute on the main thread | ||
| // The runnable should be designed to handle being ticked | ||
| // rather than running in a loop | ||
| } |
There was a problem hiding this comment.
Incomplete Tick() implementation - runnable is not actually ticked.
The Tick() method has an empty body after the early return check. In fake thread mode, it should delegate to the runnable's single-thread interface to actually perform work. Without this, forkable threads won't execute any work before fork.
void Tick()
{
if (m_bIsRealThread || !m_Runnable)
{
return;
}
- // In fake thread mode, we execute on the main thread
- // The runnable should be designed to handle being ticked
- // rather than running in a loop
+ // In fake thread mode, delegate to the single-thread interface
+ if (FSingleThreadRunnable* SingleThreadRunnable = m_Runnable->GetSingleThreadInterface())
+ {
+ SingleThreadRunnable->Tick();
+ }
}🤖 Prompt for AI Agents
OloEngine/src/OloEngine/Misc/Fork.h lines 273-283: the Tick() method returns
early in fake-thread mode but never invokes the runnable, so no work runs;
modify Tick() to call the runnable's single-thread tick method (the runnable's
Tick/RunSingleIteration/similar single-iteration API) when m_bIsRealThread is
false and m_Runnable is set, respect the runnable's return/finished signal to
stop or continue ticking, and ensure appropriate lifecycle handling (e.g., check
m_Runnable each iteration and handle errors or completion) so fake-thread mode
actually executes work before fork.
| { | ||
| // Register with thread manager so it gets converted on fork | ||
| return ForkableThread; |
There was a problem hiding this comment.
Missing thread manager registration.
The comment indicates the forkable thread should be registered for conversion on fork, but no registration actually occurs. Without this, OnForkingOccured() won't be able to convert forkable threads to real threads.
Would you like me to help design a thread registry mechanism, or is this intentionally deferred? A typical implementation would involve a static list/set of forkable threads that OnForkingOccured() iterates to call ConvertToRealThread() on each.
| alignas(T) unsigned char Data[sizeof(T)]; | ||
| T* Ptr; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Member variable naming convention.
Per the coding guidelines, member variables should use m_PascalCase naming. The current Data and Ptr should be m_Data and m_Ptr.
Apply this diff to align with the naming convention:
- alignas(T) unsigned char Data[sizeof(T)];
- T* Ptr;
+ alignas(T) unsigned char m_Data[sizeof(T)];
+ T* m_Ptr;You'll also need to update references throughout the class (lines 117, 120, 132, 137, 143, 145, 146).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Misc/LazySingleton.h around lines 110-111, rename the
member variables Data and Ptr to m_Data and m_Ptr to follow the m_PascalCase
convention and update all uses in this class (occurrences at lines 117, 120,
132, 137, 143, 145, 146) to reference m_Data and m_Ptr respectively; ensure
alignas and sizeof remain unchanged, update any pointer casts or initializations
to use m_Ptr, and rebuild to verify no remaining references to the old names.
| TLazySingleton(void(*Constructor)(void*)) | ||
| { | ||
| if (Constructor) | ||
| { | ||
| Constructor(Data); | ||
| } | ||
|
|
||
| Ptr = Constructor ? reinterpret_cast<T*>(Data) : nullptr; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider std::launder for strict aliasing compliance.
While the reinterpret_cast on line 120 works due to proper alignment, C++17's std::launder should be used after placement new to ensure strict aliasing rules are satisfied. This tells the compiler that a new object lifetime has begun at that address.
If targeting C++17 or later, apply this diff:
+#include <new>
+
TLazySingleton(void(*Constructor)(void*))
{
if (Constructor)
{
Constructor(Data);
+ Ptr = std::launder(reinterpret_cast<T*>(Data));
+ }
+ else
+ {
+ Ptr = nullptr;
}
-
- Ptr = Constructor ? reinterpret_cast<T*>(Data) : nullptr;
}Committable suggestion skipped: line range outside the PR's diff.
| #define REGISTER_INLINE_TYPE_LAYOUT(T) \ | ||
| static struct ANONYMOUS_VARIABLE(RegisterTypeLayout) { \ | ||
| ANONYMOUS_VARIABLE(RegisterTypeLayout)() { \ | ||
| T::StaticGetTypeLayout().Name = TEXT(#T); \ | ||
| OloEngine::FTypeLayoutDesc::Register(T::StaticGetTypeLayout()); \ | ||
| } \ | ||
| } ANONYMOUS_VARIABLE(RegisterTypeLayoutInstance) |
There was a problem hiding this comment.
Type mismatch bug in REGISTER_INLINE_TYPE_LAYOUT macro.
Line 1081 assigns TEXT(#T) to Name, but:
TEXT(x)expands toL##x(wide string literal,const wchar_t*)FTypeLayoutDesc::Nameisconst char*(line 155)
This will cause a compilation error when the macro is used.
Apply this diff to fix the type mismatch:
#define REGISTER_INLINE_TYPE_LAYOUT(T) \
static struct ANONYMOUS_VARIABLE(RegisterTypeLayout) { \
ANONYMOUS_VARIABLE(RegisterTypeLayout)() { \
- T::StaticGetTypeLayout().Name = TEXT(#T); \
+ T::StaticGetTypeLayout().Name = #T; \
OloEngine::FTypeLayoutDesc::Register(T::StaticGetTypeLayout()); \
} \
} ANONYMOUS_VARIABLE(RegisterTypeLayoutInstance)🧰 Tools
🪛 Clang (14.0.6)
[warning] 1081-1081: macro argument should be enclosed in parentheses
(bugprone-macro-parentheses)
[warning] 1082-1082: macro argument should be enclosed in parentheses
(bugprone-macro-parentheses)
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Serialization/MemoryLayout.h around lines 1078-1084,
the REGISTER_INLINE_TYPE_LAYOUT macro assigns TEXT(#T) (a wide string) to
FTypeLayoutDesc::Name which is const char*, causing a type mismatch; change the
assignment to use a narrow string literal by replacing TEXT(#T) with #T (i.e.,
T::StaticGetTypeLayout().Name = #T;) so Name receives a const char* and the
macro compiles correctly.
|
- Created FunctionWithContextTest.cpp to test TFunctionWithContext functionality including default construction, lambda construction, function pointer construction, and usage in a parking lot style API. - Created TypeTraitsTest.cpp to validate various type traits such as TFormatSpecifier, TNameOf, TNthTypeFromParameterPack, TIsFundamentalType, TIsFunction, TCallTraits, and logical combinators. - Added atomic_queue as a dependency in CMakeLists.txt and created an interface target for easy inclusion.
- Updated comments - Updated README.md to include instructions for code formatting and pre-commit hooks. - Fixed minor formatting issues in various files to ensure consistent style. - Ensured all modified files end with a newline where applicable
There was a problem hiding this comment.
Actionable comments posted: 39
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
OloEngine/src/OloEngine/Core/Reflection/TypeUtils.h (1)
1-10: Remove unused<iostream>include.The header at line 5 (
#include <iostream>) is not used anywhere in this file. Consider removing it to reduce compilation overhead.🔎 Proposed fix
#pragma once #include "OloEnginePCH.h" #include <array> - #include <iostream> #include <tuple> #include <type_traits> #include <vector> #include <utility>OloEngine/src/OloEngine/Audio/SoundGraph/NodeProcessor.h (1)
261-261: Consider migrating threading primitives to OloEngine's mutex/lock types for consistency.While the current use of
std::shared_mutexwithstd::shared_lockandstd::unique_lockis correct, the PR introduces OloEngine-native threading primitives (FSharedMutex,TSharedLock,TDynamicSharedLock,TUniqueLock). For long-term consistency across the codebase, consider adopting these when they become widely available in public headers.This is not blocking for the audio module (which is outside the task-system scope), but would align with the broader codebase direction established in this PR.
Also applies to: 270-270, 279-279
OloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphCache.h (2)
1-19: Add missing#include <list>for LRU tracking.Line 123 declares
std::list<std::string> m_LRUOrder;but<list>is not explicitly included. Although it may be transitively included through other headers, this violates the "include what you use" principle and makes the code fragile.🔎 Proposed fix
#include <string> #include <unordered_map> #include <unordered_set> #include <atomic> #include <chrono> #include <condition_variable> #include <deque> #include <functional> #include <queue> #include <mutex> #include <vector> +#include <list> #include <optional> #include <utility>
105-106: Consider using OloEngine's callable type wrapper instead ofstd::function.The
LoadCallbacktypedef usesstd::function, which incurs heap allocation and virtual indirection. Per the task system port in this PR, OloEngine favors lightweight callable wrappers (e.g.,TFunctionRef) overstd::functionwhere appropriate. For a public API method that accepts a callback, a reference-based callable might be more aligned with OloEngine's design patterns.Please verify whether OloEngine has a
TFunctionRefor similar lightweight function wrapper available viaCore/Base.hor other headers, and whether it's appropriate for this public-facing callback parameter.OloEngine/src/OloEngine/Asset/PlaceholderAsset.cpp (1)
99-100: Remove or implement the dead/commented-out code.Lines 99-100 contain commented code with a note indicating uncertainty about the API contract. Either implement the functionality properly with appropriate error handling or remove the commented lines entirely. Dead code creates maintenance burden and signals incomplete work.
🔎 Proposed fix to remove dead code
auto placeholderTexture = PlaceholderAssetManager::GetPlaceholderAsset(AssetType::Texture2D); if (auto texPlaceholder = placeholderTexture.As<PlaceholderTexture>()) { - // Note: SetAlbedoMap may need AssetHandle instead of Ref<Texture2D> - // m_Material->SetAlbedoMap(texPlaceholder->GetTexture()); }Alternatively, if the functionality is needed, complete the implementation with proper documentation of API requirements.
OloEngine/src/OloEngine/Audio/AudioThread.h (2)
20-20: Consider using ported function types for consistency with the new task system.This PR introduces
TFunction,TFunctionRef,TUniqueFunction, andTFunctionWithContexttypes. Currently,AudioThreadusesstd::function<void()>. Depending on design intent, consider aligning with the ported primitives for consistency across the codebase.If alignment is desired, the typedef could be:
using Task = TUniqueFunction<void()>; // or TFunction<void()> if copyable storage is preferredVerify whether the new ported function types are intended as the canonical choice for OloEngine or if
std::functionremains acceptable for components like AudioThread.Also applies to: 29-29, 57-57
77-78: Consider lock-free queue for potential performance improvement.The comment indicates awareness that a lock-free queue would be preferable. This PR introduces a lock-free MPSC
LocalQueueas part of the work-stealing Scheduler. Depending on performance requirements, AudioThread could potentially leverage this infrastructure for the task queue instead ofstd::queue+std::mutex.This is not urgent but worth revisiting if audio thread latency becomes a concern.
Would you like me to help integrate the ported
LocalQueueor other lock-free primitives from the new task system intoAudioThread?OloEngine/src/OloEngine/Debug/ALLOCATION_TRACKING_GUIDE.md (1)
1-233: Fix guide examples: Entity and AnimationClip classes do not use allocation tracking.The allocation tracking macros and system are properly implemented, but the guide's example code is misleading. Entity and AnimationClip classes do not inherit from
AllocationTracker, so they lack the methods documented in examples:
Entity::GetLiveCount()Entity::GetPeakCount()Entity::GetStatsString()AnimationClip::GetLiveCount()These method calls will fail to compile. Replace example classes with ones that actually implement
OLO_ALLOCATION_TRACKED(ClassName)orOLO_TRACKED_REFCOUNTED(ClassName), or create minimal example classes for demonstration purposes.OloEngine/src/OloEngine/Asset/AssetPackBuilder.cpp (1)
72-73: Consider usingf32instead offloatfor type consistency.Local progress variables use
floatwhile the rest of the codebase employsf32. Whilefloatis functionally correct, standardizing onf32aligns with the OloEngine type convention established elsewhere in the file (e.g., line 394).🔎 Proposed consistency updates
- float loadProgress = 0.0f; - float progressPerAsset = 0.3f / static_cast<float>(allAssets.size()); + f32 loadProgress = 0.0f; + f32 progressPerAsset = 0.3f / static_cast<f32>(allAssets.size());And in the lambda (line 146):
- float internal = internalProgress.load(); + f32 internal = internalProgress.load();Also applies to: 146-146
OloEditor/src/Panels/AssetPackBuilderPanel.cpp (4)
242-246: Critical bug in progress percentage calculation.The progress bar calculation is incorrect.
permilleProgressis in the range 0–1000 (per line 331), but the code divides by 10 instead of 1000. This produces values 0–100 instead of the 0.0–1.0 range required byImGui::ProgressBar. The subsequent multiplication by 100 at line 246 compounds this error, displaying values like 10000% instead of 100%.🔎 Proposed fix for progress calculation
i32 permilleProgress = m_BuildProgressPermille.load(); - f32 progress = static_cast<f32>(permilleProgress) / 10.0f; + f32 progress = static_cast<f32>(permilleProgress) / 1000.0f; ImGui::Text("Building asset pack..."); ImGui::ProgressBar(progress, ImVec2(-1.0f, 0.0f), nullptr);
19-26: Move closing brace to a new line per coding guidelines.Line 26 places the closing brace on the same line as the assignment statement. Per coding guidelines, braces should be placed on new lines except for trivial cases. Buffer initialization is not a trivial case.
🔎 Proposed formatting fix
// Initialize output path buffer with default value const char* defaultPath = "Assets/AssetPack.olopack"; sizet len = std::strlen(defaultPath); sizet copyLen = std::min(len, m_OutputPathBuffer.size() - 1); std::memcpy(m_OutputPathBuffer.data(), defaultPath, copyLen); - m_OutputPathBuffer[copyLen] = '\0'; } + m_OutputPathBuffer[copyLen] = '\0'; + }
38-47: Move closing brace to a new line per coding guidelines.Line 46's closing brace should be on a separate line, consistent with the coding standard of placing braces on new lines except for trivial cases.
🔎 Proposed formatting fix
void AssetPackBuilderPanel::SyncUIFromSettings() { // Synchronize output path buffer from settings std::string pathStr = m_BuildSettings.m_OutputPath.string(); // Safely copy to buffer with bounds checking sizet copyLength = std::min(pathStr.length(), m_OutputPathBuffer.size() - 1); std::memcpy(m_OutputPathBuffer.data(), pathStr.c_str(), copyLength); - m_OutputPathBuffer[copyLength] = '\0'; // Ensure null termination + m_OutputPathBuffer[copyLength] = '\0'; // Ensure null termination + }
320-356: Unused lambda parameters should be removed or properly named.Lines 328 and 338 declare lambda parameters
(std::stop_token)without using them. The lambdas capturestopTokenfrom the outer scope instead. Remove the unused parameters or bind them with meaningful names to clarify intent.🔎 Proposed refactor
Remove the unused parameter:
- std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token) { + std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token) { while (!stopToken.stop_requested()) {If the parameter is intentional for the jthread stop_token interface, rename it for clarity:
- std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token) { + std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token stoken) { while (!stopToken.stop_requested()) {Same fix applies to line 338:
- std::jthread cancellationMonitor([&cancelRequested, stopToken](std::stop_token) { + std::jthread cancellationMonitor([&cancelRequested, stopToken](std::stop_token stoken) {.github/workflows/Windows.yml (1)
49-49: CTest is not configured to output XML results; artifact upload will not capture test results.The workflow runs tests from
build/OloEngine/tests(line 49) withctest -V, but this command doesn't generate XML output. The artifact upload (line 58) expects XML files at${{runner.workspace}}/OloEngineBase/test_results/*.xml, which will never be created.To fix this:
- Add
--gtest_output=xmlflag to the ctest command to generate JUnit XML reports, or- Configure GoogleTest XML output in the CMakeLists.txt test definition, then adjust the artifact path to match the actual output location
Without this, test result artifacts are silently skipped (due to
if-no-files-found: warn), breaking test result reporting in CI.OloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphCache.cpp (3)
62-80: Add profiling macro for observability consistency.The
Get()method is missing theOLO_PROFILE_FUNCTION()macro that is consistently applied to other accessor methods (Has(),GetHitRatio(), etc.). This creates an inconsistency in performance monitoring coverage.🔎 Proposed fix
Ref<SoundGraph> SoundGraphCache::Get(const std::string& sourcePath) { + OLO_PROFILE_FUNCTION(); std::lock_guard<std::mutex> lock(m_Mutex);
504-509: Improve audio data memory estimation logic.The conservative 2MB per-node estimate for potential audio data (line 509) lacks justification and could significantly overestimate actual memory usage, leading to unnecessary cache evictions. The TODO comment indicates this estimation is incomplete.
Consider: (1) implementing actual audio data size inspection via WavePlayer's public API, (2) making the estimate configurable, or (3) documenting the rationale for the 2MB figure. If inspection is infeasible, add a helper method that interrogates the audio data structure directly rather than using a blanket estimate.
Would you like me to help refactor this section to calculate actual audio memory usage instead of using conservative estimates?
186-187: Use.load()when reading atomic counters.Lines 186 and 424 attempt to add
m_HitCountandm_MissCountdirectly without calling.load(). Since these are declared asstd::atomic<u64>(header lines 134-135), the operator+ is not overloaded for atomic types and will fail to compile. Both locations require:u64 totalAccesses = m_HitCount.load() + m_MissCount.load();Similarly, lines 187 and 425 must cast the loaded value:
static_cast<f32>(m_HitCount.load()) / static_cast<f32>(totalAccesses)Line 427 correctly uses
.load()in the logging statement, confirming these are atomic types.Also applies to: 424-425
OloEngine/src/OloEngine/Audio/SoundGraph/SoundGraph.h (2)
108-108: Consider usingRef<NodeProcessor>instead of raw pointers form_WavePlayers.Per coding guidelines, prefer
Ref<T>smart pointers over raw pointers. While the comment indicates these point intom_Nodes(and thus lifetimes are coupled), usingRef<T>would provide safer memory management and explicit ownership semantics.🔎 Proposed refactor
- /// Wave players for audio file playback (subset of nodes) - std::vector<NodeProcessor*> m_WavePlayers; // Raw pointers to nodes in m_Nodes vector + /// Wave players for audio file playback (subset of nodes) + std::vector<Ref<NodeProcessor>> m_WavePlayers; // Refs to nodes in m_Nodes vector
125-125: Consider usingRef<StreamWriter>for safer lifetime management ofm_Endpoint.The
InterpolatedValuestruct holds a raw pointer to aStreamWriter. Per coding guidelines, preferRef<T>smart pointers for clearer ownership and safer memory management.OloEngine/src/OloEngine/Containers/BitArray.h (1)
1616-1677: Fix allocator type inWriteMemoryImagefallback pathIn
TBitArray::WriteMemoryImage:if constexpr (TAllocatorTraits<Allocator>::SupportsFreezeMemoryImage) { ... } else { Writer.WriteBytes(TBitArray()); }Inside the
template<typename Allocator>method,TBitArray()here instantiatesTBitArray<>with the default allocator, not the currentAllocatortemplate argument. ForTBitArrayusing a custom allocator this will write the wrong type/layout into the memory image.Change it to use the current allocator:
- else - { - Writer.WriteBytes(TBitArray()); - } + else + { + // Fallback: write a default-initialized instance of this exact TBitArray<Allocator> type + Writer.WriteBytes(TBitArray<Allocator>()); + }This keeps the frozen representation consistent with the actual
TBitArray<Allocator>instantiation.
♻️ Duplicate comments (9)
OloEngine/src/OloEngine/Containers/ConsumeAllMpmcQueue.h (2)
44-51: The static_assert documentation was already addressed in a previous review.
127-137: The unnecessary atomic exchange in the reversal loop was already addressed in a previous review.OloEngine/src/CMakeLists.txt (2)
273-273: Platform-specific file added unconditionally (previously flagged).WindowsEvent.h is Windows-specific but added to the build unconditionally. While the header has
#ifdef OLO_PLATFORM_WINDOWSguards, CMake best practices recommend conditional inclusion:if(WIN32) "OloEngine/HAL/Windows/WindowsEvent.h" endif()This was previously flagged in past reviews. Based on learnings, as per coding guidelines...
444-465: Comprehensive task system additions, but files still missing (previously flagged).The 22 task system files listed provide the core LowLevelTasks and high-level Tasks API as described in the PR objectives. However, the previous review identified that
Task.cppandLocalWorkQueue.hare missing from this list:
OloEngine/Task/Task.cpp— Contains FTaskPriorityCVar implementation requiring compilationOloEngine/Task/LocalWorkQueue.h— Header-only template utility for work-stealing patternsThese files should be added to ensure complete build coverage.
OloEngine/src/OloEngine/Containers/HazardPointer.h (1)
396-396: Remove unused variables_LastCollectionTime(previously flagged).This static thread_local variable is declared but never used. The code correctly uses
TlsData->TimeOfLastCollectionat lines 399 and 404 instead.🔎 Proposed fix
- static thread_local double s_LastCollectionTime = 0.0;OloEngine/src/OloEngine/Debug/TaskTrace.h (1)
186-189: Remove unusedm_pZonemember.This was flagged in a previous review. The implementation (lines 352-353) explicitly states that
ScopedZonecannot be stored as a member and uses message markers instead. This unused member should be removed.OloEngine/src/OloEngine/Core/PlatformTime.h (2)
52-57: Unsupported platforms silently return 0.This issue was previously flagged. Returning 0 for unsupported architectures could cause subtle bugs in callers expecting valid cycle counts.
63-80: Hardcoded 3 GHz frequency produces incorrect time conversions.This issue was previously flagged. The fixed 3 GHz assumption is incorrect across different hardware.
OloEngine/src/OloEngine/Experimental/ConcurrentLinearAllocator.h (1)
243-252: Add a static_assert guarding the padding size aroundNumAllocationsThe padding field:
using NumAllocationsType = std::conditional_t<...>; NumAllocationsType NumAllocations{ .Value = UINT_MAX }; u8 Padding[OLO_PLATFORM_CACHE_LINE_SIZE - sizeof(std::atomic_uint)];assumes
OLO_PLATFORM_CACHE_LINE_SIZE >= sizeof(std::atomic_uint). If that’s ever not true for a platform, this will produce a negative (or zero) array bound with a very cryptic error.Add an explicit check:
+ static_assert(OLO_PLATFORM_CACHE_LINE_SIZE >= sizeof(std::atomic_uint), + "OLO_PLATFORM_CACHE_LINE_SIZE must be >= sizeof(std::atomic_uint)"); u8 Padding[OLO_PLATFORM_CACHE_LINE_SIZE - sizeof(std::atomic_uint)]; // Avoid false sharingso configuration issues fail with a clear diagnostic instead of an obscure compile error.
| BasedOnStyle: LLVM | ||
| IndentWidth: 4 | ||
| UseTab: Never | ||
| ColumnLimit: 0 |
There was a problem hiding this comment.
Set a reasonable ColumnLimit instead of disabling it.
A ColumnLimit: 0 disables line-length enforcement entirely, which can degrade code readability and create challenges for code review and terminal-based workflows. Industry-standard C++ projects typically enforce limits of 80–120 characters to maintain visual clarity.
🔎 Recommended fix for ColumnLimit
-ColumnLimit: 0
+ColumnLimit: 120Alternatively, use ColumnLimit: 100 or ColumnLimit: 80 depending on your team's preferences.
📝 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.
| ColumnLimit: 0 | |
| ColumnLimit: 120 |
🤖 Prompt for AI Agents
In .clang-format around line 4, ColumnLimit is set to 0 which disables
line-length enforcement; change it to a reasonable limit (e.g., 100 or 80) to
restore column wrapping and improve readability. Update the ColumnLimit value to
your chosen integer, run clang-format on the codebase to reflow long lines, and
commit the updated .clang-format so future formatting enforces the selected
maximum line width.
| on: | ||
| push: | ||
| branches: [ "**" ] | ||
| pull_request: | ||
| branches: [ "**" ] |
There was a problem hiding this comment.
Fix YAML formatting for branch patterns.
Remove extra spaces inside brackets to comply with YAML style guidelines.
🔎 Proposed fix
on:
push:
- branches: [ "**" ]
+ branches: ["**"]
pull_request:
- branches: [ "**" ]
+ branches: ["**"]📝 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.
| on: | |
| push: | |
| branches: [ "**" ] | |
| pull_request: | |
| branches: [ "**" ] | |
| on: | |
| push: | |
| branches: ["**"] | |
| pull_request: | |
| branches: ["**"] |
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 7-7: too many spaces inside brackets
(brackets)
[error] 7-7: too many spaces inside brackets
(brackets)
🤖 Prompt for AI Agents
.github/workflows/pre-commit.yml around lines 3 to 7: the branch pattern arrays
contain extra spaces inside the square brackets (branches: [ "**" ]) which is
poor YAML style; replace both occurrences with compact array syntax without
internal spaces (branches: ["**"]) so the arrays are formatted as branches:
["**"] for both push and pull_request.
| - name: Setup Python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '3.x' |
There was a problem hiding this comment.
Update to actions/setup-python@v5.
The v4 action is deprecated and incompatible with current GitHub Actions runners.
🔎 Proposed fix
- name: Setup Python
- uses: actions/setup-python@v4
+ uses: actions/setup-python@v5
with:
python-version: '3.x'📝 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.
| - name: Setup Python | |
| uses: actions/setup-python@v4 | |
| with: | |
| python-version: '3.x' | |
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: '3.x' |
🧰 Tools
🪛 actionlint (1.7.9)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/pre-commit.yml around lines 16 to 19, the workflow uses the
deprecated actions/setup-python@v4; update the step to use
actions/setup-python@v5 instead. Edit the file to change the action reference
from actions/setup-python@v4 to actions/setup-python@v5, keep the existing with:
python-version: '3.x' (or switch to the newer 'python-version' CLI format if
your runner requires it), and commit the updated workflow so the runner uses the
supported v5 action.
| - name: Install pre-commit | ||
| run: python -m pip install --upgrade pip pre-commit | ||
|
|
||
| - name: Run pre-commit | ||
| run: pre-commit run --all-files |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching pre-commit environments for faster CI runs.
Caching pre-commit's virtual environments can significantly speed up workflow execution on subsequent runs.
🔎 Suggested enhancement
- name: Install pre-commit
run: python -m pip install --upgrade pip pre-commit
+ - name: Cache pre-commit
+ uses: actions/cache@v4
+ with:
+ path: ~/.cache/pre-commit
+ key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }}
+
- name: Run pre-commit
run: pre-commit run --all-files📝 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.
| - name: Install pre-commit | |
| run: python -m pip install --upgrade pip pre-commit | |
| - name: Run pre-commit | |
| run: pre-commit run --all-files | |
| - name: Install pre-commit | |
| run: python -m pip install --upgrade pip pre-commit | |
| - name: Cache pre-commit | |
| uses: actions/cache@v4 | |
| with: | |
| path: ~/.cache/pre-commit | |
| key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }} | |
| - name: Run pre-commit | |
| run: pre-commit run --all-files |
🤖 Prompt for AI Agents
.github/workflows/pre-commit.yml lines 21-25: the workflow repeatedly reinstalls
pre-commit and rebuilds its virtualenvs which slows CI; add a cache step to
persist pre-commit's environments between runs. Before installing pre-commit,
restore a cache keyed by the OS and a checksum of .pre-commit-config.yaml (and
optionally pre-commit version), set PRE_COMMIT_HOME to a cached directory (e.g.,
~/.cache/pre-commit), and after installation save that directory back to the
cache so the virtualenvs are reused; then run pre-commit as before. Ensure the
cache key includes changes to pre-commit config so environments rebuild when
hooks change.
| # Skip vendor and common IDE/build metadata | ||
| # pre-commit paths are matched against repository-relative file paths; the regex below | ||
| # excludes common third-party / generated directories we don't want to modify. | ||
| exclude: '^(vendor/|\.vs/|\.vscode/|build/|bin/|\.github/workflows/|\.vs/|\.idea/)' |
There was a problem hiding this comment.
Remove duplicate .vs/ entry in exclude pattern.
The exclude pattern contains .vs/ twice, which is redundant.
🔎 Proposed fix
-exclude: '^(vendor/|\.vs/|\.vscode/|build/|bin/|\.github/workflows/|\.vs/|\.idea/)'
+exclude: '^(vendor/|\.vs/|\.vscode/|build/|bin/|\.github/workflows/|\.idea/)'📝 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.
| exclude: '^(vendor/|\.vs/|\.vscode/|build/|bin/|\.github/workflows/|\.vs/|\.idea/)' | |
| exclude: '^(vendor/|\.vs/|\.vscode/|build/|bin/|\.github/workflows/|\.idea/)' |
🤖 Prompt for AI Agents
In .pre-commit-config.yaml around line 4, the exclude regex contains a
duplicated ".vs/" entry which is redundant; remove the duplicate so each path
appears only once (e.g., keep a single ".vs/" segment in the pattern) and ensure
the resulting regex still matches the intended directories.
| enum class ECsvCustomStatOp | ||
| { | ||
| Set, | ||
| Accumulate, | ||
| Min, | ||
| Max, | ||
| }; | ||
|
|
||
| #if OLO_PROFILE && TRACY_ENABLE | ||
| // Tracy plots can be used to track numeric values over time | ||
| #define CSV_CUSTOM_STAT(Category, Stat, Value, Op) \ | ||
| do { TracyPlot(#Category "_" #Stat, static_cast<double>(Value)); } while(0) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ECsvCustomStatOp enum is unused in macro implementation.
The Op parameter is captured in the macro but ignored — TracyPlot always overwrites the value (equivalent to Set). This is acceptable for basic compatibility, but consider either:
- Documenting that only
Setsemantics are supported - Or removing the unused enum if full CSV stat semantics aren't needed
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Debug/TaskTrace.h around lines 485-496, the
ECsvCustomStatOp enum and the Op parameter in the CSV_CUSTOM_STAT macro are
unused (the macro always calls TracyPlot which behaves like Set). Either remove
the unused enum and the Op parameter from the macro signature to avoid dead
code, or keep the enum/parameter but add a clear comment above the macro
documenting that only Set semantics are supported and that Op is intentionally
ignored for TracyPlot compatibility; pick one approach and update any callers or
header documentation accordingly.
| void FEvent::AdvanceStats() | ||
| { | ||
| // Stats tracking would be implemented here | ||
| // For now, this is a placeholder | ||
| } | ||
|
|
||
| void FEvent::WaitForStats() | ||
| { | ||
| // Record that a wait has started | ||
| m_EventStartCycles.fetch_add(1, std::memory_order_relaxed); | ||
| } | ||
|
|
||
| void FEvent::TriggerForStats() | ||
| { | ||
| // Record that the event was triggered | ||
| // In UE5.7 this sends stats messages - we just track the cycle | ||
| } | ||
|
|
||
| void FEvent::ResetForStats() | ||
| { | ||
| // Reset the cycle counter | ||
| m_EventStartCycles.store(0, std::memory_order_relaxed); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stats tracking placeholders are incomplete.
The stats methods (AdvanceStats, TriggerForStats, ResetForStats) are minimal placeholders. WaitForStats() increments m_EventStartCycles, but the other methods don't utilize it meaningfully.
Consider whether full stats tracking is needed for OloEngine, or document that these are intentional stubs for future implementation.
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/HAL/Event.cpp around lines 24 to 46, the stats
methods are only placeholders and do not meaningfully use m_EventStartCycles;
either implement basic stats tracking or explicitly document them as intentional
stubs. Fix by one of two options: (A) implement minimal tracking — in
WaitForStats record a start timestamp/cycle count, in TriggerForStats read
current cycles, compute and accumulate duration counters (e.g., total wait
cycles, max/min, and increment trigger count) with atomic updates, in
AdvanceStats roll per-frame/sample counters into aggregates and clear per-event
accumulators, and ResetForStats zero all counters atomically; or (B) if you do
not want runtime tracking now, add a clear comment/TODO stating these are
intentional no-ops, remove misleading placeholder comments, and optionally
annotate with [[maybe_unused]] or compile-time macro to avoid unused-variable
warnings so reviewers know this is deliberate. Ensure thread-safety with
std::atomic and memory_order_relaxed where appropriate and keep changes limited
to these methods and any new member counters needed.
| #ifdef OLO_PLATFORM_WINDOWS | ||
| Event = new FEventWin(); | ||
| #else | ||
| static_assert(false, "Platform-specific event implementation required"); |
There was a problem hiding this comment.
Fix the static_assert to prevent compilation failure on non-Windows platforms.
The current static_assert(false, ...) will unconditionally fail at compile time, even when the code is never instantiated. This prevents compilation on all platforms.
🔎 Proposed fix
Replace with a template-dependent false or use #error:
#ifdef OLO_PLATFORM_WINDOWS
Event = new FEventWin();
#else
- static_assert(false, "Platform-specific event implementation required");
+ #error "Platform-specific event implementation required"
#endif🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/HAL/Event.cpp around line 56, the current
unconditional static_assert(false, "...") will always fail and break builds on
non-Windows platforms; replace it with a template-dependent false or conditional
compile guard so the assertion only triggers when instantiated or on unsupported
platforms. Implement a helper such as a templated constexpr always_false<T> and
use static_assert(always_false<YourType>::value, "Platform-specific event
implementation required") (or alternatively wrap the file in #error/#ifdef
checks for the target platform) so compilation only fails when the
platform-specific implementation is actually required.
| FEventRef::~FEventRef() | ||
| { | ||
| if (m_Event != nullptr) | ||
| { | ||
|
|
||
| // Try to access the vtable to see if the object is valid | ||
| bool isManualReset = false; | ||
| try { | ||
| isManualReset = m_Event->IsManualReset(); | ||
| } catch (...) { | ||
| return; | ||
| } | ||
|
|
||
| if (isManualReset) | ||
| { | ||
| auto& pool = TEventPool<EEventMode::ManualReset>::Get(); | ||
| pool.ReturnToPool(m_Event); | ||
| } | ||
| else | ||
| { | ||
| TEventPool<EEventMode::AutoReset>::Get().ReturnToPool(m_Event); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe exception handling risks resource leaks.
The destructor attempts to detect invalid objects by catching all exceptions during IsManualReset() and silently returning without releasing the event. This approach has several problems:
- Catching all exceptions (
catch (...)) and silently returning can leak the event back to the pool - Relying on exception-based vtable validity checks is fragile and non-portable
- If
m_Eventpoints to an invalid object, the program state is already corrupted
If this check is truly necessary, the underlying lifetime management issue should be addressed instead.
🔎 Recommended approach
Remove the try-catch and rely on proper lifetime guarantees:
FEventRef::~FEventRef()
{
if (m_Event != nullptr)
{
-
- // Try to access the vtable to see if the object is valid
- bool isManualReset = false;
- try {
- isManualReset = m_Event->IsManualReset();
- } catch (...) {
- return;
- }
-
+ bool isManualReset = m_Event->IsManualReset();
+
if (isManualReset)
{
auto& pool = TEventPool<EEventMode::ManualReset>::Get();If object validity is genuinely in question, this indicates a deeper design or lifecycle issue that should be fixed at the source.
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/HAL/Event.cpp around lines 85-108, the destructor
currently uses a try/catch(...) around m_Event->IsManualReset() which silently
returns on any exception and can leak the event; remove the exception handling
and the vtable-validity check: keep the nullptr check only, directly call
IsManualReset(), and always return the event to the correct pool (or delete it)
based on the result; if you need a safety check for debug builds, replace the
catch with an assert or annotated comment that the lifetime must be guaranteed
by callers and ensure ownership rules are fixed instead of swallowing
exceptions.
| FEvent() | ||
| : m_EventId(0) | ||
| , m_EventStartCycles(0) | ||
| { | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider initializing m_EventId from s_EventUniqueId.
The constructor initializes m_EventId to 0, but the static s_EventUniqueId counter suggests each event should receive a unique ID.
🔎 Proposed enhancement
FEvent()
- : m_EventId(0)
+ : m_EventId(s_EventUniqueId.fetch_add(1, std::memory_order_relaxed))
, m_EventStartCycles(0)
{
}This ensures each event gets a unique identifier for debugging and profiling purposes.
📝 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.
| FEvent() | |
| : m_EventId(0) | |
| , m_EventStartCycles(0) | |
| { | |
| } | |
| FEvent() | |
| : m_EventId(s_EventUniqueId.fetch_add(1, std::memory_order_relaxed)) | |
| , m_EventStartCycles(0) | |
| { | |
| } |
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/HAL/Event.h around lines 104 to 108, the default
constructor sets m_EventId to 0 but should assign a unique id from the static
s_EventUniqueId; change the constructor to initialize m_EventId by fetching and
incrementing s_EventUniqueId (use the appropriate atomic or thread-safe
increment method already used for s_EventUniqueId in the codebase), and ensure
s_EventUniqueId is incremented after assignment so each FEvent gets a unique
identifier for debugging/profiling.
…formatting and exclusions
- Reorganized member variables and methods in Sandbox3D.h for better clarity and maintainability. - Updated access specifiers for consistency and clarity. - Enhanced comments for better understanding of scene management and rendering methods. - Minor formatting adjustments in SandboxApp.cpp for improved readability.
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
OloEngine/src/OloEngine/Asset/AssetSystem/RuntimeAssetSystem.h (1)
98-112: Fix typo instd::hash<UUID>specialization:sizetshould besize_t.The
std::hash<OloEngine::UUID>specialization inOloEngine/src/OloEngine/Core/UUID.hcontains a critical typo in the return type of the operator(). The return type is written assizetbut should besize_t. This preventsstd::unordered_set<AssetHandle>from compiling, sinceAssetHandleis a type alias forUUID.OloEngine/src/OloEngine/Audio/AudioCallback.cpp (1)
26-33: Add null check for pFrameCountOut before dereferencing at line 31.The pointer
pFrameCountOutis dereferenced without validation. While miniaudio typically provides valid pointers, defensive checks exist elsewhere in ProcessBlockBase (line 159) that acknowledge miniaudio can pass null frame count pointers for source-style nodes. Protect against this by checkingpFrameCountOutbefore use:if (pFrameCountOut && *pFrameCountOut > 0) { ma_silence_pcm_frames(ppFramesOut[i], *pFrameCountOut, ma_format_f32, channelCount); }OloEngine/src/OloEngine/Animation/BoneEntityUtils.cpp (2)
238-253: Wrap the break statement in braces for consistency.Line 242 contains a
break;statement following anifcondition without explicit braces. Although minor, the coding guidelines prefer braces on new lines except for trivial cases. For consistency with the rest of the codebase (where most control structures use explicit braces), add braces around thebreakstatement.🔎 Proposed refactor
// Check for cycles - if this parent was already visited, break to prevent infinite loop UUID parentUUID = parentEntity.GetUUID(); if (visitedParents.find(parentUUID) != visitedParents.end()) - break; + { + break; + }
15-68: Consider adding profiling instrumentation to performance-critical functions.The recursive tree-traversal and transform-calculation functions in this file would benefit from profiling macros to monitor performance in production builds. While not a functional issue, adding
OLO_PROFILE_FUNCTION()at the start of public functions andOLO_PROFILE_SCOPE(name)around hot loops (e.g., the loop at lines 31–65) would improve observability.OloEditor/src/Panels/AssetPackBuilderPanel.cpp (2)
321-357: Race condition: auxiliary threads may outlive their captured references.The
progressMonitorandcancellationMonitorjthreads capturefloatProgressandcancelRequestedby reference. While std::jthread's destructor will join these threads before the lambda exits, there's a subtle timing issue:
cancellationMonitorsetscancelRequested.store(true)only after detectingstopToken.stop_requested()- But the build call on line 348 may have already completed before this flag is set
- The intent to bridge
stop_token→cancelRequestedis sound, but the 10ms polling delay means cancellation won't propagate instantlyMore importantly, the
progressMonitor's inner lambda declares an unusedstd::stop_tokenparameter while actually using the outerstopTokencaptured by value—this is confusing and should be cleaned up.🔎 Proposed improvement for clarity
- std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token) { + std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token /*unused*/) { while (!stopToken.stop_requested()) {Or better, remove the unused parameter entirely and rely on the captured
stopToken:- std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token) { + std::jthread progressMonitor([this, &floatProgress, stopToken]() {
362-383: State inconsistency: UI state updated before build thread actually stops.
CancelBuild()immediately setsm_IsBuildInProgress.store(false)and resets progress to 0 (lines 376-377), but the build thread may still be running until it checkscancelRequested. This creates several issues:
- Double-build risk: User could click "Build" again while the previous build is still running
- Progress flicker: Build thread may continue updating
m_BuildProgressPermilleafter it's reset to 0- Result overwrite: If user starts a new build,
m_LastBuildResultcould be overwritten by the still-running previous buildThe comment on lines 381-382 acknowledges this but the destructor wait doesn't help if a new build is started.
🔎 Proposed fix
void AssetPackBuilderPanel::CancelBuild() { if (!m_IsBuildInProgress.load()) { return; } // Request cancellation using C++20 structured cancellation if (m_BuildThread.joinable()) { m_BuildThread.request_stop(); + // Wait for the thread to actually complete before updating UI state + m_BuildThread.join(); } - // Update UI state immediately for responsive feedback + // Update UI state after thread has stopped m_IsBuildInProgress.store(false); m_BuildProgressPermille.store(0); OLO_CORE_INFO("Asset pack build cancellation requested"); - - // Note: The actual build may continue in the background until completion - // The destructor will wait() on the future to ensure proper cleanup }If blocking the UI thread during cancellation is unacceptable, consider adding a separate
m_CancellationRequestedflag that prevents starting a new build until the previous thread has joined.OloEngine/src/OloEngine/Asset/AssetManager/RuntimeAssetManager.h (1)
101-123: Redundantprivate:access specifier.Line 123 declares
private:again after line 101 already established a private section. This is syntactically valid but unnecessary.🔎 Suggested fix
private: /** * @brief Load an asset from the asset pack system * @param handle Asset handle to load * @return Loaded asset or nullptr if failed */ Ref<Asset> LoadAssetFromPack(AssetHandle handle); /** * @brief Check if an asset exists in any loaded pack * @param handle Asset handle to check * @return True if asset exists in packs */ bool AssetExistsInPacks(AssetHandle handle) const; /** * @brief Get asset type from pack metadata * @param handle Asset handle * @return Asset type or AssetType::None if not found */ AssetType GetAssetTypeFromPacks(AssetHandle handle) const; - private: // Loaded assets cache std::unordered_map<AssetHandle, Ref<Asset>> m_LoadedAssets;OloEngine/src/OloEngine/Animation/AnimationAsset.cpp (1)
23-126: Consider extracting dependency re-registration to reduce duplication.The dependency re-registration logic (lines 67-74, 90-97, 104-111) is duplicated three times. This increases maintenance burden and risk of inconsistency.
🔎 Proposed refactor using a scope guard or helper lambda
Application::Get().SubmitToMainThread([selfHandle, dependencyHandle = handle, animationSource, mesh]() { + // Helper to re-register dependencies + auto reregisterDependencies = [&]() { + if (animationSource != 0) + { + AssetManager::RegisterDependency(selfHandle, animationSource); + } + if (mesh != 0) + { + AssetManager::RegisterDependency(selfHandle, mesh); + } + }; + try { // Deregister existing dependencies before reload AssetManager::DeregisterDependencies(selfHandle); // Trigger synchronous reload of this animation asset bool reloadSuccess = AssetManager::ReloadData(selfHandle); // Always re-register dependencies regardless of reload success - if (animationSource != 0) - { - AssetManager::RegisterDependency(selfHandle, animationSource); - } - if (mesh != 0) - { - AssetManager::RegisterDependency(selfHandle, mesh); - } + reregisterDependencies(); if (reloadSuccess) { // ... success logging } // ... } catch (const std::exception& e) { - if (animationSource != 0) - { - AssetManager::RegisterDependency(selfHandle, animationSource); - } - if (mesh != 0) - { - AssetManager::RegisterDependency(selfHandle, mesh); - } + reregisterDependencies(); OLO_CORE_ERROR("AnimationAsset::OnDependencyUpdated failed during reload: {}", e.what()); } catch (...) { - if (animationSource != 0) - { - AssetManager::RegisterDependency(selfHandle, animationSource); - } - if (mesh != 0) - { - AssetManager::RegisterDependency(selfHandle, mesh); - } + reregisterDependencies(); OLO_CORE_ERROR("AnimationAsset::OnDependencyUpdated failed during reload: unknown exception"); } });OloEngine/src/CMakeLists.txt (1)
496-540: Add missing Async directory files to CMakeLists.txt.The new
OloEngine/Async/*files in this PR are not listed in CMakeLists.txt:
OloEngine/Async/Async.hOloEngine/Async/Future.hOloEngine/Async/QueuedWork.hOloEngine/Async/QueuedThreadPool.hOloEngine/Async/QueuedThreadPool.cppThese files won't be compiled without adding them to the SOURCES list. Insert after line 465 (end of Task section) and before the Templates section:
"OloEngine/Async/Async.h" "OloEngine/Async/Future.h" "OloEngine/Async/QueuedWork.h" "OloEngine/Async/QueuedThreadPool.h" "OloEngine/Async/QueuedThreadPool.cpp"
♻️ Duplicate comments (15)
OloEditor/assets/backpack/source_attribution.txt (1)
3-3: Grammar issue already flagged in previous review—still needs fixing.This issue was flagged before: the phrase "easier load" should be hyphenated as "easier-to-load" when used as a compound modifier. Since you're already touching this file to add the trailing newline, apply this grammar fix as well.
🔎 Suggested fix
-Modified material assignment (Joey de Vries) for easier load in OpenGL model loading chapter, and renamed albedo to diffuse and metallic to specular to match non-PBR lighting setup. +Modified material assignment (Joey de Vries) for easier-to-load in OpenGL model loading chapter, and renamed albedo to diffuse and metallic to specular to match non-PBR lighting setup..github/workflows/pre-commit.yml (3)
5-7: Fix YAML formatting for branch patterns.Remove extra spaces inside brackets to comply with YAML style guidelines.
🔎 Proposed fix
- branches: [ "**" ] + branches: ["**"] pull_request: - branches: [ "**" ] + branches: ["**"]
16-19: Update to actions/setup-python@v5.The v4 action is deprecated and incompatible with current GitHub Actions runners.
🔎 Proposed fix
- name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: '3.x'
21-25: Consider caching pre-commit environments for faster CI runs.Caching pre-commit's virtual environments can significantly speed up workflow execution on subsequent runs.
🔎 Suggested enhancement
- name: Install pre-commit run: python -m pip install --upgrade pip pre-commit + - name: Cache pre-commit + uses: actions/cache@v4 + with: + path: ~/.cache/pre-commit + key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }} + - name: Run pre-commit run: pre-commit run --all-files.pre-commit-config.yaml (1)
22-24: Remove redundantnameoverride.The
name: clang-formatfield on line 23 is redundant since the hook id is alreadyclang-format.🔎 Proposed refactor
hooks: - id: clang-format - name: clang-format files: '\.(c|cc|cpp|cxx|h|hh|hpp|ipp|inl)$' exclude: '^OloEngine/(vendor|mono)/' args: ["--style=file"].clang-format (1)
37-37: ColumnLimit: 0 disables line-length enforcement.This setting was previously flagged. Consider setting a reasonable limit (e.g., 100 or 120) to maintain code readability and facilitate code review workflows.
OloEngine/CMakeLists.txt (1)
118-118: Hardcoded Windows library breaks cross-platform builds.This issue was already flagged in a previous review.
Synchronization.libprovides theWaitOnAddress/WakeByAddressAPIs which are Windows-specific. This will fail on Linux and macOS builds.OloEngine/src/OloEngine/Async/QueuedThreadPool.cpp (2)
74-77: Inconsistent memory ordering for task count increment.Using
memory_order_acquirewithfetch_addis unusual for a counter increment. The same issue exists at lines 107, 158, and 193. Usememory_order_acq_relormemory_order_releasefor proper synchronization on the write side.
221-236:RemoveAt(0)has O(n) complexity per dequeue.Removing from the front of a dynamic array requires shifting all remaining elements. For high-throughput scenarios, consider using
std::dequefor O(1) front removal.OloEngine/src/CMakeLists.txt (2)
444-465: Verify Task.cpp and LocalWorkQueue.h are included if they exist.A previous review noted that
OloEngine/Task/Task.cppandOloEngine/Task/LocalWorkQueue.hmay be missing from this list. If these files exist in the repository, they should be added to ensure proper compilation.#!/bin/bash # Check if the potentially missing Task files exist echo "=== Checking for Task.cpp ===" fd -t f "Task.cpp" OloEngine/src/OloEngine/Task/ 2>/dev/null || echo "Not found via fd" ls -la OloEngine/src/OloEngine/Task/Task.cpp 2>/dev/null || echo "Task.cpp does not exist" echo "" echo "=== Checking for LocalWorkQueue.h ===" fd -t f "LocalWorkQueue.h" OloEngine/src/OloEngine/Task/ 2>/dev/null || echo "Not found via fd" ls -la OloEngine/src/OloEngine/Task/LocalWorkQueue.h 2>/dev/null || echo "LocalWorkQueue.h does not exist"
252-274: Consider conditional inclusion for platform-specific headers.
WindowsEvent.h(line 273) is listed unconditionally. While the source code itself has#ifdef OLO_PLATFORM_WINDOWSguards, CMake best practices suggest conditionally including platform-specific files:if(WIN32) "OloEngine/HAL/Windows/WindowsEvent.h" endif()This is a minor improvement for cleaner CMake organization.
OloEngine/src/OloEngine/Async/Future.h (1)
742-746:GetFuture()allows multiple calls despite documentation.The comment states "This should only be called once per promise" but the implementation doesn't enforce this. Multiple calls create multiple
TFutureinstances sharing the same state.This was flagged in a previous review. While
std::promise::get_future()throws on the second call, this implementation may intentionally differ. If single-call semantics are desired, consider adding enforcement.Is the current behavior (allowing multiple
GetFuture()calls) intentional? If not, add a flag to track and assert:+ private: + bool m_bFutureRetrieved = false; + + public: TFuture<ResultType> GetFuture() { OLO_CORE_ASSERT(m_State, "Promise already moved"); + OLO_CORE_ASSERT(!m_bFutureRetrieved, "GetFuture() can only be called once"); + m_bFutureRetrieved = true; return TFuture<ResultType>(m_State); }OloEngine/src/OloEngine/Async/Async.h (2)
246-258: Fix failure path that uses moved-from callable and leaves original promise unfulfilled.This issue was flagged in a previous review and remains unaddressed. When
FRunnableThread::Createfails:
FunctionandPromisewere already moved intoRunnable(line 210)delete Runnabledestroys the moved-into copies (line 249)SetPromise(SyncPromise, Function)calls the moved-fromFunction— undefined behavior- Returns a new
SyncFuturewhile the originalFuture(line 180) remains unfulfilled🔎 Proposed fix
else { - // Thread creation failed, run synchronously - delete Runnable; - TPromise<ResultType> SyncPromise; - TFuture<ResultType> SyncFuture = SyncPromise.GetFuture(); - SetPromise(SyncPromise, Function); - if (CompletionCallback) - { - CompletionCallback(); - } - return SyncFuture; + // Thread creation failed: execute inline using Runnable's copies + Runnable->Run(); + delete Runnable; + if (CompletionCallback) + { + CompletionCallback(); + } + return MoveTemp(Future); }
355-366: Same moved-from callable issue in AsyncThread failure path.Identical problem:
FunctionandPromiseare moved intoRunnableat line 321, then the failure path deletesRunnableand attempts to use the moved-fromFunction.🔎 Proposed fix
else { - delete Runnable; - // Run synchronously on failure - TPromise<ResultType> SyncPromise; - SetPromise(SyncPromise, Function); - if (CompletionCallback) - { - CompletionCallback(); - } - return SyncPromise.GetFuture(); + // Thread creation failed: execute inline using Runnable's copies + Runnable->Run(); + delete Runnable; + if (CompletionCallback) + { + CompletionCallback(); + } + return MoveTemp(Future); }OloEngine/src/OloEngine/Async/QueuedThreadPool.h (1)
64-202: Past review comments still apply to FThreadPoolPriorityQueue.Two previous reviews noted:
m_NumQueuedWorkis atomic despite the class being documented as "NOT thread-safe" - consider documenting why (e.g., lock-free reads) or using plaini32.Dequeue()usesRemoveAt(0)which is O(n) due to element shifting. For high-throughput scenarios, considerstd::dequeorTDeque.These are nitpicks and don't block functionality since the class requires external synchronization anyway.
| - repo: https://github.com/pre-commit/mirrors-clang-format | ||
| rev: v21.1.8 | ||
| hooks: | ||
| - id: clang-format | ||
| name: clang-format | ||
| files: '\.(c|cc|cpp|cxx|h|hh|hpp|ipp|inl)$' | ||
| exclude: '^OloEngine/(vendor|mono)/' | ||
| args: ["--style=file"] |
There was a problem hiding this comment.
Update mirrors-clang-format version from v21.1.8 to v21.1.7.
v21.1.8 does not exist; the latest clang-format release is v21.1.7 (released December 4, 2025). Using a non-existent version will cause pre-commit installation to fail.
🤖 Prompt for AI Agents
.pre-commit-config.yaml lines 19-26: the clang-format hook references a
non-existent rev v21.1.8 so pre-commit install fails; change the rev to the
correct existing release v21.1.7 (or run `pre-commit autoupdate --repo
https://github.com/pre-commit/mirrors-clang-format --freeze` to pin the latest
valid tag), then save and run `pre-commit install` (and `pre-commit run
--all-files` to verify).
| ## Pre-commit hooks (style & sanity checks) ✅ | ||
| We use `pre-commit` to enforce formatting and basic repository checks (e.g., trailing whitespace, EOF fixes, clang-format). |
There was a problem hiding this comment.
Add blank line below heading per markdown best practices.
The static analysis tool flagged missing blank lines below headings at lines 5 and 16. Adding blank lines improves readability and satisfies markdown linting rules.
🔎 Proposed fix
## Pre-commit hooks (style & sanity checks) ✅
+
We use `pre-commit` to enforce formatting and basic repository checks (e.g., trailing whitespace, EOF fixes, clang-format).Also apply the same fix at line 16:
### Fix warnings about deprecated hook stages
+
If you see warnings about deprecated stages in `pre-commit` hooks (e.g. `commit`, `push`), update the affected hook repo:📝 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.
| ## Pre-commit hooks (style & sanity checks) ✅ | |
| We use `pre-commit` to enforce formatting and basic repository checks (e.g., trailing whitespace, EOF fixes, clang-format). | |
| ## Pre-commit hooks (style & sanity checks) ✅ | |
| We use `pre-commit` to enforce formatting and basic repository checks (e.g., trailing whitespace, EOF fixes, clang-format). |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In CONTRIBUTING.md around lines 5 and 16, the headings lack the required blank
line beneath them; insert a single empty line immediately after each heading at
line 5 and line 16 to satisfy markdown linting and improve readability, ensuring
no other content or list items directly follow the heading on the same line.
| static Font* s_Font; | ||
|
|
||
| EditorLayer::EditorLayer() | ||
| : Layer("EditorLayer"), m_CameraController(1280.0f / 720.0f), m_SquareColor({ 0.2f, 0.3f, 0.8f, 1.0f }) | ||
| { | ||
| s_Font = new Font("C:/Windows/Fonts/arial.ttf"); | ||
| } |
There was a problem hiding this comment.
Hardcoded Windows font path breaks cross-platform support.
The path "C:/Windows/Fonts/arial.ttf" is Windows-specific and will fail on Linux/macOS builds. This should use platform-specific font discovery or bundle a fallback font with the application.
🔎 Proposed fix using platform detection
EditorLayer::EditorLayer()
: Layer("EditorLayer"), m_CameraController(1280.0f / 720.0f), m_SquareColor({ 0.2f, 0.3f, 0.8f, 1.0f })
{
+#if defined(OLO_PLATFORM_WINDOWS)
s_Font = new Font("C:/Windows/Fonts/arial.ttf");
+#elif defined(OLO_PLATFORM_LINUX)
+ s_Font = new Font("/usr/share/fonts/truetype/dejavu/DejaVuSans.ttf");
+#elif defined(OLO_PLATFORM_MACOS)
+ s_Font = new Font("/System/Library/Fonts/Helvetica.ttc");
+#else
+ s_Font = new Font("Resources/Fonts/OpenSans-Regular.ttf"); // Bundled fallback
+#endif
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 27-27: variable 's_Font' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 27-27: variable 's_Font' provides global access to a non-const object; consider making the pointed-to data 'const'
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 29-29: constructor does not initialize these fields: m_CameraController, m_SquareVA, m_FlatColorShader, m_Framebuffer, m_ActiveScene, m_EditorScene, m_EditorScenePath, m_SquareEntity, m_CameraEntity, m_SecondCamera, m_HoveredEntity, m_EditorCamera, m_CheckerboardTexture, m_BuildFuture, m_ContentBrowserPanel, m_AssetPackBuilderPanel, m_IconPlay, m_IconPause, m_IconSimulate, m_IconStep, m_IconStop
(cppcoreguidelines-pro-type-member-init)
[warning] 29-29: use '= default' to define a trivial default constructor
(modernize-use-equals-default)
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In OloEditor/src/EditorLayer.cpp around lines 27 to 33, the code hardcodes a
Windows-only font path ("C:/Windows/Fonts/arial.ttf") which breaks
cross-platform builds; replace this with platform-specific discovery or a
bundled fallback: detect the OS at compile/runtime and attempt known platform
locations (Windows: C:/Windows/Fonts/..., macOS: /Library/Fonts/... or
~/Library/Fonts, Linux: /usr/share/fonts/truetype/... or
/usr/local/share/fonts), or better yet include a bundled font in the project
resources and load it via a relative/resource path; ensure you try each
candidate path in order and fall back to the bundled font and handle failures
(log error and avoid dereferencing null) so the app works on all platforms.
| static Font* s_Font; | ||
|
|
There was a problem hiding this comment.
Memory leak: s_Font is never deleted.
The static Font* is allocated with new in the constructor but never freed. Consider using a smart pointer or adding cleanup logic.
🔎 Proposed fix
-static Font* s_Font;
+static std::unique_ptr<Font> s_Font;
EditorLayer::EditorLayer()
: Layer("EditorLayer"), m_CameraController(1280.0f / 720.0f), m_SquareColor({ 0.2f, 0.3f, 0.8f, 1.0f })
{
- s_Font = new Font("C:/Windows/Fonts/arial.ttf");
+ s_Font = std::make_unique<Font>("C:/Windows/Fonts/arial.ttf");
}Update the usage in UI_Settings accordingly:
-ImGui::Image((ImTextureID)s_Font->GetAtlasTexture()->GetRendererID(), { 512, 512 }, { 0, 1 }, { 1, 0 });
+ImGui::Image((ImTextureID)s_Font->GetAtlasTexture()->GetRendererID(), { 512, 512 }, { 0, 1 }, { 1, 0 });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 27-27: variable 's_Font' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 27-27: variable 's_Font' provides global access to a non-const object; consider making the pointed-to data 'const'
(cppcoreguidelines-avoid-non-const-global-variables)
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In OloEditor/src/EditorLayer.cpp around lines 27-28, the static pointer s_Font
is allocated but never freed causing a memory leak; replace the raw static Font*
with a smart pointer (e.g., static std::unique_ptr<Font>) or add explicit
cleanup to delete s_Font at shutdown, and update any code that accesses s_Font
(including UI_Settings) to use the smart pointer API (or nullptr checks) so
construction and destruction are managed safely.
| class OloEngineEditor : public Application | ||
| { | ||
| public: | ||
| explicit OloEngineEditor(const ApplicationSpecification& spec) | ||
| : Application(spec) | ||
| { | ||
| PushLayer(new EditorLayer()); | ||
| } | ||
|
|
||
| ~OloEngineEditor() final = default; | ||
| }; | ||
| ~OloEngineEditor() final = default; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Define or delete all special member functions for OloEngineEditor.
The class has an explicitly defined destructor (line 20) but lacks explicit copy and move constructors/assignment operators. Following the Rule of Five, either provide all five operations or explicitly delete the unneeded ones.
🔎 Suggested approach: explicitly delete copy operations if not needed
If OloEngineEditor should not be copyable or movable, explicitly delete those operations:
class OloEngineEditor : public Application
{
public:
explicit OloEngineEditor(const ApplicationSpecification& spec)
: Application(spec)
{
PushLayer(new EditorLayer());
}
~OloEngineEditor() final = default;
+
+ OloEngineEditor(const OloEngineEditor&) = delete;
+ OloEngineEditor& operator=(const OloEngineEditor&) = delete;
+ OloEngineEditor(OloEngineEditor&&) = delete;
+ OloEngineEditor& operator=(OloEngineEditor&&) = delete;
};Alternatively, if move semantics are desired, define them explicitly or use = default if the base class supports it.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 11-11: class 'OloEngineEditor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
(cppcoreguidelines-special-member-functions)
[warning] 14-14: parameter 'spec' is unused
(misc-unused-parameters)
🪛 Cppcheck (2.18.0)
[information] 11-11: Include file
(missingIncludeSystem)
[information] 12-12: Include file
(missingIncludeSystem)
[style] 19-19: The function 'GetSelectedEntity' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In OloEditor/src/OloEditorApp.cpp around lines 11 to 21, the class
OloEngineEditor declares a custom destructor but does not explicitly declare
copy/move constructors or copy/move assignment operators; follow the Rule of
Five by either explicitly deleting the copy and move operations if the editor
should not be copied or moved (declare copy ctor, copy assignment, move ctor and
move assignment as = delete) or explicitly declare them (use = default for move
operations only if the base Application supports it) so the intent is clear and
accidental copies/moves are prevented.
| void AudioListener::SetPosition(const glm::vec3& position) const | ||
| { | ||
| auto* engine = (ma_engine*)AudioEngine::GetEngine(); | ||
| ::ma_engine_listener_set_position(engine, m_ListenerIndex, position.x, position.y, position.z); | ||
|
|
||
| void AudioListener::SetDirection(const glm::vec3& forward) const | ||
| { | ||
| auto* engine = (ma_engine*)AudioEngine::GetEngine(); | ||
| ::ma_engine_listener_set_direction(engine, m_ListenerIndex, forward.x, forward.y, forward.z); | ||
| } | ||
| static bool setupWorldUp = false; | ||
| if (!setupWorldUp) | ||
| { | ||
| ::ma_engine_listener_set_world_up(engine, m_ListenerIndex, 0, 1, 0); | ||
| setupWorldUp = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Static world-up setup affects all listeners; replace C-style cast.
Issue 1 (Critical): Lines 21-26 use a static bool setupWorldUp that is shared across all AudioListener instances. This means the world-up vector is only set for the first listener created, and subsequent listeners will not have their world-up configured. Each listener should have its world-up set independently.
Issue 2 (Major): Line 18 uses a C-style cast.
🔎 Proposed fix
void AudioListener::SetPosition(const glm::vec3& position) const
{
- auto* engine = (ma_engine*)AudioEngine::GetEngine();
+ auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine());
::ma_engine_listener_set_position(engine, m_ListenerIndex, position.x, position.y, position.z);
-
- static bool setupWorldUp = false;
- if (!setupWorldUp)
- {
- ::ma_engine_listener_set_world_up(engine, m_ListenerIndex, 0, 1, 0);
- setupWorldUp = true;
- }
+ ::ma_engine_listener_set_world_up(engine, m_ListenerIndex, 0, 1, 0);
}Alternatively, if setting world-up every frame is expensive, consider tracking this per-instance (e.g., add a bool m_WorldUpInitialized member to the class).
🧰 Tools
🪛 Clang (14.0.6)
[warning] 16-16: method 'SetPosition' can be made static
(readability-convert-member-functions-to-static)
[warning] 16-16: parameter 'position' is unused
(misc-unused-parameters)
[warning] 18-18: do not use C-style cast to convert between unrelated types
(cppcoreguidelines-pro-type-cstyle-cast)
🪛 Cppcheck (2.18.0)
[style] 16-16: The function 'SetPosition' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Audio/AudioListener.cpp around lines 16 to 27,
replace the file-scope static bool that gates setting the world-up with a
per-instance flag and replace the C-style cast: add a member bool
m_WorldUpInitialized to AudioListener (initialize to false in the constructor),
change the cast to the engine to use
static_cast<ma_engine*>(AudioEngine::GetEngine()), and in SetPosition call
::ma_engine_listener_set_world_up for this listener when m_WorldUpInitialized is
false then set it true (or set world-up in the constructor using the same
static_cast); this ensures each listener gets its world-up set independently and
eliminates the C-style cast.
| void AudioListener::SetDirection(const glm::vec3& forward) const | ||
| { | ||
| auto* engine = (ma_engine*)AudioEngine::GetEngine(); | ||
| ::ma_engine_listener_set_direction(engine, m_ListenerIndex, forward.x, forward.y, forward.z); | ||
| } |
There was a problem hiding this comment.
Replace C-style cast with static_cast.
Line 31 uses a C-style cast. Use static_cast for consistency with modern C++ guidelines.
🔎 Proposed fix
- auto* engine = (ma_engine*)AudioEngine::GetEngine();
+ auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine());📝 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.
| void AudioListener::SetDirection(const glm::vec3& forward) const | |
| { | |
| auto* engine = (ma_engine*)AudioEngine::GetEngine(); | |
| ::ma_engine_listener_set_direction(engine, m_ListenerIndex, forward.x, forward.y, forward.z); | |
| } | |
| void AudioListener::SetDirection(const glm::vec3& forward) const | |
| { | |
| auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine()); | |
| ::ma_engine_listener_set_direction(engine, m_ListenerIndex, forward.x, forward.y, forward.z); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 29-29: method 'SetDirection' can be made static
(readability-convert-member-functions-to-static)
[warning] 29-29: parameter 'forward' is unused
(misc-unused-parameters)
[warning] 31-31: do not use C-style cast to convert between unrelated types
(cppcoreguidelines-pro-type-cstyle-cast)
🪛 Cppcheck (2.18.0)
[style] 29-29: The function 'SetDirection' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Audio/AudioListener.cpp around lines 29 to 33,
replace the C-style cast on line 31 with a static_cast to follow modern C++
guidelines: change (ma_engine*)AudioEngine::GetEngine() to
static_cast<ma_engine*>(AudioEngine::GetEngine()); keep the rest of the call
unchanged so the listener direction call continues to pass forward.x, forward.y,
forward.z.
| void AudioListener::SetVelocity(const glm::vec3& velocity) const | ||
| { | ||
| auto* engine = (ma_engine*)AudioEngine::GetEngine(); | ||
| ::ma_engine_listener_set_velocity(engine, m_ListenerIndex, velocity.x, velocity.y, velocity.z); | ||
| } |
There was a problem hiding this comment.
Replace C-style cast with static_cast.
Line 37 uses a C-style cast. Use static_cast for consistency with modern C++ guidelines.
🔎 Proposed fix
- auto* engine = (ma_engine*)AudioEngine::GetEngine();
+ auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine());📝 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.
| void AudioListener::SetVelocity(const glm::vec3& velocity) const | |
| { | |
| auto* engine = (ma_engine*)AudioEngine::GetEngine(); | |
| ::ma_engine_listener_set_velocity(engine, m_ListenerIndex, velocity.x, velocity.y, velocity.z); | |
| } | |
| void AudioListener::SetVelocity(const glm::vec3& velocity) const | |
| { | |
| auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine()); | |
| ::ma_engine_listener_set_velocity(engine, m_ListenerIndex, velocity.x, velocity.y, velocity.z); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 35-35: method 'SetVelocity' can be made static
(readability-convert-member-functions-to-static)
[warning] 35-35: parameter 'velocity' is unused
(misc-unused-parameters)
[warning] 37-37: do not use C-style cast to convert between unrelated types
(cppcoreguidelines-pro-type-cstyle-cast)
🪛 Cppcheck (2.18.0)
[style] 35-35: The function 'SetVelocity' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Audio/AudioListener.cpp around lines 35 to 39,
replace the C-style cast on line 37 with a C++ static_cast: change
"(ma_engine*)AudioEngine::GetEngine()" to
"static_cast<ma_engine*>(AudioEngine::GetEngine())" to follow modern C++ casting
guidelines while preserving the pointer type and behavior.
| schedParam.sched_priority = sched_get_priority_min(SCHED_RR) + | ||
| ((sched_get_priority_max(SCHED_RR) - sched_get_priority_min(SCHED_RR)) * 3 / 4); | ||
| result = pthread_setschedparam(thread, SCHED_RR, &schedParam); |
There was a problem hiding this comment.
Missing error check for SCHED_RR priority range.
When falling back to SCHED_RR, sched_get_priority_min/max(SCHED_RR) could return -1 on error, but this isn't checked before computing the priority.
🔎 Proposed fix
// Fall back to trying SCHED_RR
+ i32 rrMin = sched_get_priority_min(SCHED_RR);
+ i32 rrMax = sched_get_priority_max(SCHED_RR);
+ if (rrMin == -1 || rrMax == -1)
+ {
+ OLO_CORE_WARN("AudioThread: Failed to get SCHED_RR priority range (errno: {})", errno);
+ // Skip SCHED_RR attempt, fall through to warning below
+ }
+ else
+ {
- schedParam.sched_priority = sched_get_priority_min(SCHED_RR) +
- ((sched_get_priority_max(SCHED_RR) - sched_get_priority_min(SCHED_RR)) * 3 / 4);
+ schedParam.sched_priority = rrMin + ((rrMax - rrMin) * 3 / 4);
- result = pthread_setschedparam(thread, SCHED_RR, &schedParam);
+ result = pthread_setschedparam(thread, SCHED_RR, &schedParam);
+ }🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Audio/AudioThread.cpp around lines 264 to 266, the
code computes a SCHED_RR priority using sched_get_priority_min/max without
checking for error returns (-1); validate both sched_get_priority_min(SCHED_RR)
and sched_get_priority_max(SCHED_RR) before using them, handle error cases by
logging and choosing a safe fallback (e.g., skip setting SCHED_RR or use a
default/clamped priority), and only call pthread_setschedparam when the min/max
values are valid; ensure any arithmetic avoids overflow and clamps the computed
priority to the valid [min,max] range.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
OloEngine-ScriptCore/src/OloEngine/Input.cs (1)
1-9: Replace tabs with 4-space indentation.The file uses tab characters for indentation (lines 3-9), but the coding guidelines require 4-space indentation for C# files. Convert all tabs to spaces.
OloEngine/src/OloEngine/Animation/AnimationAsset.cpp (1)
67-111: Refactor: Extract duplicate dependency re-registration into a scope guard.The same dependency re-registration logic is repeated in three places (success path,
std::exceptioncatch, andcatch(...)). This violates DRY and creates maintenance risk if the registration logic changes.Consider using a scope guard or RAII wrapper to ensure dependencies are always re-registered on scope exit:
🔎 Proposed refactor using a simple scope guard
Application::Get().SubmitToMainThread([selfHandle, dependencyHandle = handle, animationSource, mesh]() { + // Scope guard to ensure dependencies are re-registered on any exit path + auto registerDependenciesOnExit = [&]() + { + if (animationSource != 0) + { + AssetManager::RegisterDependency(selfHandle, animationSource); + } + if (mesh != 0) + { + AssetManager::RegisterDependency(selfHandle, mesh); + } + }; + try { // Deregister existing dependencies before reload AssetManager::DeregisterDependencies(selfHandle); // Trigger synchronous reload of this animation asset bool reloadSuccess = AssetManager::ReloadData(selfHandle); - // Always re-register dependencies regardless of reload success to preserve dependency graph - if (animationSource != 0) - { - AssetManager::RegisterDependency(selfHandle, animationSource); - } - if (mesh != 0) - { - AssetManager::RegisterDependency(selfHandle, mesh); - } + // Re-register dependencies to preserve dependency graph + registerDependenciesOnExit(); if (reloadSuccess) { OLO_CORE_INFO("AnimationAsset {} reload successful due to dependency {} update", static_cast<u64>(selfHandle), static_cast<u64>(dependencyHandle)); } else { OLO_CORE_ERROR("AnimationAsset {} reload failed due to dependency {} update", static_cast<u64>(selfHandle), static_cast<u64>(dependencyHandle)); } } catch (const std::exception& e) { - // Re-register dependencies even in exception paths to preserve dependency graph - if (animationSource != 0) - { - AssetManager::RegisterDependency(selfHandle, animationSource); - } - if (mesh != 0) - { - AssetManager::RegisterDependency(selfHandle, mesh); - } + registerDependenciesOnExit(); OLO_CORE_ERROR("AnimationAsset::OnDependencyUpdated failed during reload: {}", e.what()); } catch (...) { - // Re-register dependencies even in exception paths to preserve dependency graph - if (animationSource != 0) - { - AssetManager::RegisterDependency(selfHandle, animationSource); - } - if (mesh != 0) - { - AssetManager::RegisterDependency(selfHandle, mesh); - } + registerDependenciesOnExit(); OLO_CORE_ERROR("AnimationAsset::OnDependencyUpdated failed during reload: unknown exception"); } });Alternatively, if you have a scope guard utility (like
ScopeGuardordefer), that would be even cleaner.OloEditor/src/Panels/AssetPackBuilderPanel.cpp (1)
330-337: Critical: Progress monitor checks wrong stop token, causing potential hang.The
progressMonitorlambda captures the build thread'sstopTokenby value and has its ownstd::stop_tokenparameter (passed bystd::jthread), but the loop checks the capturedstopToken(build thread's token) instead of the monitor's own stop token. When the build completes normally (without cancellation), the lambda exits andprogressMonitor's destructor requests stop on the monitor's token and attempts to join—but the loop never sees that request because it's checking the build thread's token. This causes a hang.🔎 Proposed fix
Name the monitor's stop token parameter and check both tokens:
- std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token) { - while (!stopToken.stop_requested()) { + std::jthread progressMonitor([this, &floatProgress, stopToken](std::stop_token monitorStopToken) { + while (!stopToken.stop_requested() && !monitorStopToken.stop_requested()) { f32 progress = floatProgress.load(); i32 permille = static_cast<i32>(progress * 1000.0f); m_BuildProgressPermille.store(permille); std::this_thread::sleep_for(std::chrono::milliseconds(50)); } });OloEditor/src/EditorLayer.cpp (1)
1119-1156: Add catch-all handler in async lambda for robust cleanup.The lambda only catches
std::exception, but if a non-standard exception is thrown (e.g., SEH on Windows),m_BuildInProgresswon't be cleared. While the destructor'scatch(...)prevents crash on teardown, the stalem_BuildInProgress = truecould block subsequent builds during normal operation.🔎 Proposed fix
catch (const std::exception& ex) { m_BuildInProgress.store(false); OLO_CORE_ERROR("Asset Pack build exception: {}", ex.what()); AssetPackBuilder::BuildResult errorResult{}; errorResult.m_Success = false; errorResult.m_ErrorMessage = ex.what(); errorResult.m_OutputPath.clear(); errorResult.m_AssetCount = 0; errorResult.m_SceneCount = 0; return errorResult; + } + catch (...) + { + m_BuildInProgress.store(false); + OLO_CORE_ERROR("Asset Pack build failed with unknown exception"); + AssetPackBuilder::BuildResult errorResult{}; + errorResult.m_Success = false; + errorResult.m_ErrorMessage = "Unknown exception during build"; + errorResult.m_OutputPath.clear(); + errorResult.m_AssetCount = 0; + errorResult.m_SceneCount = 0; + return errorResult; }
♻️ Duplicate comments (4)
OloEditor/src/Panels/AssetPackBuilderPanel.cpp (1)
67-70: Progress set to 100% unconditionally after join, regardless of build outcome.The issue raised in the previous review remains unaddressed: after joining the build thread,
m_BuildProgressPermilleis set to 1000 (100%) even if the build failed or was cancelled (line 69). This overrides the conditional progress setting in the build thread (lines 351-353) and can mislead users by showing 100% progress for failed builds.🔎 Proposed fix
Either remove line 69 entirely (since the build thread already sets progress conditionally), or make it conditional:
m_BuildThread.join(); m_HasBuildResult.store(true); - m_BuildProgressPermille.store(1000); // 100% in permille + // Progress already set by build thread based on outcome if (m_LastBuildResult.m_Success)OloEditor/src/EditorLayer.cpp (1)
27-33: Duplicate: Font initialization issues already flagged.The hardcoded Windows font path and memory leak concerns have been raised in previous review comments.
OloEngine/src/CMakeLists.txt (1)
252-273: HAL infrastructure additions look comprehensive.The HAL additions provide the threading primitives (
Event,ParkingLot,ManualResetEvent,Semaphore) and platform abstractions (Runnable,RunnableThread,Thread,ThreadManager) required by the task system.Note:
WindowsEvent.h(line 273) is listed unconditionally. While source-level#ifdef OLO_PLATFORM_WINDOWSguards prevent cross-platform build failures, conditionally including platform-specific files in CMake is a cleaner approach.OloEditor/src/Panels/SceneHierarchyPanel.cpp (1)
116-116: Replace hardcoded pointer literal with deterministic ID.The hardcoded pointer value
(void*)9817239was previously flagged in an earlier review. This magic number should be replaced with a meaningful, deterministic ID derived from the entity or a named constant to ensure correct tree state behavior and improve maintainability.Refer to the previous review comment for the recommended fix.
| ImGui::SetTooltip("Enable expensive physics debug capture during play mode.\nOff by default for production performance."); | ||
| } | ||
|
|
||
| ImGui::Image((ImTextureID)s_Font->GetAtlasTexture()->GetRendererID(), { 512, 512 }, { 0, 1 }, { 1, 0 }); |
There was a problem hiding this comment.
Add null check before dereferencing s_Font.
The chained dereference s_Font->GetAtlasTexture()->GetRendererID() assumes both s_Font and the atlas texture are valid. If the font fails to load or the atlas isn't ready, this could cause a null pointer dereference.
🔎 Proposed defensive fix
-ImGui::Image((ImTextureID)s_Font->GetAtlasTexture()->GetRendererID(), { 512, 512 }, { 0, 1 }, { 1, 0 });
+if (s_Font && s_Font->GetAtlasTexture())
+{
+ ImGui::Image((ImTextureID)s_Font->GetAtlasTexture()->GetRendererID(), { 512, 512 }, { 0, 1 }, { 1, 0 });
+}🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In OloEditor/src/EditorLayer.cpp around line 585, the code dereferences s_Font
and its atlas texture without checks; add defensive null checks so you only call
GetAtlasTexture() and GetRendererID() when s_Font is non-null and
s_Font->GetAtlasTexture() is non-null. If either is null, skip the ImGui::Image
call (or use a fallback/default texture ID) to avoid a potential null pointer
dereference and ensure the UI still renders safely.
| if (m_ShowPhysicsColliders) | ||
| { | ||
| if (const f64 epsilon = 1e-5; std::abs(Renderer2D::GetLineWidth() - -2.0f) > static_cast<f32>(epsilon)) | ||
| { | ||
| Renderer2D::Flush(); | ||
| Renderer2D::SetLineWidth(2.0f); | ||
| } |
There was a problem hiding this comment.
Potential typo: -2.0f should likely be 2.0f.
The comparison Renderer2D::GetLineWidth() - -2.0f evaluates to GetLineWidth() + 2.0f, which is always greater than epsilon for any non-negative line width. This makes the condition always true, defeating the purpose of the check.
The intent appears to be avoiding redundant SetLineWidth(2.0f) calls when the width is already 2.0f:
🔎 Proposed fix
-if (const f64 epsilon = 1e-5; std::abs(Renderer2D::GetLineWidth() - -2.0f) > static_cast<f32>(epsilon))
+if (const f64 epsilon = 1e-5; std::abs(Renderer2D::GetLineWidth() - 2.0f) > static_cast<f32>(epsilon))📝 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.
| if (m_ShowPhysicsColliders) | |
| { | |
| if (const f64 epsilon = 1e-5; std::abs(Renderer2D::GetLineWidth() - -2.0f) > static_cast<f32>(epsilon)) | |
| { | |
| Renderer2D::Flush(); | |
| Renderer2D::SetLineWidth(2.0f); | |
| } | |
| if (m_ShowPhysicsColliders) | |
| { | |
| if (const f64 epsilon = 1e-5; std::abs(Renderer2D::GetLineWidth() - 2.0f) > static_cast<f32>(epsilon)) | |
| { | |
| Renderer2D::Flush(); | |
| Renderer2D::SetLineWidth(2.0f); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 811-811: variable 'epsilon' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In OloEditor/src/EditorLayer.cpp around lines 809 to 815, the comparison uses
Renderer2D::GetLineWidth() - -2.0f which effectively adds 2.0f and makes the
check always true; change the literal from -2.0f to 2.0f so the condition
correctly tests if the current line width differs from 2.0f by more than
epsilon, then call Flush() and SetLineWidth(2.0f) only when needed; ensure the
numeric types are consistent (use matching float/double or cast) and keep the
epsilon comparison using std::abs for the same type.
| void EditorLayer::SaveProject() | ||
| { | ||
| // Project::SaveActive(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding TODO or removing unused stub.
SaveProject is flagged as unused by static analysis and has a commented-out implementation. If this is placeholder for future functionality, a TODO comment would clarify intent.
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 894-894: The function 'SaveProject' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In OloEditor/src/EditorLayer.cpp around lines 894–897, the SaveProject() method
is an unused stub with a commented-out implementation; either remove the entire
method to eliminate dead code, or keep it as an explicit placeholder by adding a
single-line TODO comment describing the intended behavior (e.g., "TODO:
implement project save; call Project::SaveActive() and handle errors") and, if
your static analyzer requires, add an appropriate annotation or suppression
(e.g., [[maybe_unused]] or a comment pragma) so the intent is clear.
| std::jthread cancellationMonitor([&cancelRequested, stopToken](std::stop_token) { | ||
| while (!stopToken.stop_requested()) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(10)); | ||
| } | ||
| cancelRequested.store(true); | ||
| }); |
There was a problem hiding this comment.
Critical: Cancellation monitor checks wrong stop token, causing potential hang.
The cancellationMonitor has the same concurrency bug as the progressMonitor: it captures the build thread's stopToken and checks it in the loop, but its destructor requests stop on the monitor's own token. When the build completes normally, the monitor's destructor tries to join, but the loop never exits because it's checking the build thread's token instead of its own.
🔎 Proposed fix
Name the monitor's stop token parameter, check both tokens, and only set cancelRequested if the build thread's stop was actually requested:
- std::jthread cancellationMonitor([&cancelRequested, stopToken](std::stop_token) {
- while (!stopToken.stop_requested()) {
+ std::jthread cancellationMonitor([&cancelRequested, stopToken](std::stop_token monitorStopToken) {
+ while (!stopToken.stop_requested() && !monitorStopToken.stop_requested()) {
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}
- cancelRequested.store(true);
+ if (stopToken.stop_requested()) {
+ cancelRequested.store(true);
+ }
});🤖 Prompt for AI Agents
In OloEditor/src/Panels/AssetPackBuilderPanel.cpp around lines 340-345, the
cancellationMonitor lambda captures and checks the build thread's stopToken
instead of its own monitor stop token, so the monitor's loop may never exit when
the monitor is requested to stop; rename the monitor lambda parameter (e.g.,
monitorToken) and change the loop to check the monitor's own stop token while
also observing the build thread's stop token, and only set cancelRequested when
the build thread's stop token is requested (i.e., use
monitorToken.stop_requested() to control loop exit and inspect the build
stopToken to decide whether to store true into cancelRequested).
| char buffer[256]; | ||
| ::memset(buffer, 0, sizeof(buffer)); | ||
| ::strncpy_s(buffer, tag.c_str(), sizeof(buffer)); | ||
| if (ImGui::InputText("##Tag", buffer, sizeof(buffer))) | ||
| { | ||
| tag = std::string(buffer); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider replacing C-style string manipulation with modern C++ alternatives.
The code uses a C-style char buffer[256] with strncpy_s for tag editing. Note that strncpy_s is Windows-specific and may impact portability. Consider using std::string directly with ImGui's InputText overload that accepts std::string*, or ensure cross-platform compatibility.
🔎 Proposed refactor using std::string directly
- char buffer[256];
- ::memset(buffer, 0, sizeof(buffer));
- ::strncpy_s(buffer, tag.c_str(), sizeof(buffer));
- if (ImGui::InputText("##Tag", buffer, sizeof(buffer)))
+ if (ImGui::InputText("##Tag", &tag))
{
- tag = std::string(buffer);
+ // tag is already updated by ImGui
}Note: This requires the imgui_stdlib.h header, which is already included at line 8.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 99-99: do not declare C-style arrays, use std::array<> instead
(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
[warning] 99-99: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In OloEditor/src/Panels/SceneHierarchyPanel.cpp around lines 99 to 105, replace
the C-style fixed char buffer and strncpy_s usage with ImGui's std::string
overload: create a temporary std::string initialized from tag, call
ImGui::InputText with the std::string* overload (requires imgui_stdlib.h), and
on input success assign the temp string back to tag; this removes the
Windows-only strncpy_s and improves safety and portability.
| template<typename T, typename UIFunction> | ||
| static void DrawComponent(const std::string& name, Entity entity, UIFunction uiFunction) | ||
| { | ||
| const ImGuiTreeNodeFlags treeNodeFlags = ImGuiTreeNodeFlags_DefaultOpen | ImGuiTreeNodeFlags_Framed | ImGuiTreeNodeFlags_SpanAvailWidth | ImGuiTreeNodeFlags_AllowOverlap | ImGuiTreeNodeFlags_FramePadding; | ||
| if (entity.HasComponent<T>()) | ||
| { | ||
| static char imguiPopupID[64]; | ||
| ::sprintf_s(imguiPopupID, 64, "ComponentSettings%s", typeid(T).name()); | ||
| ImGui::PushID(imguiPopupID); | ||
|
|
||
| auto& component = entity.GetComponent<T>(); | ||
| const ImVec2 contentRegionAvailable = ImGui::GetContentRegionAvail(); | ||
|
|
||
| ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, ImVec2{ 4, 4 }); | ||
| const f32 lineHeight = ImGui::GetFontSize() + (::GImGui->Style.FramePadding.y * 2.0f); | ||
| ImGui::Separator(); | ||
| const bool open = ImGui::TreeNodeEx(reinterpret_cast<void*>(typeid(T).hash_code()), treeNodeFlags, name.c_str()); | ||
| ImGui::PopStyleVar(); | ||
| ImGui::SameLine(contentRegionAvailable.x - (lineHeight * 0.5f)); | ||
| if (ImGui::Button("+", ImVec2{ lineHeight, lineHeight })) | ||
| { | ||
| ImGui::OpenPopup("ComponentSettings"); | ||
| } | ||
|
|
||
| bool removeComponent = false; | ||
| if (ImGui::BeginPopup("ComponentSettings")) | ||
| { | ||
| if (ImGui::MenuItem("Remove component")) | ||
| { | ||
| removeComponent = true; | ||
| } | ||
|
|
||
| ImGui::EndPopup(); | ||
| } | ||
|
|
||
| ImGui::PopID(); | ||
|
|
||
| if (open) | ||
| { | ||
| uiFunction(component); | ||
| ImGui::TreePop(); | ||
| } | ||
|
|
||
| if (removeComponent) | ||
| { | ||
| entity.RemoveComponent<T>(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Generic component drawer is well-designed; note portability concern.
The DrawComponent template provides excellent reusability for component UI rendering. The use of typeid(T).hash_code() for tree node IDs is appropriate.
However, line 213 uses sprintf_s, which is Windows-specific. Consider using snprintf or modern C++ string formatting for cross-platform compatibility.
🔎 Optional: Replace sprintf_s with cross-platform alternative
- static char imguiPopupID[64];
- ::sprintf_s(imguiPopupID, 64, "ComponentSettings%s", typeid(T).name());
+ static char imguiPopupID[64];
+ ::snprintf(imguiPopupID, sizeof(imguiPopupID), "ComponentSettings%s", typeid(T).name());Or use modern C++ string formatting if available:
- static char imguiPopupID[64];
- ::sprintf_s(imguiPopupID, 64, "ComponentSettings%s", typeid(T).name());
- ImGui::PushID(imguiPopupID);
+ const std::string imguiPopupID = std::string("ComponentSettings") + typeid(T).name();
+ ImGui::PushID(imguiPopupID.c_str());📝 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.
| template<typename T, typename UIFunction> | |
| static void DrawComponent(const std::string& name, Entity entity, UIFunction uiFunction) | |
| { | |
| const ImGuiTreeNodeFlags treeNodeFlags = ImGuiTreeNodeFlags_DefaultOpen | ImGuiTreeNodeFlags_Framed | ImGuiTreeNodeFlags_SpanAvailWidth | ImGuiTreeNodeFlags_AllowOverlap | ImGuiTreeNodeFlags_FramePadding; | |
| if (entity.HasComponent<T>()) | |
| { | |
| static char imguiPopupID[64]; | |
| ::sprintf_s(imguiPopupID, 64, "ComponentSettings%s", typeid(T).name()); | |
| ImGui::PushID(imguiPopupID); | |
| auto& component = entity.GetComponent<T>(); | |
| const ImVec2 contentRegionAvailable = ImGui::GetContentRegionAvail(); | |
| ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, ImVec2{ 4, 4 }); | |
| const f32 lineHeight = ImGui::GetFontSize() + (::GImGui->Style.FramePadding.y * 2.0f); | |
| ImGui::Separator(); | |
| const bool open = ImGui::TreeNodeEx(reinterpret_cast<void*>(typeid(T).hash_code()), treeNodeFlags, name.c_str()); | |
| ImGui::PopStyleVar(); | |
| ImGui::SameLine(contentRegionAvailable.x - (lineHeight * 0.5f)); | |
| if (ImGui::Button("+", ImVec2{ lineHeight, lineHeight })) | |
| { | |
| ImGui::OpenPopup("ComponentSettings"); | |
| } | |
| bool removeComponent = false; | |
| if (ImGui::BeginPopup("ComponentSettings")) | |
| { | |
| if (ImGui::MenuItem("Remove component")) | |
| { | |
| removeComponent = true; | |
| } | |
| ImGui::EndPopup(); | |
| } | |
| ImGui::PopID(); | |
| if (open) | |
| { | |
| uiFunction(component); | |
| ImGui::TreePop(); | |
| } | |
| if (removeComponent) | |
| { | |
| entity.RemoveComponent<T>(); | |
| } | |
| } | |
| } | |
| template<typename T, typename UIFunction> | |
| static void DrawComponent(const std::string& name, Entity entity, UIFunction uiFunction) | |
| { | |
| const ImGuiTreeNodeFlags treeNodeFlags = ImGuiTreeNodeFlags_DefaultOpen | ImGuiTreeNodeFlags_Framed | ImGuiTreeNodeFlags_SpanAvailWidth | ImGuiTreeNodeFlags_AllowOverlap | ImGuiTreeNodeFlags_FramePadding; | |
| if (entity.HasComponent<T>()) | |
| { | |
| const std::string imguiPopupID = std::string("ComponentSettings") + typeid(T).name(); | |
| ImGui::PushID(imguiPopupID.c_str()); | |
| auto& component = entity.GetComponent<T>(); | |
| const ImVec2 contentRegionAvailable = ImGui::GetContentRegionAvail(); | |
| ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, ImVec2{ 4, 4 }); | |
| const f32 lineHeight = ImGui::GetFontSize() + (::GImGui->Style.FramePadding.y * 2.0f); | |
| ImGui::Separator(); | |
| const bool open = ImGui::TreeNodeEx(reinterpret_cast<void*>(typeid(T).hash_code()), treeNodeFlags, name.c_str()); | |
| ImGui::PopStyleVar(); | |
| ImGui::SameLine(contentRegionAvailable.x - (lineHeight * 0.5f)); | |
| if (ImGui::Button("+", ImVec2{ lineHeight, lineHeight })) | |
| { | |
| ImGui::OpenPopup("ComponentSettings"); | |
| } | |
| bool removeComponent = false; | |
| if (ImGui::BeginPopup("ComponentSettings")) | |
| { | |
| if (ImGui::MenuItem("Remove component")) | |
| { | |
| removeComponent = true; | |
| } | |
| ImGui::EndPopup(); | |
| } | |
| ImGui::PopID(); | |
| if (open) | |
| { | |
| uiFunction(component); | |
| ImGui::TreePop(); | |
| } | |
| if (removeComponent) | |
| { | |
| entity.RemoveComponent<T>(); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 207-207: 2 adjacent parameters of 'DrawComponent' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 207-207: the first parameter in the range is 'name'
(clang)
[note] 207-207: the last parameter in the range is 'entity'
(clang)
[note] 207-207: 'const int &' and 'int' parameters accept and bind the same kind of values
(clang)
[warning] 207-207: parameter 'name' is unused
(misc-unused-parameters)
[warning] 207-207: parameter 'entity' is unused
(misc-unused-parameters)
[warning] 209-209: variable 'treeNodeFlags' is not initialized
(cppcoreguidelines-init-variables)
[warning] 212-212: do not declare C-style arrays, use std::array<> instead
(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
[warning] 212-212: 64 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 214-214: variable 'imguiPopupID' is not initialized
(cppcoreguidelines-init-variables)
[warning] 217-217: variable 'contentRegionAvailable' is not initialized
(cppcoreguidelines-init-variables)
[warning] 220-220: variable 'lineHeight' is not initialized
(cppcoreguidelines-init-variables)
[warning] 222-222: variable 'open' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
OloEditor/src/Panels/SceneHierarchyPanel.cpp around lines 206 to 254: the call
to sprintf_s (line ~213) is Windows-specific; replace it with a cross-platform
alternative such as snprintf or build the ID using std::string (or std::format
if available) to format "ComponentSettings" + type name, ensure you respect the
buffer size (or avoid a raw buffer entirely by using std::string), and keep the
resulting unique ID used with ImGui::PushID; include the necessary headers
(<cstdio> or <string>/<format>) and ensure no buffer overflow risk.
| char buffer[256]; | ||
| ::memset(buffer, 0, sizeof(buffer)); | ||
| ::strncpy_s(buffer, sizeof(buffer), tag.c_str(), sizeof(buffer)); | ||
| if (ImGui::InputText("##Tag", buffer, sizeof(buffer))) | ||
| { | ||
| tag = std::string(buffer); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider modern C++ for tag input (same pattern as line 99-105).
This code repeats the C-style string manipulation pattern used earlier for tag editing, including the Windows-specific strncpy_s. As suggested for lines 99-105, consider using ImGui's std::string overload for better portability and modern C++ style.
🔎 Proposed refactor
- char buffer[256];
- ::memset(buffer, 0, sizeof(buffer));
- ::strncpy_s(buffer, sizeof(buffer), tag.c_str(), sizeof(buffer));
- if (ImGui::InputText("##Tag", buffer, sizeof(buffer)))
+ if (ImGui::InputText("##Tag", &tag))
{
- tag = std::string(buffer);
+ // tag is already updated
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 262-262: do not declare C-style arrays, use std::array<> instead
(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
[warning] 262-262: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In OloEditor/src/Panels/SceneHierarchyPanel.cpp around lines 262 to 268, the
code uses a C-style fixed buffer and strncpy_s to edit tag text; replace that
pattern with ImGui's std::string overload to avoid manual buffer management and
Windows-specific APIs. Remove the char buffer, memset and strncpy_s calls and
call ImGui::InputText("##Tag", &tag) (or InputTextWithHint if a hint is needed),
and ensure imgui_stdlib (or your project's ImGui string helpers) is included so
the std::string overload is available.
| // Ensure we're working with valid handles | ||
| if (handle == 0 || GetHandle() == 0) | ||
| { | ||
| OLO_CORE_WARN("AnimationAsset::OnDependencyUpdated - Invalid handle(s): dependency={}, self={}", | ||
| static_cast<u64>(handle), static_cast<u64>(GetHandle())); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a typed invalid handle constant.
Comparing AssetHandle directly to 0 works but is less expressive. If AssetHandle has a named invalid constant (e.g., AssetHandle::Invalid or InvalidAssetHandle), prefer using that for clarity and type safety.
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Animation/AnimationAsset.cpp around lines 30 to 36,
the code checks AssetHandle values against literal 0 which is unclear; replace
comparisons to use the project's typed invalid-handle constant (e.g.,
AssetHandle::Invalid or InvalidAssetHandle) instead of 0 to improve
expressiveness and type-safety. Update both checks and any casts used in the log
to use the proper invalid constant and, if necessary, include the header or
namespace where that constant is defined so the file compiles.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
OloEngine/src/OloEngine/Algo/BinaryHeap.h (1)
200-200: Remove unnecessary explicit cast (duplicate issue).The explicit cast
(IndexType)0is unnecessary since the literal0implicitly converts to any numeric type. This issue was previously flagged and marked as addressed, but the cast remains in the code.🔎 Proposed simplification
- HeapSiftDown(First, (IndexType)0, Index, Proj, ReversePredicateWrapper); + HeapSiftDown(First, 0, Index, Proj, ReversePredicateWrapper);
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
OloEngine/src/OloEngine/Algo/BinaryHeap.h(2 hunks)OloEngine/src/OloEngine/Async/QueuedThreadPool.h(1 hunks)OloEngine/src/OloEngine/Audio/LockFreeEventQueue.h(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Use modern C++20 baseline (aiming for C++23 or C++26 features)
Use PascalCase for class names
Use m_PascalCase for member variables
Use s_PascalCase for static member variables
Use Ref smart pointer from Core/Ref.h instead of raw pointers
Include what you use: project headers in quotes, third-party/system headers in angle brackets
Place braces on new lines except for trivial cases
Add ECS components via entity.AddComponent() pattern
Use AssetManager::LoadAssetFromFile() to load assets and GetAsset() to retrieve typed assets
Use scene->GetPhysicsScene() to access 3D physics (Jolt)
Use Box2D for 2D physics with custom collision layers
Files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Algo/BinaryHeap.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
**/*.{cpp,h,hpp,cs,lua}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 4-space indentation for code
Files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Algo/BinaryHeap.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
**/*.{h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use #pragma once for header guards
Files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Algo/BinaryHeap.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
OloEngine/src/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement ECS systems using EnTT with Entity wrapper (UUID)
Files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Algo/BinaryHeap.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: When porting UE5 task system, port `FTask`, `FTaskEvent`, `TTask<T>` types as needed for OloEngine
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 types `int64`, `int32`, `int16`, `int8` with `i64`, `i32`, `i16`, `i8` respectively
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 types `uint64`, `uint32`, `uint16`, `uint8` with `u64`, `u32`, `u16`, `u8` respectively
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 `FThreadSafeCounter` with `std::atomic<i32>`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 `FThreadSafeCounter64` with `std::atomic<i64>`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 type `SIZE_T` with `sizet`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `UE_LOG(Category, Level, ...)` with appropriate OloEngine logging macro `OLO_CORE_INFO/WARN/ERROR(...)`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 type `TCHAR*` with `char*`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `UE_BUILD_DEVELOPMENT` with `OLO_RELEASE`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `UE_BUILD_DEBUG` with `OLO_DEBUG`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Replace deprecated `FThreadSafeCounter64` with `std::atomic<i64>` and translate method calls: `Increment()` → `++atomic`, `Decrement()` → `--atomic`, `Add(n)` → `atomic.fetch_add(n)`, `Subtract(n)` → `atomic.fetch_sub(n)`, `Set(v)` → `atomic.exchange(v)`, `Reset()` → `atomic.exchange(0)`, `GetValue()` → `atomic.load()`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Skip or stub out UE5 AutoRTFM (transactional memory) features as they are not needed in OloEngine
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Replace deprecated `FThreadSafeCounter` with `std::atomic<i32>` and translate method calls: `Increment()` → `++atomic`, `Decrement()` → `--atomic`, `Add(n)` → `atomic.fetch_add(n)`, `Subtract(n)` → `atomic.fetch_sub(n)`, `Set(v)` → `atomic.exchange(v)`, `Reset()` → `atomic.exchange(0)`, `GetValue()` → `atomic.load()`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Add(n)` with `atomic.fetch_add(n)` returning the OLD value
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Place code in `namespace OloEngine`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Increment()` with `++atomic` (or `atomic.fetch_add(1) + 1`) returning the NEW value
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Place all code in `namespace OloEngine`
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: For thread pools during porting, consider using OloEngine's existing threading if available, or port UE5's `FQueuedThreadPool`
📚 Learning: 2025-12-12T14:50:55.336Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Replace deprecated `FThreadSafeCounter` with `std::atomic<i32>` and translate method calls: `Increment()` → `++atomic`, `Decrement()` → `--atomic`, `Add(n)` → `atomic.fetch_add(n)`, `Subtract(n)` → `atomic.fetch_sub(n)`, `Set(v)` → `atomic.exchange(v)`, `Reset()` → `atomic.exchange(0)`, `GetValue()` → `atomic.load()`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Algo/BinaryHeap.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:50:55.336Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Replace deprecated `FThreadSafeCounter64` with `std::atomic<i64>` and translate method calls: `Increment()` → `++atomic`, `Decrement()` → `--atomic`, `Add(n)` → `atomic.fetch_add(n)`, `Subtract(n)` → `atomic.fetch_sub(n)`, `Set(v)` → `atomic.exchange(v)`, `Reset()` → `atomic.exchange(0)`, `GetValue()` → `atomic.load()`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Algo/BinaryHeap.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 `FThreadSafeCounter` with `std::atomic<i32>`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Set(v)` with `atomic.exchange(v)`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Add(n)` with `atomic.fetch_add(n)` returning the OLD value
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::GetValue()` with `atomic.load()`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Reset()` with `atomic.exchange(0)`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 `FThreadSafeCounter64` with `std::atomic<i64>`
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Increment()` with `++atomic` (or `atomic.fetch_add(1) + 1`) returning the NEW value
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Subtract(n)` with `atomic.fetch_sub(n)` returning the OLD value
Applied to files:
OloEngine/src/OloEngine/Audio/LockFreeEventQueue.hOloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: For thread pools during porting, consider using OloEngine's existing threading if available, or port UE5's `FQueuedThreadPool`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Map UE5 task priorities to OloEngine thread priorities
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Place code in `namespace OloEngine`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace `FThreadSafeCounter::Decrement()` with `--atomic` (or `atomic.fetch_sub(1) - 1`) returning the NEW value
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `UE_BUILD_DEBUG` with `OLO_DEBUG`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `RESTRICT` with `OLO_RESTRICT`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `ensure(x)` with `OLO_CORE_VERIFY(x)`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `check(x)` with `OLO_CORE_ASSERT(x)`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.{h,cpp} : Replace UE5 macro `UE_FORCEINLINE` with `OLO_FINLINE`
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: Applies to OloEngine/src/OloEngine/Task/**/*.h : Use `#pragma once` for include guards in C++ header files
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:51:14.998Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/task-system.instructions.md:0-0
Timestamp: 2025-12-12T14:51:14.998Z
Learning: When porting UE5 task system, port `FTask`, `FTaskEvent`, `TTask<T>` types as needed for OloEngine
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
📚 Learning: 2025-12-12T14:50:55.336Z
Learnt from: CR
Repo: drsnuggles8/OloEngineBase PR: 0
File: .github/instructions/memory-system.instructions.md:0-0
Timestamp: 2025-12-12T14:50:55.336Z
Learning: Applies to OloEngine/src/OloEngine/Memory/**/*.{h,hpp,cpp} : Skip, stub, or defer implementation of `IsInGameThread()` checks when porting UE5 Memory code
Applied to files:
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
🧬 Code graph analysis (1)
OloEngine/src/OloEngine/Algo/BinaryHeap.h (5)
OloEngine/src/OloEngine/Templates/Invoke.h (1)
OloEngine(11-26)OloEngine/src/OloEngine/Templates/ReversePredicate.h (1)
OloEngine(11-33)OloEngine/src/OloEngine/Algo/HeapSort.h (1)
OloEngine(15-40)OloEngine/src/OloEngine/Algo/IntroSort.h (1)
OloEngine(20-173)OloEngine/src/OloEngine/Algo/IsHeap.h (1)
OloEngine(16-42)
🪛 Clang (14.0.6)
OloEngine/src/OloEngine/Algo/BinaryHeap.h
[warning] 19-19: variable 'OloEngine' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
OloEngine/src/OloEngine/Async/QueuedThreadPool.h
[error] 12-12: 'OloEngine/Core/Base.h' file not found
(clang-diagnostic-error)
[warning] 27-27: variable 'OLO' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (c-cpp, manual)
- GitHub Check: Analyze (csharp, none)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
OloEngine/src/OloEngine/Async/QueuedThreadPool.h (4)
1-26: LGTM!The header documentation is clear,
#pragma onceis used correctly, and includes follow the project convention (project headers in quotes, system headers in angle brackets). The include path forManualResetEvent.his now correct.Note: The static analysis error about
'OloEngine/Core/Base.h' file not foundis a false positive due to missing build context in the analyzer.
204-265: LGTM!The abstract interface is well-designed with clear documentation, appropriate pure virtual methods, a virtual destructor, and a factory method. The static
OverrideStackSizemember is correctly declared (will need a definition in the.cppfile for ODR compliance).
273-349: LGTM with minor suggestion.The
FQueuedThreadPoolSchedulerimplementation is well-structured:
- Correctly marked
finalfor optimization- Properly deletes copy/move operations
- Uses appropriate atomic members for thread safety
- Clean separation between public API and private implementation details
Consider adding
noexceptto the destructor for clarity, though it's implicitlynoexceptwhen the base destructor is:~FQueuedThreadPoolScheduler() noexcept override;
351-355: LGTM!The global thread pool declarations follow the standard extern pattern for engine-wide singletons. The comment appropriately documents that initialization occurs during engine startup.
OloEngine/src/OloEngine/Algo/BinaryHeap.h (3)
31-55: Excellent overflow protection implementation.The overflow-safe left-child index calculation properly addresses the concern raised in the previous review. The sentinel value approach is well-documented and safely handled by callers—HeapIsLeaf implicitly treats overflow as "is leaf," causing HeapSiftDown to exit early, which is correct behavior.
69-131: Well-refactored sift operations.The refactored HeapSiftDown and HeapSiftUp implementations are cleaner and more efficient. The single-swap pattern in HeapSiftDown and the simplified bubble-up logic in HeapSiftUp improve both readability and maintainability.
133-170: Robust heapify implementation with clear documentation.The explicit signed/unsigned handling with
if constexprand the improved documentation that clarifies how the predicate determines heap type (min vs max) are excellent improvements. The early guards and assertions enhance robustness.OloEngine/src/OloEngine/Audio/LockFreeEventQueue.h (5)
43-52: LGTM: Copy assignment is correctly implemented.The self-assignment check and full member-wise copy (m_Type, m_DataSize, m_Storage) align with the copy constructor behavior. The fixed-size memcpy enables efficient compiler optimization.
119-122: LGTM: Copy constructor correctly uses member initializer list.The refactored syntax is more idiomatic and maintains correct copy semantics for all members.
152-170: LGTM: Copy operations are optimized correctly.The fixed-size memcpy approach (avoiding strlen) is an appropriate performance optimization for the real-time audio thread, as noted in the comments. Both copy constructor and assignment operator are implemented correctly.
248-251: Excellent: Critical memory ordering issue resolved.The change from
memory_order_relaxedtomemory_order_releaseat line 251 correctly addresses the critical data race identified in the previous review. This release store now properly synchronizes with the producer's acquire load at line 219, establishing the necessary happens-before relationship that prevents the race condition when the producer wraps around and writes to a previously consumed slot.The added documentation at lines 248-249 clearly explains this synchronization requirement.
196-289: LGTM: Lock-free SPSC queue is well-implemented.The implementation correctly achieves real-time safety with:
- Wait-free Push/Pop operations with no allocations or locks
- Proper cache line padding (alignas(64)) to prevent false sharing
- Correct ring buffer indexing for power-of-2 capacity
- Clear documentation of safety constraints (e.g., Clear requires no concurrent access)
The static assertions, approximate query methods, and overall structure are sound.
| inline const char* ToString(EQueuedWorkPriority priority) | ||
| { | ||
| switch (priority) | ||
| { | ||
| case EQueuedWorkPriority::Blocking: | ||
| return "Blocking"; | ||
| case EQueuedWorkPriority::Highest: | ||
| return "Highest"; | ||
| case EQueuedWorkPriority::High: | ||
| return "High"; | ||
| case EQueuedWorkPriority::Normal: | ||
| return "Normal"; | ||
| case EQueuedWorkPriority::Low: | ||
| return "Low"; | ||
| case EQueuedWorkPriority::Lowest: | ||
| return "Lowest"; | ||
| default: | ||
| return "Unknown"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making ToString constexpr for compile-time evaluation.
The function is a simple switch on an enum, making it a good candidate for constexpr. This would allow compile-time string selection when the priority is known at compile time.
Note: The static analysis warning about OLO being a "non-const global variable" is a false positive—it's a namespace, not a variable.
🔎 Proposed fix
- inline const char* ToString(EQueuedWorkPriority priority)
+ constexpr const char* ToString(EQueuedWorkPriority priority)📝 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.
| inline const char* ToString(EQueuedWorkPriority priority) | |
| { | |
| switch (priority) | |
| { | |
| case EQueuedWorkPriority::Blocking: | |
| return "Blocking"; | |
| case EQueuedWorkPriority::Highest: | |
| return "Highest"; | |
| case EQueuedWorkPriority::High: | |
| return "High"; | |
| case EQueuedWorkPriority::Normal: | |
| return "Normal"; | |
| case EQueuedWorkPriority::Low: | |
| return "Low"; | |
| case EQueuedWorkPriority::Lowest: | |
| return "Lowest"; | |
| default: | |
| return "Unknown"; | |
| } | |
| } | |
| constexpr const char* ToString(EQueuedWorkPriority priority) | |
| { | |
| switch (priority) | |
| { | |
| case EQueuedWorkPriority::Blocking: | |
| return "Blocking"; | |
| case EQueuedWorkPriority::Highest: | |
| return "Highest"; | |
| case EQueuedWorkPriority::High: | |
| return "High"; | |
| case EQueuedWorkPriority::Normal: | |
| return "Normal"; | |
| case EQueuedWorkPriority::Low: | |
| return "Low"; | |
| case EQueuedWorkPriority::Lowest: | |
| return "Lowest"; | |
| default: | |
| return "Unknown"; | |
| } | |
| } |
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Async/QueuedThreadPool.h around lines 41 to 60, the
ToString(EQueuedWorkPriority) function should be made constexpr so the compiler
can evaluate the switch at compile time when given a constexpr priority; change
the declaration to a constexpr function (e.g., inline constexpr const char*
ToString(...)) and ensure the body remains a constexpr-friendly switch returning
string literals (optionally mark noexcept), then rebuild to confirm no warnings
(the static analyzer warning about OLO is a false positive and can be ignored).
| void UpdateFirstNonEmptyIndex() | ||
| { | ||
| for (size_t i = 0; i < static_cast<size_t>(EQueuedWorkPriority::Count); ++i) | ||
| { | ||
| if (!m_PriorityQueues[i].IsEmpty()) | ||
| { | ||
| m_FirstNonEmptyQueueIndex = i; | ||
| return; | ||
| } | ||
| } | ||
| m_FirstNonEmptyQueueIndex = static_cast<size_t>(EQueuedWorkPriority::Count); | ||
| } | ||
|
|
||
| size_t m_FirstNonEmptyQueueIndex = 0; | ||
| std::array<TArray<IQueuedWork*>, static_cast<size_t>(EQueuedWorkPriority::Count)> m_PriorityQueues; | ||
| std::atomic<i32> m_NumQueuedWork{ 0 }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent initial state for m_FirstNonEmptyQueueIndex.
The member is default-initialized to 0 (line 199), but UpdateFirstNonEmptyIndex() sets it to Count when all queues are empty (line 196). While this doesn't cause bugs (Dequeue/Peek verify emptiness), initializing to Count would be more consistent with the semantic meaning "no non-empty queue exists."
🔎 Proposed fix
- size_t m_FirstNonEmptyQueueIndex = 0;
+ size_t m_FirstNonEmptyQueueIndex = static_cast<size_t>(EQueuedWorkPriority::Count);📝 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.
| void UpdateFirstNonEmptyIndex() | |
| { | |
| for (size_t i = 0; i < static_cast<size_t>(EQueuedWorkPriority::Count); ++i) | |
| { | |
| if (!m_PriorityQueues[i].IsEmpty()) | |
| { | |
| m_FirstNonEmptyQueueIndex = i; | |
| return; | |
| } | |
| } | |
| m_FirstNonEmptyQueueIndex = static_cast<size_t>(EQueuedWorkPriority::Count); | |
| } | |
| size_t m_FirstNonEmptyQueueIndex = 0; | |
| std::array<TArray<IQueuedWork*>, static_cast<size_t>(EQueuedWorkPriority::Count)> m_PriorityQueues; | |
| std::atomic<i32> m_NumQueuedWork{ 0 }; | |
| void UpdateFirstNonEmptyIndex() | |
| { | |
| for (size_t i = 0; i < static_cast<size_t>(EQueuedWorkPriority::Count); ++i) | |
| { | |
| if (!m_PriorityQueues[i].IsEmpty()) | |
| { | |
| m_FirstNonEmptyQueueIndex = i; | |
| return; | |
| } | |
| } | |
| m_FirstNonEmptyQueueIndex = static_cast<size_t>(EQueuedWorkPriority::Count); | |
| } | |
| size_t m_FirstNonEmptyQueueIndex = static_cast<size_t>(EQueuedWorkPriority::Count); | |
| std::array<TArray<IQueuedWork*>, static_cast<size_t>(EQueuedWorkPriority::Count)> m_PriorityQueues; | |
| std::atomic<i32> m_NumQueuedWork{ 0 }; |
🤖 Prompt for AI Agents
In OloEngine/src/OloEngine/Async/QueuedThreadPool.h around lines 186 to 201,
m_FirstNonEmptyQueueIndex is default-initialized to 0 but
UpdateFirstNonEmptyIndex() uses Count to represent "no non-empty queue"; change
the initializer to static_cast<size_t>(EQueuedWorkPriority::Count) so the
member's initial state matches the empty-queue sentinel, keeping the same cast
style used elsewhere.
Complete port of Unreal Engine 5.7.1 low-level and high-level task system with all supporting infrastructure:
Task System (LowLevelTasks namespace)
Task System (Tasks namespace - high-level API)
Threading Primitives
Supporting Infrastructure
Changes from UE5.7
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.