feat: migrate Condition to modern smart pointers#106
feat: migrate Condition to modern smart pointers#106luanluciano93 wants to merge 13 commits intoatlas-kit:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request migrates the Condition class and related code from raw pointers to modern C++ smart pointers (primarily std::unique_ptr), addressing issue #40. This is a significant refactoring that improves memory safety and adheres to modern C++ best practices.
Key changes:
- Factory methods
Condition::createCondition()now returnstd::unique_ptr<Condition>instead of raw pointers - The
clone()method across all Condition subclasses now returnsstd::unique_ptr<Condition> - Condition storage in creatures, players, and combat parameters migrated to
std::unique_ptr
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/condition.h | Updated return types for createCondition() and clone() methods to return std::unique_ptr<Condition> |
| src/condition.cpp | Refactored factory method to use std::make_unique for all condition types |
| src/creature.h | Changed conditions member to std::vector<std::unique_ptr<Condition>> and updated method signatures |
| src/creature.cpp | Updated condition management methods to use move semantics and smart pointers; fixed order of operations in removal methods |
| src/player.h | Changed storedConditionList to std::forward_list<std::unique_ptr<Condition>> |
| src/player.cpp | Updated condition handling to use move semantics for stored conditions |
| src/spells.cpp | Replaced raw pointer creation with auto and added std::move() when adding conditions |
| src/movement.cpp | Replaced raw pointer creation with auto and added std::move() when adding conditions |
| src/monsters.h | Updated getDamageCondition() return type to std::unique_ptr<ConditionDamage> |
| src/monsters.cpp | Refactored monster spell condition creation to use smart pointers |
| src/luascript.cpp | Added .release() calls where raw pointers are needed for Lua userdata |
| src/iologindata.cpp | Updated condition loading to use smart pointers with automatic cleanup |
| src/game.h | Updated forceAddCondition() to accept std::unique_ptr<Condition> |
| src/game.cpp | Removed manual delete call since smart pointers handle cleanup |
| src/combat.h | Updated addCondition() to accept std::unique_ptr<Condition> |
| src/combat.cpp | Updated condition cloning and application to use smart pointers with move semantics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto condition = std::unique_ptr<ConditionDamage>( | ||
| static_cast<ConditionDamage*>(Condition::createCondition(CONDITIONID_COMBAT, conditionType, 0, 0).release())); |
There was a problem hiding this comment.
The pattern of creating a unique_ptr, immediately calling release(), and wrapping it in another unique_ptr is unnecessarily complex. Consider using a helper function or static_pointer_cast pattern to simplify this type conversion. While this code is functionally correct, it reduces code clarity and could be streamlined.
There was a problem hiding this comment.
Yeah std::static_pointer_cast is the way
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis refactoring converts condition object ownership from raw pointers (Condition*) to smart pointers (std::unique_ptr) throughout the codebase. Changes affect factory methods, virtual clone implementations, method signatures in core classes (Creature, Combat, Game, Player), member storage containers, and numerous call sites across game logic and spell systems. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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 (2)
src/creature.cpp (2)
964-1009: Condition ownership & lifecycle changes are sound; just address the clang-format warningThe migration of condition handling to
std::unique_ptr<Condition>(inaddCondition,addCombatCondition, the variousremoveConditionoverloads, andexecuteConditions) correctly transfers ownership, avoids manual deletes, and carefully uses rawCondition*only as non-owning views (via.get()) when needed. The HASTE‑while‑PARALYZE deferral path uses a mutable lambda that moves the unique_ptr intoGame::forceAddCondition, which is the right pattern for scheduled work.CI does report a clang-format violation around the HASTE deferral lambda (line 973). Please run clang-format on this file to satisfy the style check.
Also applies to: 1011-1061, 1077-1081, 1119-1144, 1146-1163
1328-1333: Fix isInvisible after switching conditions to std::unique_ptr
conditionsis now astd::vector<std::unique_ptr<Condition>>, butstd::find_ifhere still assumes elements areCondition*, which will no longer compile. Iterate over the unique_ptrs directly and call->getType()on each.Proposed fix for
Creature::isInvisible-bool Creature::isInvisible() const -{ - return std::find_if(conditions.begin(), conditions.end(), [](const Condition* condition) { - return condition->getType() == CONDITION_INVISIBLE; - }) != conditions.end(); -} +bool Creature::isInvisible() const +{ + for (const auto& condition : conditions) { + if (condition->getType() == CONDITION_INVISIBLE) { + return true; + } + } + return false; +}
🧹 Nitpick comments (8)
src/condition.cpp (1)
234-293: Deserialization overload using std::unique_ptr is consistent; consider guarding for corrupted inputThe
createCondition(PropStream&)overload correctly returnsstd::unique_ptr<Condition>and delegates to the other factory. Semantics (returningnullptron any read/tag mismatch) match the old raw-pointer version.If you ever expect corrupted
conditionsblobs, you might optionally log or assert whencreateCondition(...)returnsnullptrwhile there is still unread data, to make such cases easier to diagnose.src/player.h (1)
1273-1274: storedConditionList now owns Condition objects via std::unique_ptr – good alignment with new modelSwitching
storedConditionListtostd::forward_list<std::unique_ptr<Condition>>matches the rest of the smart-pointer migration and removes the manual ownership burden from IOLoginData/Player.clang-format is currently failing on this area; please run the project’s clang-format profile so the declaration is formatted according to the house style.
src/iologindata.cpp (1)
194-200: Condition loading now uses unique_ptr and moves into Player::storedConditionList – semantics preservedUsing
auto condition = Condition::createCondition(propStream);andpush_front(std::move(condition))is the right way to tie deserialized conditions tostoredConditionListwithout leaks. The loop structure and failure handling (unserializereturning false) match the old behavior.If you want to simplify, you could fold creation and unserialize into a single
whilecondition, but that’s purely cosmetic.src/combat.cpp (1)
1337-1363: MagicField::onStepInField: cloning item conditionDamage and moving into creature is correctUsing
auto conditionCopy = it.conditionDamage->clone();followed by optionalCONDITION_PARAM_OWNERtagging andcreature->addCondition(std::move(conditionCopy));correctly adapts field damage conditions to the new unique_ptr APIs.Given clang-format failures are reported in this file, please run clang-format on
src/combat.cppso these newauto conditionCopy = ...blocks match the project’s style (line wrapping/indentation).src/spells.cpp (1)
738-759: Spell cooldown conditions now created and owned via std::unique_ptr; behavior preservedAll three cooldown families (per-spell, group, secondary group) are now allocated via
Condition::createCondition(..., CONDITION_SPELLCOOLDOWN/CONDITION_SPELLGROUPCOOLDOWN, ...)and added withplayer->addCondition(std::move(condition));both in the normal post-cast path and the early-error paths inInstantSpell::playerCastInstant. This matches the new ownership model and keeps prior semantics (cooldowns still applied on failure where intended).There is some repetition of the same three-condition pattern across these branches; if you touch this again, consider a small helper like
applySpellCooldowns(Player&)to centralize it. Also, clang-format is complaining on these ranges, so please run clang-format onsrc/spells.cppto fix wrapping/indentation.Also applies to: 868-884, 932-947
src/combat.h (1)
121-124: Combat::addCondition now transfers ownership via std::unique_ptr – API is coherentStoring conditions as
std::vector<std::unique_ptr<const Condition>>and exposingaddCondition(std::unique_ptr<Condition> condition)that emplaces withstd::movegives you an owning container of immutable prototypes, which is exactly what the cloning code incombat.cppexpects.Minor nit: you could tighten the API by taking
std::unique_ptr<const Condition>here to match the container type, but the current form is perfectly valid. Also, clang-format is currently failing on this block; please run clang-format onsrc/combat.h.src/game.h (1)
83-84: Game::forceAddCondition now owns the passed Condition via std::unique_ptr – consistent with Creature APIUpdating
forceAddConditionto takestd::unique_ptr<Condition>and forward it viastd::moveintocreature->addCondition(..., true)is the right way to align with the new ownership model. The forward declarationclass Condition;is still sufficient for this signature.Optionally, you may want to include
<memory>directly ingame.hto make the dependency onstd::unique_ptrexplicit rather than relying on transitive includes.src/movement.cpp (1)
716-757: Equip-item ability conditions correctly migrated to unique_ptr; consider light robustness + formattingThe changes in
MoveEvent::EquipItemto:
- create invisibility/mana-shield/regeneration via
Condition::createCondition(static_cast<ConditionId_t>(slot), CONDITION_..., -1, 0)and- pass them into
player->addCondition(std::move(condition));align perfectly with the new condition-ownership model and preserve the previous semantics (id keyed by slot, infinite duration while equipped).
Given
CONDITION_INVISIBLE,CONDITION_MANASHIELD, andCONDITION_REGENERATIONare all handled explicitly in the factory, lack of a null-check is acceptable, but if you ever add dynamic/unknown types here, a simpleif (!condition) return RETURNVALUE_NOTPOSSIBLE;-style guard would make failures easier to debug. Also, clang-format is currently failing on this region; please run clang-format onsrc/movement.cpp.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/combat.cppsrc/combat.hsrc/condition.cppsrc/condition.hsrc/creature.cppsrc/creature.hsrc/game.cppsrc/game.hsrc/iologindata.cppsrc/lua/modules/condition.cppsrc/monsters.cppsrc/monsters.hsrc/movement.cppsrc/player.cppsrc/player.hsrc/spells.cpp
🧰 Additional context used
🧬 Code graph analysis (12)
src/player.h (1)
src/condition.h (1)
Condition(60-119)
src/movement.cpp (1)
src/condition.cpp (4)
createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
src/iologindata.cpp (1)
src/condition.cpp (4)
createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
src/spells.cpp (1)
src/condition.cpp (4)
createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
src/game.h (2)
src/game.cpp (2)
forceAddCondition(5413-5421)forceAddCondition(5413-5413)src/condition.h (1)
Condition(60-119)
src/monsters.h (1)
src/monsters.cpp (2)
getDamageCondition(70-81)getDamageCondition(70-72)
src/game.cpp (1)
src/condition.cpp (4)
createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
src/monsters.cpp (1)
src/condition.cpp (4)
createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
src/creature.cpp (3)
src/scheduler.cpp (2)
createSchedulerTask(70-73)createSchedulerTask(70-70)src/player.cpp (4)
onAddCondition(3408-3417)onAddCondition(3408-3408)onEndCondition(3463-3479)onEndCondition(3463-3463)src/monster.cpp (4)
onAddCondition(673-673)onAddCondition(673-673)onEndCondition(675-682)onEndCondition(675-675)
src/combat.h (1)
src/condition.h (2)
Condition(60-119)std(84-87)
src/player.cpp (2)
src/creature.cpp (2)
addCondition(964-996)addCondition(964-964)src/condition.cpp (18)
addCondition(350-355)addCondition(350-350)addCondition(381-405)addCondition(381-381)addCondition(838-851)addCondition(838-838)addCondition(1011-1021)addCondition(1011-1011)addCondition(1446-1490)addCondition(1446-1446)addCondition(1670-1699)addCondition(1670-1670)addCondition(1776-1786)addCondition(1776-1776)createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
src/condition.h (1)
src/condition.cpp (6)
setTicks(150-154)setTicks(150-150)createCondition(167-232)createCondition(167-169)createCondition(234-293)createCondition(234-234)
🪛 GitHub Actions: Check code style
src/movement.cpp
[error] 722-722: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 737-737: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
src/spells.cpp
[error] 744-744: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 745-745: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 874-874: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 875-875: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 880-880: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 881-881: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 932-932: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 933-933: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
src/monsters.cpp
[error] 416-416: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 622-622: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 728-728: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
src/creature.cpp
[error] 973-973: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 973-973: clang-format violation detected at line 973. Code should be clang-formatted [-Wclang-format-violations].
src/combat.h
[error] 121-121: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 122-122: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
[error] 123-123: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: docker-image
- GitHub Check: test
🔇 Additional comments (21)
src/condition.cpp (1)
167-232: Factory overload now returning std::unique_ptr – ownership and switch coverage look correctThe migration of
Condition::createCondition(ConditionId_t, ConditionType_t, ...)to returnstd::unique_ptr<Condition>withstd::make_unique<...>for each case is clean, preserves the previous behavior, and correctly returnsnullptrin the default case. No functional issues spotted here.src/combat.cpp (4)
609-616: Condition cloning + addCombatCondition now correctly use std::unique_ptrIn the non-damage branch of
Combat::doCombat, cloning fromparams.conditionListand callingtarget->addCombatCondition(std::move(conditionCopy));is the right adaptation to the new ownership model. This preserves per-target condition instances and avoids manual deletes.
692-703: Area combat application of conditions correctly moves unique_ptr into each creatureIn the area-based
doCombat, cloning each condition and moving it intocreature->addCombatConditionis consistent with the single-target path and ensures each creature gets its own independentConditioninstance. The optional owner assignment guarded byif (caster)is also correct.
768-777: Post-hit condition application in doTargetCombat updated cleanly to unique_ptrAfter a successful hit, cloning into
conditionCopyand moving intotarget->addCombatCondition(std::move(conditionCopy))is the right ownership transfer. Behavior remains the same while eliminating raw-pointer management.
936-945: Area damage condition propagation uses move semantics correctlyThe area-damage loop now clones from
params.conditionListand moves the clone intocreature->addCombatCondition, mirroring the other call sites. This keeps ownership well-defined and matches the unique_ptr-based storage onCreature.src/monsters.h (1)
267-268: Clean migration to smart pointer return type.The signature change correctly reflects ownership transfer semantics. The implementation in
src/monsters.cpp(lines 69-80) properly usesstd::make_unique<ConditionDamage>and returns the unique pointer, ensuring consistent ownership throughout the call chain.src/player.cpp (7)
1009-1012: Correct ownership transfer from stored conditions.The iteration uses
auto&to get a reference to theunique_ptr, andstd::moveproperly transfers ownership toaddCondition. The container is cleared after the loop, which is correct since all elements have been moved out.
1319-1321: LGTM - proper smart pointer creation and ownership transfer.The condition is created, null-checked, and ownership is correctly transferred via
std::move.
1572-1573: Correct smart pointer usage for mute condition.
2114-2124: Proper iteration over unique_ptr container.Using
auto&correctly references theunique_ptr<Condition>elements, and the erase-by-iterator pattern works correctly with the smart pointer container.
2128-2138: Consistent iteration pattern in the else branch.
2218-2220: LGTM - in-fight condition creation follows the established pattern.
3653-3655: Consistent smart pointer pattern for white skull condition.src/creature.h (2)
265-266: Clean API migration to unique_ptr parameters.The method signatures correctly express ownership transfer semantics. Callers must transfer ownership via
std::move, and the creature takes full responsibility for the condition's lifetime. This aligns with the implementation insrc/creature.cpp(lines 963-995 from the relevant snippets).
390-390: Proper container migration to unique_ptr.Changing from
std::vector<Condition*>tostd::vector<std::unique_ptr<Condition>>ensures automatic cleanup when conditions are removed or the creature is destroyed, eliminating potential memory leaks from the previous raw pointer design.src/lua/modules/condition.cpp (2)
18-24: Correct ownership transfer to Lua userdata.Using
release()on theunique_ptrcorrectly transfers ownership to Lua. The__gcmetamethod (registered at line 244 vialuaConditionDelete) ensures proper cleanup when the Lua object is garbage collected. This is the standard pattern for C++/Lua interop with owned objects.
103-105: Clone correctly transfers ownership to Lua.The
clone()method now returnsunique_ptr, andrelease()properly transfers ownership to the Lua userdata. The metatable is set correctly to ensure the__gcdestructor will be called.src/game.cpp (2)
3553-3581: Yell cooldown condition now correctly uses unique_ptr and Creature::addConditionUsing
Condition::createConditionto obtain astd::unique_ptr<Condition>and passing it viastd::moveintoplayer->addConditionmatches the new ownership model and avoids manual deletes while preserving the previous behavior of applying a yell cooldown.
5413-5421: forceAddCondition signature and call site align with unique_ptr ownershipAccepting
std::unique_ptr<Condition>by value and forwarding it withstd::moveintocreature->addCondition(..., true)cleanly transfers ownership and safely no-ops if the creature no longer exists.src/monsters.cpp (1)
70-81: Damage condition factory and call sites correctly migrated to std::unique_ptr
Monsters::getDamageConditionnow returnsstd::unique_ptr<ConditionDamage>and is consumed directly bycombat->addCondition(...)in both XML andMonsterSpelldeserialization paths. Ownership and parameterization (min/max/start damage, tick interval, delayed flag) mirror the prior raw-pointer implementation without changing behavior.Also applies to: 288-290, 477-478, 622-623
src/condition.h (1)
84-96: Condition cloning and factory APIs are consistently updated to use std::unique_ptrSwitching
Condition::cloneand bothCondition::createConditionoverloads to returnstd::unique_ptr<Condition>, with each derived class implementingclone()viastd::make_unique<Derived>(*this), cleanly aligns the hierarchy with the new ownership model and removes raw-pointer allocation from these APIs without altering behavior.Also applies to: 135-136, 154-155, 192-193, 222-223, 245-246, 265-266, 318-319, 352-353, 377-378, 403-404, 417-418, 438-439, 461-462
| auto condition = | ||
| Condition::createCondition(CONDITIONID_COMBAT, CONDITION_DRUNK, duration, drunkenness); | ||
| combat->addCondition(condition); | ||
| combat->addCondition(std::move(condition)); | ||
| } else if (tmpName == "firefield") { |
There was a problem hiding this comment.
Fix clang-format violations reported by CI in this file
The CI pipeline reports clang-format failures around these new condition-creation/addCondition lines. Please run clang-format with the project’s configuration (e.g., clang-format -i src/monsters.cpp) to bring these sections into compliance so the style check passes.
Also applies to: 622-623, 728-731
🧰 Tools
🪛 GitHub Actions: Check code style
[error] 416-416: clang-format check failed. code should be clang-formatted [-Wclang-format-violations]. Command: 'clang-format -n -style=file --Werror src/*.{cpp,h}'
🤖 Prompt for AI Agents
In src/monsters.cpp around lines 416-419 (also apply same change at 622-623 and
728-731), clang-format style violations were introduced around the Condition
creation and addCondition calls; run the project clang-format (e.g.,
clang-format -i src/monsters.cpp) or reformat those blocks so they conform to
the repository's style (aligning parameters, breaks, and indentation consistent
with surrounding code) and re-run CI to ensure the formatting check passes.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 8 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
- Remove dead `if (condition)` inside scheduler lambda; the unique_ptr is validated non-null at function entry and moved into the lambda. - Remove obsolete `Caution: condition variable could be deleted` note; with unique_ptr semantics the variable is moved-from, not deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull Request Prelude
Changes Proposed
Issues addressed: #40
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.