Open
Conversation
WalkthroughIntegrates Flecs ECS into Resource and ResourceManager: Resources gain a root Flecs entity and OwnedResource component; ResourceManager creates a shared "Resources" root and parents resource roots to it; child entities are deferred-destroyed when resources stop or on teardown. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ResMgr as ResourceManager
participant World as FlecsWorld
participant Resource
participant Flecs as FlecsRuntime
Client->>ResMgr: Init(jsEngine, world, config)
ResMgr->>World: entity("Resources")
World-->>ResMgr: _rootEntity
Client->>ResMgr: DiscoverResource(manifest)
ResMgr->>Resource: new Resource(path, world)
Resource->>World: entity(manifest_name)
World-->>Resource: _rootEntity
Resource->>Flecs: add OwnedResource(component -> Resource*)
Resource-->>ResMgr: GetRootEntity()
ResMgr->>Flecs: child_of(resource._rootEntity, ResMgr._rootEntity)
Client->>Resource: TransitionTo(Stopped)
Resource->>Resource: DestroyChildEntities()
Resource->>Flecs: defer destroy(child entities)
Flecs->>Flecs: execute deferred destruction
Client->>ResMgr: Destroy ResourceManager
ResMgr->>Flecs: destruct ResMgr._rootEntity
Flecs->>Flecs: cascade destroy resource root entities
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/scripting/resource/resource.cpp (1)
44-57:⚠️ Potential issue | 🟠 MajorTransfer
_rootEntityownership when movingResource.Lines 53 and 71-76 copy the Flecs handle instead of transferring it. That leaves
OwnedResourcepointing at the moved-from object, the move ctor keepsother._rootEntitylive so the moved-from destructor can delete the same entity, and move-assignment overwrites any existing root without cleaning it up. Either delete the move operations or fully transfer/rebind the handle.Also applies to: 59-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/framework/src/scripting/resource/resource.cpp` around lines 44 - 57, The move constructor and move-assignment for Resource currently copy the Flecs entity handle (_rootEntity) leaving the moved-from object owning the same entity; update Resource::Resource(Resource&&) to transfer ownership by rebind/steal the Flecs handle into this->_rootEntity and then reset other._rootEntity to an empty/null/invalid state so the moved-from destructor won't destroy it, and update Resource::operator=(Resource&&) to first clean up/destroy any existing _rootEntity owned by *this (matching OwnedResource destructor semantics) before stealing the source's handle and nulling other._rootEntity; alternatively, if transfer is not desired, delete the move ctor/assignment to avoid double-destroy—make changes around _rootEntity, Resource::Resource(Resource&&), Resource::operator=(Resource&&) and OwnedResource cleanup to ensure exclusive ownership is maintained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/framework/src/scripting/resource/resource_manager.cpp`:
- Around line 44-46: Check for null before dereferencing
CoreModules::GetWorldEngine() and its GetWorld() result when initializing
_rootEntity in ResourceManager; if either is null, avoid calling
entity("Resources") (set _rootEntity to a safe null/invalid state) and defer
root creation until the world is available (e.g., add a lazy init or a BuildRoot
method invoked after module registration). Update the constructor/site that
calls _rootEntity =
CoreModules::GetWorldEngine()->GetWorld()->entity("Resources") to guard using
CoreModules::GetWorldEngine() and CoreModules::GetWorldEngine()->GetWorld() and
ensure ResourceManager later attempts to create/attach the "Resources" root when
those getters return valid pointers.
In `@code/framework/src/scripting/resource/resource.cpp`:
- Around line 299-301: The code only calls DestroyChildEntities() when newState
== ResourceState::Stopped, which leaves child entities alive on error/restart
paths; update state-transition handling in the method that sets resource state
(the block around ResourceState changes) so that DestroyChildEntities() is also
invoked when transitioning to ResourceState::Error or during restart/failure
flows (ensure it runs when StartResource()/SetError() or
HandleResourceRuntimeError()/RestartResource() cause a non-running state), and
ensure StopResource()/IsRunning() logic does not early-return before child
cleanup; locate and modify the state-change logic and calls to
DestroyChildEntities(), StartResource, SetError, HandleResourceRuntimeError,
RestartResource, StopResource, and IsRunning to guarantee children are destroyed
on error and before restart.
- Around line 32-33: Validate the ECS world pointers before creating the root
entity: check that CoreModules::GetWorldEngine() returns non-null and that
GetWorld() returns non-null before calling GetWorld()->entity(...). If either is
null, avoid dereferencing, do not set _rootEntity or OwnedResource, and instead
handle the failure cleanly (e.g., set _rootEntity to an empty/invalid entity,
return/propagate an error state, and/or log a descriptive message referencing
_manifest.GetName()). Update the code around _rootEntity,
CoreModules::GetWorldEngine(), GetWorld(), _manifest, and OwnedResource so null
pointers are guarded and resource discovery cannot crash during early
initialization.
---
Outside diff comments:
In `@code/framework/src/scripting/resource/resource.cpp`:
- Around line 44-57: The move constructor and move-assignment for Resource
currently copy the Flecs entity handle (_rootEntity) leaving the moved-from
object owning the same entity; update Resource::Resource(Resource&&) to transfer
ownership by rebind/steal the Flecs handle into this->_rootEntity and then reset
other._rootEntity to an empty/null/invalid state so the moved-from destructor
won't destroy it, and update Resource::operator=(Resource&&) to first clean
up/destroy any existing _rootEntity owned by *this (matching OwnedResource
destructor semantics) before stealing the source's handle and nulling
other._rootEntity; alternatively, if transfer is not desired, delete the move
ctor/assignment to avoid double-destroy—make changes around _rootEntity,
Resource::Resource(Resource&&), Resource::operator=(Resource&&) and
OwnedResource cleanup to ensure exclusive ownership is maintained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dce8884-857b-4bee-acaf-b575549a52bd
📒 Files selected for processing (4)
code/framework/src/scripting/resource/resource.cppcode/framework/src/scripting/resource/resource.hcode/framework/src/scripting/resource/resource_manager.cppcode/framework/src/scripting/resource/resource_manager.h
Comment on lines
+44
to
46
|
|
||
| _rootEntity = CoreModules::GetWorldEngine()->GetWorld()->entity("Resources"); | ||
| } |
There was a problem hiding this comment.
Validate the ECS world before building the manager root.
Line 45 dereferences CoreModules::GetWorldEngine() and GetWorld() without a guard. Those getters are raw pointers, so constructing ResourceManager before module registration turns this into a null dereference, and later discovery will try to parent resources into a missing root.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/framework/src/scripting/resource/resource_manager.cpp` around lines 44 -
46, Check for null before dereferencing CoreModules::GetWorldEngine() and its
GetWorld() result when initializing _rootEntity in ResourceManager; if either is
null, avoid calling entity("Resources") (set _rootEntity to a safe null/invalid
state) and defer root creation until the world is available (e.g., add a lazy
init or a BuildRoot method invoked after module registration). Update the
constructor/site that calls _rootEntity =
CoreModules::GetWorldEngine()->GetWorld()->entity("Resources") to guard using
CoreModules::GetWorldEngine() and CoreModules::GetWorldEngine()->GetWorld() and
ensure ResourceManager later attempts to create/attach the "Resources" root when
those getters return valid pointers.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/scripting/resource/resource.cpp (1)
43-76:⚠️ Potential issue | 🔴 CriticalFix root-entity ownership transfer in the move operations.
_rootEntityis copied here, but the entity still points atotherviaOwnedResource, the move ctor leavesother._rootEntitylive, and move assignment overwrites an existing root without destroying it first. That lets a moved-fromResourcetear down the moved-to root and leaks the old target root.Possible fix
Resource::Resource(Resource &&other) noexcept : _path(std::move(other._path)) , _manifest(std::move(other._manifest)) , _manifestValid(other._manifestValid) , _state(other._state) , _errorMessage(std::move(other._errorMessage)) , _stateTimestamp(other._stateTimestamp) , _loadTimestamp(other._loadTimestamp) , _isolate(other._isolate) , _rootEntity(other._rootEntity) , _exports(std::move(other._exports)) , _restartAttempts(std::move(other._restartAttempts)) { + if (_rootEntity.is_valid()) { + _rootEntity.set<OwnedResource>({this}); + } other._isolate = nullptr; + other._rootEntity = {}; } Resource &Resource::operator=(Resource &&other) noexcept { if (this != &other) { + if (_rootEntity.is_valid()) { + _rootEntity.destruct(); + } ClearExports(); _path = std::move(other._path); _manifest = std::move(other._manifest); _manifestValid = other._manifestValid; _state = other._state; _errorMessage = std::move(other._errorMessage); _stateTimestamp = other._stateTimestamp; _loadTimestamp = other._loadTimestamp; _isolate = other._isolate; _rootEntity = other._rootEntity; _exports = std::move(other._exports); _restartAttempts = std::move(other._restartAttempts); + if (_rootEntity.is_valid()) { + _rootEntity.set<OwnedResource>({this}); + } other._isolate = nullptr; other._rootEntity = {}; } return *this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/framework/src/scripting/resource/resource.cpp` around lines 43 - 76, The move ctor and move-assignment are copying _rootEntity instead of transferring ownership, leaving other._rootEntity live and risking double-destroy/leak; change the move ctor (Resource::Resource(Resource&&)) to move the root (use _rootEntity(std::move(other._rootEntity)) in the initializer list) and then clear other._rootEntity (set to {}), and in Resource::operator=(Resource&&) ensure you destroy/reset the existing root before overwriting (call the existing root teardown helper or reset _rootEntity) and then assign _rootEntity = std::move(other._rootEntity) and clear other._rootEntity = {}; ensure ownership semantics align with OwnedResource so the moved-from Resource no longer references the entity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/framework/src/scripting/resource/resource.cpp`:
- Around line 298-300: The code currently calls DestroyChildEntities() whenever
newState == ResourceState::Stopped || newState == ResourceState::Error, which
causes child ECS entities to be destroyed even for non-terminal Error
transitions (SetError() is invoked earlier and errorBehavior can be "continue").
Update the condition in the state-change handling (the block using newState and
calling DestroyChildEntities()) so that child cleanup only happens for terminal
stop/restart paths (e.g., ResourceState::Stopped and any explicit terminal
restart state) and not for ResourceState::Error; keep SetError() and
errorBehavior logic intact so error transitions that should continue do not
trigger DestroyChildEntities(). Ensure you reference DestroyChildEntities(),
ResourceState::Stopped, ResourceState::Error and SetError() when making the
change.
---
Outside diff comments:
In `@code/framework/src/scripting/resource/resource.cpp`:
- Around line 43-76: The move ctor and move-assignment are copying _rootEntity
instead of transferring ownership, leaving other._rootEntity live and risking
double-destroy/leak; change the move ctor (Resource::Resource(Resource&&)) to
move the root (use _rootEntity(std::move(other._rootEntity)) in the initializer
list) and then clear other._rootEntity (set to {}), and in
Resource::operator=(Resource&&) ensure you destroy/reset the existing root
before overwriting (call the existing root teardown helper or reset _rootEntity)
and then assign _rootEntity = std::move(other._rootEntity) and clear
other._rootEntity = {}; ensure ownership semantics align with OwnedResource so
the moved-from Resource no longer references the entity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 562c1d79-7cc3-4958-9955-9f367610becf
📒 Files selected for processing (10)
code/framework/src/integrations/client/scripting/module.cppcode/framework/src/integrations/server/scripting/module.cppcode/framework/src/scripting/resource/resource.cppcode/framework/src/scripting/resource/resource.hcode/framework/src/scripting/resource/resource_manager.cppcode/framework/src/scripting/resource/resource_manager.hcode/tests/modules/js_features_ut.hcode/tests/modules/resource_manager_ut.hcode/tests/modules/resource_ut.hcode/tests/modules/timer_context_ut.h
🚧 Files skipped from review as they are similar to previous changes (2)
- code/framework/src/scripting/resource/resource_manager.h
- code/framework/src/scripting/resource/resource_manager.cpp
Comment on lines
+298
to
+300
| if (newState == ResourceState::Stopped || newState == ResourceState::Error) { | ||
| DestroyChildEntities(); | ||
| } |
There was a problem hiding this comment.
Don't destroy children on every Error transition.
Lines 720-732 of code/framework/src/scripting/resource/resource_manager.cpp call SetError() before applying errorBehavior. With errorBehavior == "continue", this branch now wipes the resource's ECS subtree even though the resource is intentionally left running. Child cleanup needs to stay on terminal stop/restart paths instead of being tied directly to ResourceState::Error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/framework/src/scripting/resource/resource.cpp` around lines 298 - 300,
The code currently calls DestroyChildEntities() whenever newState ==
ResourceState::Stopped || newState == ResourceState::Error, which causes child
ECS entities to be destroyed even for non-terminal Error transitions (SetError()
is invoked earlier and errorBehavior can be "continue"). Update the condition in
the state-change handling (the block using newState and calling
DestroyChildEntities()) so that child cleanup only happens for terminal
stop/restart paths (e.g., ResourceState::Stopped and any explicit terminal
restart state) and not for ResourceState::Error; keep SetError() and
errorBehavior logic intact so error transitions that should continue do not
trigger DestroyChildEntities(). Ensure you reference DestroyChildEntities(),
ResourceState::Stopped, ResourceState::Error and SetError() when making the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds to each resource a root entity which can be used as an handle for all entities created for given resource. When entity get added as a child of resource its lifetime is tied to resource lifetime and get automatically removed when resource stopped.
In flecs explorer an example created entity will be visible in hierarchy:
Summary by CodeRabbit
New Features
Refactor
Chores