fix(graphic): remove memory leaks#429
Conversation
…nto PluginCameraMovement
…ssues and free capabilities if allocated
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR introduces a new ResetCameraBehavior system for camera management, refactors multiple buffer classes in the default pipeline to use explicit destructors with centralized Destroy() methods, adds move semantics and RAII improvements to graphics resources (BindGroup and Shader), updates bind group ownership transfers to use std::move, and enhances resource cleanup in graphics system initialization and presentation phases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
…tructor and assignment operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/plugin/camera-movement/src/system/ResetCameraBehavior.hpp`:
- Around line 7-12: The comment for ResetCameraBehavior.hpp is misleading: it
describes input-triggered behavior but the system is actually registered to run
on Shutdown; update the docblock for the ResetCameraBehavior system to state
that it runs during the engine Shutdown phase (not on input events), describe
what it does at shutdown (resets camera behavior to default), and remove or
adjust any references to input-triggered resets so the lifecycle matches the
registration of ResetCameraBehavior.
In `@src/plugin/graphic/src/resource/BindGroup.hpp`:
- Around line 40-52: The class BindGroup defines a move constructor and
destructor but lacks a move assignment operator; add a noexcept move assignment
BindGroup& operator=(BindGroup&& other) that first releases this->_bindGroup if
non-null, then moves/transfers all members (_shaderId, _layoutIndex, _assets,
_entries, _bindGroup) from other (use std::move for containers/holders), sets
other._bindGroup = nullptr, and returns *this; ensure semantics mirror the
existing move constructor and keep copy operations deleted as currently
declared.
In `@src/plugin/graphic/src/resource/Shader.hpp`:
- Around line 25-30: The Shader move constructor currently copies descriptor;
change it to move descriptor to transfer ownership: in Shader(Shader &&other)
noexcept use descriptor = std::move(other.descriptor) (leave pipeline transfer
as pipeline = other.pipeline; other.pipeline = nullptr) so strings/vectors
inside ShaderDescriptor are moved instead of copied; ensure <utility> is
included if not already.
- Around line 31-40: The Shader::operator=(Shader&& other) currently overwrites
descriptor and pipeline without releasing the existing pipeline, causing a
resource leak; update Shader::operator=(Shader&&) to first check and release or
destroy the current pipeline owned by *this (matching the class's pipeline
cleanup pattern), then move-assign descriptor using std::move(descriptor) and
move the pipeline pointer from other to this, finally nulling other.pipeline;
follow the same move-semantics used in Texture::operator= and Sampler::operator=
to ensure proper ownership transfer and no leaks.
In `@src/plugin/graphic/src/resource/Surface.hpp`:
- Around line 15-26: The updateCapabilities method dereferences the optional
member value via value->getCapabilities(...) without checking value.has_value();
add a guard at the start of updateCapabilities to verify value.has_value()
(similar to Release()), and if it is not set return an appropriate wgpu::Status
error (or construct a failed Status) instead of dereferencing; this protects the
call to value->getCapabilities and prevents undefined behavior when value is
std::nullopt, while leaving the rest of the logic (freeMembers on existing
capabilities, assigning capabilities = caps, and returning status) unchanged.
In `@src/plugin/graphic/src/system/initialization/ConfigureSurface.cpp`:
- Around line 35-45: The call to AdapterInfo::freeMembers() must only happen if
context.adapter->getInfo(&info) returned wgpu::Status::Success; change
ConfigureSurface.cpp so that freeMembers() is invoked only inside the success
path (or use a bool success flag set when getInfo(...) == wgpu::Status::Success)
and remove the unconditional freeMembers() at the end; reference AdapterInfo,
context.adapter->getInfo, and freeMembers to locate and adjust the logic
accordingly.
In `@src/plugin/graphic/src/system/initialization/CreateAdapter.cpp`:
- Around line 39-47: The cleanup currently calls adapters.data()->release()
which only releases the first adapter, leaking others; update the cleanup logic
in the CreateAdapter code path so that before returning cand you iterate over
the adapters container and call release() on every adapter element except the
one you are returning (cand), and also ensure info.freeMembers() is still called
for each adapter's info; reference adapters (the vector), cand (the chosen
adapter), info.freeMembers(), and release() to locate and implement the loop
that releases all non-returned adapters.
In `@src/plugin/graphic/src/system/presentation/Present.cpp`:
- Around line 14-18: Add a defensive existence check before calling Get on the
texture container: use
core.GetResource<Resource::TextureContainer>().Contains(Utils::END_RENDER_TEXTURE_ID)
and only call Get(...).TakeOwnership() and Remove(...) if Contains returns true;
otherwise skip those calls to avoid throwing ResourceManagerError. Update the
block that currently uses
textureContainer.Get(Utils::END_RENDER_TEXTURE_ID).TakeOwnership() and
textureContainer.Remove(Utils::END_RENDER_TEXTURE_ID) to guard both operations
with the Contains check.
🧹 Nitpick comments (4)
src/plugin/graphic/src/resource/BindGroup.hpp (1)
84-89: Potential double-release if_CreateBindGroupthrows.In
_RefreshBindGroup, if_CreateBindGroupsucceeds but the assignment somehow fails (unlikely but possible with exceptions), or if future code changes introduce exceptions, the old_bindGroupwould already be released. Consider using a safer swap pattern.♻️ Safer pattern using swap
void _RefreshBindGroup(Engine::Core &core) { auto newBindGroup = _CreateBindGroup(core); - _bindGroup.release(); - _bindGroup = newBindGroup; + std::swap(_bindGroup, newBindGroup); + if (newBindGroup) + newBindGroup.release(); }src/plugin/graphic/src/resource/Shader.hpp (2)
16-23: Minor: Unnecessarynullptrassignment in destructor.Setting
pipeline = nullptrafterrelease()in the destructor is redundant since the object is being destroyed immediately after. This doesn't affect correctness but is unnecessary code.Suggested simplification
virtual ~Shader() { if (pipeline != nullptr) { pipeline.release(); - pipeline = nullptr; } }
127-131: Acknowledge TODO:GetBindGroupLayoutreturns unmanaged resource.The returned
wgpu::BindGroupLayoutfrompipeline.getBindGroupLayout()may need explicit release by callers, potentially causing leaks if not handled properly. The TODO correctly identifies the need for an RAII wrapper.Would you like me to open an issue to track implementing an RAII wrapper class for
BindGroupLayoutmanagement, or help draft the implementation?src/plugin/camera-movement/src/system/ResetCameraBehavior.cpp (1)
1-2: Avoid transitive include coupling.
Add a direct include forresource/CameraManager.hppto keep this TU robust ifcore/Core.hppstops pulling it in.♻️ Suggested include
`#include` "core/Core.hpp" +#include "resource/CameraManager.hpp" `#include` "utils/DontMoveBehavior.hpp"
…ent to prevent errors
|
Divengerss
left a comment
Divengerss
left a comment
There was a problem hiding this comment.
I've ran a local valgrind check and most of the memory leaks have been fixed. They are still 152 bytes leaked, but they appear to be from Mesa/XCB and wgpu internal calls used by the engine, meaning it's not from our implementation.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.