Actionable comments posted: 5

In @src/plugin/graphic/src/system/initialization/ConfigureSurface.cpp:
- Around line 14-25: The code must guard against null/empty resources and clamp
surface sizes: check that context.surface and context.surface->capabilities are
non-null and that capabilities->formats is non-empty before using formats[0];
verify context.deviceContext.GetDevice().has_value() (or similar) before calling
value(); ensure surfaceSize.x and surfaceSize.y are positive and clamp them to a
safe unsigned range (e.g., >=1 and capped to a max) before assigning to
config.width/config.height to avoid signed→unsigned wrap; if any check fails,
bail out or return a sensible default/failure path instead of dereferencing
nulls.

In @src/plugin/graphic/src/system/preparation/UpdateGPUCameras.cpp:
- Around line 20-24: The code calls gpuCamera.targetTexture.value() without
confirming the optional contains a value; update the conditional to first check
gpuCamera.targetTexture.has_value() (or use if (gpuCamera.targetTexture &&
textureContainer.Contains(...))) before calling .value(), then use that safe
value to call textureContainer.Contains and textureContainer.Get and pass the
resulting texture size to gpuCamera.UpdateAspectRatio; ensure you reference
gpuCamera.targetTexture, textureContainer.Contains, textureContainer.Get, and
gpuCamera.UpdateAspectRatio in the fixed condition so no
std::bad_optional_access can be thrown.

In @src/plugin/window/src/resource/Window.hpp:
- Line 5: Remove the #include "event/OnResize.hpp" from Window.hpp since
Window's public interface does not reference OnResize, and add an explicit
#include "event/OnResize.hpp" to WindowSystem.cpp near the top with the other
includes so WindowSystem.cpp no longer relies on the transitive dependency
through Window.hpp.

src/plugin/window/src/resource/Window.hpp (1)

80-80: LGTM! Making GetSize() const is appropriate.

Making GetSize() a const method is correct if it only queries GLFW state without modifying the Window object's member variables.

The documentation comment "will update the window size" is slightly ambiguous. Consider clarifying to "will query and return the window size" if that's the intent:

📝 Proposed documentation improvement

 /**
  * @brief Get the window size.
  *
- * This function will update the window size from the GLFW window content area and return it.
+ * This function will query the current window size from the GLFW window content area and return it.
  *
  * @return A vector of integers to store the size of the window.
  */

src/plugin/graphic/src/component/GPUCamera.hpp (1)

32-42: Consider simplifying the aspect ratio calculation.

The logic correctly guards against division by zero. However, the aspect ratio calculation could be slightly simplified.

♻️ Minor refactor for cleaner code

     inline void UpdateAspectRatio(const glm::uvec2 &textureSize)
     {
         if (textureSize.y > 0)
         {
-            aspectRatio = static_cast<float>(textureSize.x) / static_cast<float>(textureSize.y);
+            aspectRatio = static_cast<float>(textureSize.x) / textureSize.y;
         }
         else
         {
             Log::Warn("GPUCamera::UpdateAspectRatio: texture height is zero, cannot update aspect ratio.");
         }
     }

The outer cast is sufficient since the division will promote textureSize.y to float automatically.

src/plugin/event/src/utils/EventContainer.hpp (1)

11-26: Core-less Trigger/Call refactor is consistent and keeps EventContainer focused on dispatch.
The std::any_cast<const TEvent&> + iteration over callbacks is straightforward.

Optional: drop Core include if no longer needed here

 #include "FunctionContainer.hpp"
-#include "core/Core.hpp"

src/plugin/event/src/resource/EventManager.hpp (1)

237-237: Verify the purpose of this mutex.

This mutex is declared but not used anywhere in the current implementation. If it was added for future use or a specific synchronization pattern, consider adding a comment explaining its purpose, or remove it if it's not needed.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 2e08ffc and c56ef38.

• examples/graphic_material_usage/xmake.lua
• src/plugin/event/src/resource/EventManager.hpp
• src/plugin/event/src/system/EventSystem.hpp
• src/plugin/event/src/utils/EventContainer.hpp
• src/plugin/event/tests/EventTest.cpp
• src/plugin/graphic/src/Graphic.hpp
• src/plugin/graphic/src/component/GPUCamera.hpp
• src/plugin/graphic/src/plugin/PluginGraphic.cpp
• src/plugin/graphic/src/system/GPUComponentManagement/OnCameraCreation.cpp
• src/plugin/graphic/src/system/initialization/ConfigureSurface.cpp
• src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.cpp
• src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.hpp
• src/plugin/graphic/src/system/preparation/UpdateGPUCameras.cpp
• src/plugin/graphic/xmake.lua
• src/plugin/physics/tests/PhysicsTest.cpp
• src/plugin/window/src/event/OnResize.hpp
• src/plugin/window/src/plugin/PluginWindow.cpp
• src/plugin/window/src/resource/Window.cpp
• src/plugin/window/src/resource/Window.hpp
• src/plugin/window/src/system/WindowSystem.cpp
• src/plugin/window/src/system/WindowSystem.hpp
• src/plugin/window/xmake.lua

• examples/graphic_material_usage/xmake.lua

src/engine/src/plugin/APlugin.hpp (2)

• RegisterSystems (12-15)
• RegisterSystems (12-12)

src/plugin/window/src/system/WindowSystem.cpp (4)

• EnableVSync (13-13)
• EnableVSync (13-13)
• SetupWindowCallbacks (54-65)
• SetupWindowCallbacks (54-54)

src/plugin/window/src/system/WindowSystem.hpp (2)

• EnableVSync (53-53)
• SetupWindowCallbacks (135-135)

src/plugin/graphic/src/system/preparation/UpdateGPUCameras.hpp (1)

• UpdateGPUCameras (7-7)

src/plugin/graphic/src/component/GPUCamera.hpp (2)

• camera (23-30)
• camera (23-23)

src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.cpp (2)

• SetupResizableRenderTexture (11-17)
• SetupResizableRenderTexture (11-11)

src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.hpp (1)

• SetupResizableRenderTexture (7-7)

src/plugin/graphic/src/system/initialization/CreateAmbientLight.hpp (1)

• CreateAmbientLight (7-7)

src/plugin/graphic/src/system/initialization/CreateAmbientLight.cpp (2)

• CreateAmbientLight (10-14)
• CreateAmbientLight (10-10)

src/plugin/graphic/src/system/initialization/CreatePointLights.cpp (2)

• CreatePointLights (10-14)
• CreatePointLights (10-10)

src/plugin/graphic/src/system/initialization/CreatePointLights.hpp (1)

• CreatePointLights (7-7)

src/plugin/event/src/utils/EventContainer.hpp (3)

• event (13-13)
• event (19-26)
• event (19-19)

src/plugin/event/src/utils/EventContainer.hpp (3)

• event (13-13)
• event (19-26)
• event (19-19)

• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Run tests (windows-latest)
• GitHub Check: Run tests (macos-latest)
• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Build and verify binaries (macos-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)

src/plugin/graphic/xmake.lua (1)

6-6: LGTM! Event plugin integration looks correct.

The addition of the event module dependency enables the graphic plugin to handle window resize events, which is essential for the auto-updating render texture feature.

Also applies to: 33-33

src/plugin/graphic/src/Graphic.hpp (1)

102-102: LGTM! Header include is appropriate.

The new include exposes the resizable render texture setup system, which is central to this PR's functionality.

src/plugin/graphic/src/system/GPUComponentManagement/OnCameraCreation.cpp (2)

11-11: LGTM! Include is necessary for END_RENDER_TEXTURE_ID.

This header provides the constant used to assign the camera's target texture.

---

24-24: LGTM! Default target texture assignment enables resize handling.

Setting the camera's target texture to END_RENDER_TEXTURE_ID ensures that cameras render into the resizable render texture, allowing automatic aspect ratio updates during window resize events. This is a sensible default for the main rendering pipeline.

src/plugin/window/src/resource/Window.hpp (2)

83-88: LGTM! Documentation clarity is improved.

The updated documentation for SetSize() is clear and well-formatted.

---

90-93: ToggleFullscreen() implementation is solid.

The method correctly saves windowed state before entering fullscreen mode and restores it when exiting. The implementation includes proper null checking and uses the appropriate GLFW calls to manage window monitor assignment and state transitions.

src/plugin/window/src/plugin/PluginWindow.cpp (1)

15-15: LGTM! Callback registration properly ordered and thread-safe.

SetupWindowCallbacks registered in the Setup phase correctly queues the callback after window creation. The framebuffer size callback safely pushes OnResize events to EventManager via PushEvent(), which is protected by std::scoped_lock on both mutexes for thread safety. Since GLFW callbacks execute on the main thread during PollEvents (PreUpdate phase), no threading issues arise.

src/plugin/window/src/event/OnResize.hpp (1)

12-17: LGTM! Clean event struct design.

The OnResize event structure is well-designed with a clear purpose and minimal surface area. The use of glm::ivec2 for the new size is appropriate for window dimensions.

src/plugin/graphic/src/plugin/PluginGraphic.cpp (2)

5-5: LGTM! Proper event plugin integration.

The addition of Event::Plugin dependency and its corresponding include correctly establishes the plugin dependency chain for event-driven resize handling.

Also applies to: 26-26

---

53-53: LGTM! Appropriate system ordering.

The SetupResizableRenderTexture system is correctly positioned in the RenderingPipeline::Setup phase, ensuring resize event handling is configured before light initialization.

src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.hpp (1)

1-9: LGTM! Clean header declaration.

The header follows standard conventions with appropriate guards and includes.

src/plugin/window/src/system/WindowSystem.cpp (1)

54-65: The initialization order is already guaranteed by design, and EventManager is thread-safe.

The StoreCoreInWindow function is registered in the RenderingPipeline::Init stage and SetupWindowCallbacks is registered in RenderingPipeline::Setup stage. The Init stage always runs before Setup, so the window user pointer is guaranteed to be set before the callbacks are established. No null pointer guard is needed.

Additionally, EventManager is documented and implemented as a thread-safe class with proper mutex protection (_queueMutex, _callbacksMutex) and scoped locks around all critical operations. GLFW callbacks execute on the main thread anyway, so thread-safety is fully handled.

The code is safe as written.

Likely an incorrect or invalid review comment.

src/plugin/window/src/resource/Window.cpp (1)

23-28: LGTM: Appropriate const correctness.

Adding the const qualifier to GetSize() is correct since it doesn't modify object state.

src/plugin/window/xmake.lua (1)

4-4: LGTM: Event dependency properly integrated.

The build configuration correctly adds the Event plugin dependency and exposes event headers for downstream usage.

Also applies to: 13-13, 24-24

src/plugin/event/src/system/EventSystem.hpp (1)

8-12: LGTM: Event processing refactored to remove core parameter.

The change correctly reflects the removal of the Engine::Core parameter from EventManager::ProcessEvents, aligning with the PR's goal to refactor the event callback system.

src/plugin/graphic/src/component/GPUCamera.hpp (1)

3-3: LGTM: Logger include and targetTexture member.

The Logger include is necessary for the new UpdateAspectRatio method, and the targetTexture member is properly initialized and consistent with other ID members.

Also applies to: 21-21

src/plugin/event/tests/EventTest.cpp (2)

52-60: Multi-scheduler flow looks consistent with core-less Trigger/Call (capture-based access).
Manual ProcessEvents<Update>() / ProcessEvents<FixedTimeUpdate>() sequencing reads clearly.

Also applies to: 64-69

---

26-35: The integration test is correct as written. RegisterCallback<TestEvent>() uses the default DirectCallbackSchedulerTag, which executes callbacks synchronously within PushEvent() rather than queuing them. The PushEvent implementation immediately calls registered DirectCallbackSchedulerTag callbacks via callback->Call(event) before returning, so EXPECT_EQ(res.value, 42) will pass reliably.

The multi-scheduler test demonstrates the contrast: callbacks registered with explicit schedulers (e.g., Update, FixedTimeUpdate) are queued and require explicit ProcessEvents<TScheduler>() calls to execute.

Likely an incorrect or invalid review comment.

src/plugin/graphic/src/system/initialization/SetupResizableRenderTexture.cpp (1)

9-17: Ensure resize callback is safe on dispatch thread.
SetupResizableRenderTexture is registered once during plugin initialization, so callback duplication is not a risk. However, the resize callback will execute directly on whatever thread calls PushEvent(OnResize), and ConfigureSurface() accesses GPU resources that typically require device thread affinity. Verify that OnResize fires on the render/device thread, or consider deferring ConfigureSurface() to the correct scheduler.

Minor cleanup (drop unused param)

-static void OnWindowResize(Engine::Core &core, const glm::ivec2 &newSize) { ConfigureSurface(core); }
+static void OnWindowResize(Engine::Core &core) { ConfigureSurface(core); }
@@
     eventManager.RegisterCallback<Window::Event::OnResize>(
-        [&core](const Window::Event::OnResize &event) { OnWindowResize(core, event.newSize); });
+        [&core](const Window::Event::OnResize &) { OnWindowResize(core); });

src/plugin/physics/tests/PhysicsTest.cpp (1)

66-80: No data race concern. Collision events are buffered in worker threads by ContactListenerImpl and flushed on the main thread via ProcessBufferedEvents(). The RegisterCallback() with default DirectCallbackSchedulerTag executes callbacks synchronously on the calling thread (main thread), so collisionAdded and collisionRemoved are safely written and read only on the main thread.

src/plugin/event/src/resource/EventManager.hpp (6)

24-26: LGTM!

The private tag struct pattern is an appropriate way to distinguish direct callback scheduling from other scheduler types.

---

55-60: LGTM!

The move constructor correctly initializes all mutexes and acquires locks on the source object's mutexes before moving the data members.

---

67-78: LGTM!

The move assignment operator correctly handles self-assignment and uses std::scoped_lock with all six mutexes, which employs a deadlock avoidance algorithm.

---

139-171: LGTM!

Good thread-safety pattern: copying the queue under lock and then processing events without holding the lock prevents potential deadlocks from callbacks that might interact with the EventManager.

---

183-201: LGTM!

The default scheduler change is consistent with RegisterCallback, and the implementation properly validates the callback existence before deletion.

---

204-222: LGTM!

Clean implementation with proper mutex usage and efficient try_emplace for container creation.

Actionable comments posted: 1

In @src/plugin/window/src/resource/Window.cpp:
- Around line 23-29: GetSize() currently calls glfwGetWindowSize on _window
without checking for null and can cause undefined behavior; add the same null
check pattern used in Destroy() and ToggleFullscreen(): if _window is null, log
an error (or handle consistently with those methods) and return a safe default
glm::uvec2(0); otherwise call glfwGetWindowSize and return the size. Ensure you
reference the Window::GetSize method and the _window member when implementing
the guard so behavior matches the other methods.

src/plugin/window/src/resource/Window.hpp (1)

72-79: Consider updating documentation to reflect the new return type.

The documentation at line 77 still refers to "A vector of integers" but the return type is now glm::uvec2 (unsigned integers).

📝 Proposed documentation update

     /**
      * @brief Get the window size.
      *
      * This function will update the window size from the GLFW window content area and return it.
      *
-     * @return A vector of integers to store the size of the window.
+     * @return A vector of unsigned integers representing the window's width and height.
      */
     glm::uvec2 GetSize() const;

src/plugin/event/src/resource/EventManager.hpp (1)

126-167: Docs/signature drift in ProcessEvents: remove stale core docs and prefer ProcessEvents() over ProcessEvents(void).
The comment block still references Engine::Core& (Line 133-134), but the function no longer takes it. Also, ProcessEvents(void) is non-idiomatic in C++.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between c56ef38 and 416f84b.

• src/plugin/event/src/resource/EventManager.hpp
• src/plugin/window/src/resource/Window.cpp
• src/plugin/window/src/resource/Window.hpp
• src/plugin/window/src/system/WindowSystem.cpp

• src/plugin/window/src/system/WindowSystem.cpp

src/plugin/window/src/resource/Window.hpp (1)

• _window (59-59)

src/plugin/event/src/utils/EventContainer.hpp (3)

• event (13-13)
• event (19-26)
• event (19-19)

src/plugin/window/src/resource/Window.cpp (1)

23-23: Good addition of const correctness.

Marking GetSize() as const is appropriate since it doesn't modify the window state.

src/plugin/event/src/resource/EventManager.hpp (2)

24-26: Private DirectCallbackSchedulerTag is a good encapsulation point.
Keeping the tag private prevents external code from depending on the “direct scheduler” type.

---

199-218: typeID computed before locking is a nice cleanup.
Keeps type lookup consistent and reduces the locked region slightly.

Actionable comments posted: 2

In @src/plugin/event/src/resource/EventManager.hpp:
- Around line 115-124: The callbacks are being invoked while holding _queueMutex
and _callbacksMutex (via the directEventCallbacks path), risking deadlocks if
callbacks call PushEvent/RegisterCallback/UnregisterCallback; change
EventManager's handling so you copy or move the
std::shared_ptr<Utils::EventContainer<TEvent>> (or its vector of callbacks) out
of _eventCallbacks[typeID] while holding the locks, then release both
_queueMutex and _callbacksMutex before iterating and calling
callback->Call(event) (follow the same lock-release-then-invoke pattern used in
ProcessEvents); ensure you access _eventCallbacks with the same keys
(std::type_index(typeid(DirectCallbackSchedulerTag)) and typeID) and preserve
the container type (Utils::EventContainer<TEvent>) when extracting it.
- Line 88: The template default for TScheduler was changed to
DirectCallbackSchedulerTag which makes callbacks run immediately and breaks
existing callers; restore the original behavior by reverting the template
declaration to use Update as the default TScheduler (e.g., template<typename
TEvent, typename TScheduler = Engine::Scheduler::Update, typename TCallBack>)
and update the documentation comment on UnregisterCallback to reflect the true
default, and if you intentionally keep the new immediate behavior instead,
update all call sites that relied on the old deferred semantics to explicitly
pass Engine::Scheduler::Update and adjust callers in graphics/physics/examples
accordingly.

src/plugin/event/src/resource/EventManager.hpp (1)

183-183: Minor: Variable declarations moved to function start.

The typeID declarations are now at the beginning of their respective functions. Since _GetId<TEvent>() has no side effects, this is purely organizational with no behavioral change.

Also applies to: 204-204

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 416f84b and ee90c32.

• src/plugin/event/src/resource/EventManager.hpp

src/plugin/event/src/utils/EventContainer.hpp (3)

• event (13-13)
• event (19-26)
• event (19-19)

src/plugin/event/src/resource/EventManager.hpp (3)

24-25: LGTM: Tag type for immediate callback dispatch.

The empty tag struct is an appropriate pattern for distinguishing direct callbacks from scheduled ones at compile time.

---

43-47: LGTM: Thread-safe cleanup prevents destruction-time crashes.

The explicit clearing of event callbacks and queue under combined locks properly addresses the window resize crash mentioned in the PR objectives by ensuring clean teardown.

---

136-136: LGTM: Callback signature simplified by removing Engine::Core dependency.

The removal of the Engine::Core parameter streamlines the event callback interface and aligns with the refactored callback signature void(const TEvent&). Note that ProcessEvents correctly invokes callbacks after releasing locks (line 165 is outside the lock scope that ends at line 161), preventing the deadlock issue present in PushEvent.

Also applies to: 165-165

Actionable comments posted: 1

In @src/plugin/event/src/resource/EventManager.hpp:
- Around line 24-26: Defaulting TScheduler to DirectCallbackSchedulerTag changes
semantics so RegisterCallback<TEvent>(...) now invokes callbacks inline in
PushEvent rather than during the scheduler phase; revert the template default
for TScheduler to the prior non-direct scheduler (or remove the default so
callers must opt-in) to preserve existing thread-affinity and reentrancy
behavior, update EventManager's template declaration (the TScheduler default)
accordingly, then audit all RegisterCallback<TEvent> call sites for places that
rely on scheduler-phase execution and add explicit DirectCallbackSchedulerTag
where inline execution is intended; run/adjust unit tests that depend on
scheduler-phase callback ordering.

src/plugin/event/src/resource/EventManager.hpp (1)

137-169: ProcessEvents(void) is a bit unidiomatic, and Trigger concurrency needs a quick sanity check.

• Prefer ProcessEvents() (C++ style) unless there’s a strong convention otherwise.
• Trigger(event) happens outside _callbacksMutex (which is good for avoiding lock-held callbacks), but assumes Utils::EventContainer<TEvent> is safe against concurrent AddFunction/DeleteFunction while triggering. Please confirm the container provides its own synchronization or copy-on-write semantics.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between ee90c32 and a0ba1a4.

• src/plugin/event/src/resource/EventManager.hpp

src/plugin/event/src/utils/EventContainer.hpp (3)

• event (13-13)
• event (19-26)
• event (19-19)

• GitHub Check: Run tests (windows-latest)
• GitHub Check: Run tests (macos-latest)
• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Build and verify binaries (macos-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)
• GitHub Check: Run tests (ubuntu-latest)

src/plugin/event/src/resource/EventManager.hpp (2)

43-48: Destructor lock+clear is fine, but it doesn’t make destruction “thread-safe”.

Clearing under std::scoped_lock is OK, but if other threads can still call into EventManager during teardown, that’s UB regardless. Worth ensuring lifecycle guarantees elsewhere.

---

201-220: typeID hoist in _RegisterCallbackImpl is a nice cleanup.

Moving typeID earlier improves readability and keeps the type lookup consistent.