refactor(events): split events into separate files#177
refactor(events): split events into separate files#177ramon-bernardo wants to merge 8 commits intodevfrom
Conversation
📝 WalkthroughWalkthroughCentralized the event system into a new Changes
Sequence Diagram(s)sequenceDiagram
participant ScriptManager
participant EventsAggregator as "tfs::events (caller)"
participant Module as "tfs::events::creature / game / party / player / monster"
participant Lua as "LuaScriptInterface"
participant FS as "data/scripts/events/*.lua"
rect rgba(200,200,255,0.5)
ScriptManager->>EventsAggregator: call load() (was single tfs::events::load)
EventsAggregator->>Module: call Module::load() for each subsystem
Module->>Lua: Lua.init() / load script file
Lua->>FS: read specific .lua (e.g., creature.lua)
FS-->>Lua: file contents
Lua-->>Module: register meta-event IDs
Module-->>EventsAggregator: return loaded
EventsAggregator-->>ScriptManager: all modules loaded
end
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)
src/CMakeLists.txt (1)
18-18:⚠️ Potential issue | 🔴 CriticalRemove stale reference to deleted
src/events.cppon line 18.The file
src/events.cppno longer exists and should not be listed in the main CMakeLists.txt. The newsrc/events/events.cppis properly compiled as part of theeventsOBJECT library (included viaadd_subdirectory(events)on line 180). Keeping the stale reference on line 18 will cause the build to fail when CMake attempts to compile a non-existent source file.
🤖 Fix all issues with AI agents
In `@src/CMakeLists.txt`:
- Around line 180-181: The new OBJECT library target events is added but not
wired into the tfslib target; add a target_sources(tfslib PRIVATE
$<TARGET_OBJECTS:events>) (mirroring the existing target_sources call that wires
luascript) so the events object files are linked into the final binary; locate
the existing target_sources call for luascript and add an equivalent line
referencing events and tfslib.
In `@src/events/CMakeLists.txt`:
- Line 19: Remove the stale reference to ${CMAKE_CURRENT_LIST_DIR}/events.cpp
from the tfs_SRC list and ensure the events OBJECT library produced by
add_subdirectory(events) is actually consumed by tfslib: delete the old
events.cpp entry from tfs_SRC, update src/CMakeLists.txt to add
target_sources(tfslib PRIVATE $<TARGET_OBJECTS:events>) after the existing
add_subdirectory(events) line, and remove or wire the events_HDR variable so
headers are either used or the variable deleted; this ensures add_library(events
OBJECT ${events_SRC}) object files are linked into tfslib and no dead
source/header variables remain.
In `@vc18/atlas.vcxproj.filters`:
- Around line 697-699: The ClInclude entry for the header register.h was moved
to the root "Header Files" filter; update the <ClInclude
Include="..\src\lua\modules\register.h"> block in the vcxproj.filters so its
<Filter> value matches the lua sub-filter (e.g. "Header Files\lua" or the
existing lua subfolder filter used by other headers), i.e. change the <Filter>
element back to the expected sub-filter for lua headers and verify other entries
in the file (other src\lua\modules\*.h) use the same filter for consistency.
🧹 Nitpick comments (6)
src/events/CMakeLists.txt (1)
10-17:events_HDRis defined but unused.The header list variable is declared but never referenced in
add_libraryor any other CMake command. If it's intended for IDE integration or the format target, it should be included; otherwise it's dead configuration.Option: include headers in the library target for IDE visibility
-add_library(events OBJECT ${events_SRC}) +add_library(events OBJECT ${events_SRC} ${events_HDR})src/events/party.h (1)
6-19: Consider adding a forward declaration forPlayer.
Partyis forward-declared on Line 6, butPlayer(used in multiple function signatures) is not. For consistency and to avoid reliance on transitive includes or PCH for this header's self-containment, a forward declaration forPlayerwould be appropriate.Proposed fix
class Party; +class Player; namespace tfs::events::party {src/events/monster.h (1)
1-15: Add forward declarations forMonster,Container, andPositionfor consistency and header hygiene.Unlike
src/events/creature.hwhich forward-declaresMonsterandContainer, this header uses these types in function signatures without forward declarations. Although the code currently compiles because translation units include necessary headers before calling these functions, explicitly forward-declaring these types improves header self-sufficiency and consistency with the codebase pattern shown increature.h.Suggested fix
`#pragma` once +class Container; +class Monster; +struct Position; + namespace tfs::events::monster {src/events/creature.cpp (1)
6-6: Include of"../item.h"— verify it's still needed.
item.his included butItemis only used as ashared_ptr<Item>parameter inonDeath(line 415). Depending on what the header forcreature.halready pulls in (e.g., forward declarations vs full definitions), this include might be redundant, or it might be needed forCombatDamage. Low priority — just a note.Also applies to: 8-10
src/events/player.cpp (1)
534-559: Intentional parameter reorder for Lua — just a note for maintainers.The C++ signature has
(player, item, outfit, podiumVisible, direction)but the Lua push order is(player, item, outfit, direction, podiumVisible)(lines 553-557), matching the Lua commentPlayer:onPodiumEdit(item, outfit, direction, isVisible). This deliberate reorder is easy to miss; a brief inline comment would help future readers.src/events/party.cpp (1)
8-8: Remove unused#include "../item.h"on line 8.The include is unused—no Item-related types appear anywhere in this file. Party event handlers only use
Party,Player, anduint64_ttypes.Proposed fix
-#include "../item.h" `#include` "../lua/env.h"
Co-authored-by: Ranieri Althoff <1993083+ranisalt@users.noreply.github.com>
Co-authored-by: Ranieri Althoff <1993083+ranisalt@users.noreply.github.com>
| void onPodiumEdit(const std::shared_ptr<Player>& player, const std::shared_ptr<Item>& item, const Outfit_t& outfit, | ||
| bool podiumVisible, Direction direction) |
There was a problem hiding this comment.
Maybe we should reorder these to match the Lua signature?
Not important
Co-authored-by: Ranieri Althoff <1993083+ranisalt@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/events/creature.cpp`:
- Around line 307-310: The current block negates damage.secondary.value whenever
damage.primary.type != COMBAT_HEALING, which can flip healing into damage;
update the logic to mirror the pattern used in src/game.cpp:3936 by checking
each damage component's type independently — i.e., keep the existing check for
damage.primary.type before negating damage.primary.value, and add a separate
conditional that verifies damage.secondary.type != COMBAT_HEALING before
negating damage.secondary.value (refer to the symbols damage.primary.type,
damage.primary.value, damage.secondary.type, damage.secondary.value, and
COMBAT_HEALING).
| if (damage.primary.type != COMBAT_HEALING) { | ||
| damage.primary.value = -damage.primary.value; | ||
| damage.secondary.value = -damage.secondary.value; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the specific file and lines mentioned
head -320 src/events/creature.cpp | tail -25Repository: atlas-kit/atlas
Length of output: 1079
🏁 Script executed:
#!/bin/bash
# Check the damage structure to understand primary and secondary fields
rg -n 'struct.*damage|class.*damage' --type cpp -A 10 -i | head -50Repository: atlas-kit/atlas
Length of output: 1886
🏁 Script executed:
#!/bin/bash
# Look for where damage.secondary is used
rg -n 'damage\.secondary' --type cpp -B 2 -A 2 | head -80Repository: atlas-kit/atlas
Length of output: 5088
🏁 Script executed:
#!/bin/bash
# Check if secondary.type exists and is checked elsewhere
rg -n 'secondary\.type' --type cpp -B 2 -A 2Repository: atlas-kit/atlas
Length of output: 7246
Secondary damage negation should check secondary type independently — correctness issue.
The negation block only checks damage.primary.type != COMBAT_HEALING but unconditionally negates damage.secondary.value. If secondary damage type is COMBAT_HEALING while primary is not, the healing value will be incorrectly negated to damage. This violates the established pattern in src/game.cpp:3936 where secondary type is always checked before negating secondary value.
Suggested fix
if (damage.primary.type != COMBAT_HEALING) {
damage.primary.value = -damage.primary.value;
- damage.secondary.value = -damage.secondary.value;
}
+ if (damage.secondary.type != COMBAT_HEALING) {
+ damage.secondary.value = -damage.secondary.value;
+ }📝 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 (damage.primary.type != COMBAT_HEALING) { | |
| damage.primary.value = -damage.primary.value; | |
| damage.secondary.value = -damage.secondary.value; | |
| } | |
| if (damage.primary.type != COMBAT_HEALING) { | |
| damage.primary.value = -damage.primary.value; | |
| } | |
| if (damage.secondary.type != COMBAT_HEALING) { | |
| damage.secondary.value = -damage.secondary.value; | |
| } |
🤖 Prompt for AI Agents
In `@src/events/creature.cpp` around lines 307 - 310, The current block negates
damage.secondary.value whenever damage.primary.type != COMBAT_HEALING, which can
flip healing into damage; update the logic to mirror the pattern used in
src/game.cpp:3936 by checking each damage component's type independently — i.e.,
keep the existing check for damage.primary.type before negating
damage.primary.value, and add a separate conditional that verifies
damage.secondary.type != COMBAT_HEALING before negating damage.secondary.value
(refer to the symbols damage.primary.type, damage.primary.value,
damage.secondary.type, damage.secondary.value, and COMBAT_HEALING).
2 participants
Pull Request Prelude
Changes Proposed
This change reorganizes the event-related classes into a dedicated events module to improve code structure, maintainability, and separation of concerns.
With this refactor, these classes were moved into a new src/events directory, and each event was split into its own header and source file. The build configuration and module references were updated accordingly to reflect the new structure.
This does not introduce new functionality but improves the overall organization of the codebase, making the event system easier to navigate, extend and maintain.
Summary by CodeRabbit
Refactor
Chores
User impact