Resource manager system transition #151
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces direct scripting engine calls with ResourceManager::InvokeGlobalEvent across core builtins; adds RemovedOnResourceReload lifecycle markers for some entities; removes legacy gamemode SDK/config; and introduces new resources freeroam and shared-utils with updated event APIs and exports. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 1
🧹 Nitpick comments (5)
code/server/src/core/builtins/vehicle.cpp (2)
14-24: Consider logging when ResourceManager is unavailable.The null-check provides safety, but silently returning when ResourceManager is unavailable could hide initialization issues. Consider adding a warning log to aid debugging if events fail to dispatch.
🔎 Suggested enhancement with logging
void Vehicle::EventVehiclePlayerEnter(flecs::entity vehicle, flecs::entity player, int seatIndex) { const auto resourceManager = Framework::CoreModules::GetResourceManager(); if (!resourceManager) { + // Log warning that ResourceManager is unavailable + FW_LOG_WARNING("ResourceManager unavailable, event onVehiclePlayerEnter not dispatched"); return; } auto vehicleObj = Vehicle(vehicle); auto playerObj = Player(player); resourceManager->InvokeGlobalEvent("onVehiclePlayerEnter", vehicleObj, playerObj, seatIndex); }
26-36: Apply the same logging recommendation here.Similar to EventVehiclePlayerEnter, consider logging when ResourceManager is unavailable to aid in debugging.
code/server/src/core/builtins/chat.h (1)
16-28: Consider logging when ResourceManager is unavailable.Similar to the vehicle event handlers, the null-checks provide safety but could hide initialization issues. Consider adding warning logs when ResourceManager is unavailable and events fail to dispatch.
🔎 Suggested enhancement with logging
static void EventChatMessage(flecs::entity e, std::string message) { const auto resourceManager = Framework::CoreModules::GetResourceManager(); - if (resourceManager) { - resourceManager->InvokeGlobalEvent("onChatMessage", Player(e), message); + if (!resourceManager) { + FW_LOG_WARNING("ResourceManager unavailable, event onChatMessage not dispatched"); + return; } + resourceManager->InvokeGlobalEvent("onChatMessage", Player(e), message); } static void EventChatCommand(flecs::entity e, std::string message, std::string command, std::vector<std::string> args) { const auto resourceManager = Framework::CoreModules::GetResourceManager(); - if (resourceManager) { - resourceManager->InvokeGlobalEvent("onChatCommand", Player(e), message, command, args); + if (!resourceManager) { + FW_LOG_WARNING("ResourceManager unavailable, event onChatCommand not dispatched"); + return; } + resourceManager->InvokeGlobalEvent("onChatCommand", Player(e), message, command, args); }code/server/src/core/builtins/player.cpp (1)
11-16: Consider logging when ResourceManager is unavailable.The null check prevents crashes, but silent failures can complicate debugging in production. Consider adding telemetry or a warning log when
resourceManageris null so operators can detect misconfiguration.🔎 Optional: Add logging for null ResourceManager
void Player::EventPlayerConnected(flecs::entity e) { const auto resourceManager = Framework::CoreModules::GetResourceManager(); if (resourceManager) { resourceManager->InvokeGlobalEvent("onPlayerConnected", Player(e)); + } else { + // Log warning: ResourceManager unavailable, event not dispatched } }resources/freeroam/server/main.lua (1)
3-4: Add error handling for the shared-utils import.If the
shared-utilsresource fails to load or export theutilsmodule, accessingutilslater (e.g., line 19, 296) will cause runtime errors. Consider adding a nil check or pcall wrapper to provide a clearer error message.🔎 Proposed error handling
-- Get utils from shared-utils resource export -local utils = Exports.get("shared-utils", "utils") +local utils = Exports.get("shared-utils", "utils") +if not utils then + error("[FREEROAM] Failed to load utils from shared-utils resource") +end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
code/server/src/core/builtins/chat.h(1 hunks)code/server/src/core/builtins/player.cpp(1 hunks)code/server/src/core/builtins/vehicle.cpp(1 hunks)code/server/src/core/modules/vehicle.cpp(1 hunks)gamemode/.editorconfig(0 hunks)gamemode/README.md(0 hunks)gamemode/client/.luarc.json(0 hunks)gamemode/client/main.lua(0 hunks)gamemode/client/sdk.client.d.lua(0 hunks)gamemode/gamemode.code-workspace(0 hunks)gamemode/manifest.json(0 hunks)gamemode/server/.luarc.json(0 hunks)gamemode/server/sdk.server.d.lua(0 hunks)gamemode/shared/.luarc.json(0 hunks)gamemode/shared/sdk.shared.d.lua(0 hunks)resources/freeroam/client/main.lua(1 hunks)resources/freeroam/manifest.json(1 hunks)resources/freeroam/server/debug.lua(3 hunks)resources/freeroam/server/main.lua(7 hunks)resources/shared-utils/manifest.json(1 hunks)resources/shared-utils/utils.lua(1 hunks)
💤 Files with no reviewable changes (11)
- gamemode/README.md
- gamemode/shared/.luarc.json
- gamemode/gamemode.code-workspace
- gamemode/.editorconfig
- gamemode/client/main.lua
- gamemode/manifest.json
- gamemode/server/.luarc.json
- gamemode/client/.luarc.json
- gamemode/server/sdk.server.d.lua
- gamemode/client/sdk.client.d.lua
- gamemode/shared/sdk.shared.d.lua
🧰 Additional context used
🧬 Code graph analysis (2)
code/server/src/core/builtins/vehicle.cpp (2)
code/server/src/core/modules/vehicle.cpp (1)
Vehicle(27-42)code/server/src/core/modules/vehicle.h (1)
Vehicle(9-22)
code/server/src/core/builtins/chat.h (1)
code/shared/rpc/chat_message.h (1)
MafiaMP(17-37)
🔇 Additional comments (13)
resources/shared-utils/utils.lua (1)
23-26: LGTM!The export registration is implemented correctly and aligns with the manifest declaration. Maintaining the return statement preserves backward compatibility for any code using
require().resources/shared-utils/manifest.json (1)
1-16: LGTM!The manifest structure is well-formed with appropriate metadata, file declarations, and exports. Priority 0 is suitable for a shared utility resource that other resources depend on.
resources/freeroam/manifest.json (1)
1-22: LGTM!The manifest is properly structured with appropriate dependency on shared-utils and file declarations. The priority of 10 ensures freeroam loads after its dependencies.
resources/freeroam/client/main.lua (1)
1-7: LGTM!The resource lifecycle functions are properly implemented with clear logging. The [FREEROAM] prefix maintains consistency with the server-side logging convention.
resources/freeroam/server/debug.lua (2)
4-19: LGTM!The migration from
Event.ontoEvent.onGlobalcorrectly aligns with the resource-based global event system. The debug logging functionality remains intact.Also applies to: 21-43, 45-56
1-1: Import path change verified and correct.The module exists at
resources/freeroam/server/config.luaand is properly loaded by bothdebug.luaandmain.luausing the new pathfreeroam/server/config. No references to the oldgamemode/server/configpath remain in the codebase.code/server/src/core/builtins/player.cpp (3)
8-9: LGTM!The include is necessary for the ResourceManager-based event dispatch pattern used throughout this file.
18-23: Consistent implementation.Same ResourceManager-based dispatch pattern as
EventPlayerConnected. The same logging consideration applies here.
25-30: Event name consistency verified across C++ and Lua files. All event names dispatched inplayer.cpp("onPlayerConnected","onPlayerDisconnected","onPlayerDied") exactly match their registrations inresources/freeroam/server/main.lua. No string literal mismatches found.resources/freeroam/server/main.lua (4)
8-28: LGTM!The lifecycle hooks are well-structured and follow convention-based patterns. The resource initialization logic is clear, and logging uses the correct
[FREEROAM]prefix.
30-105: Event registrations successfully migrated to global scope.All event handlers correctly use
Event.onGlobalinstead ofEvent.on, aligning with the ResourceManager-based dispatch from C++. The event names match the dispatched events, and logging prefixes are consistently updated.
351-351: LGTM!The change from
Event.emittoEvent.broadcastis correct for the global event system and aligns with the resource-based architecture.
335-336: LGTM!The logging prefix is correctly updated to
[FREEROAM], maintaining consistency with the rest of the file.
github-project-automation
bot
moved this from In Progress
to Done
in MafiaHub Dashboard
Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.