Adds 3 new animations sprite sheets replaces sword swing.#133
Conversation
WalkthroughAdds per-player skill animation lifecycle management, a new Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant Skill
participant Animation
participant SkillList as Player.skillAnimations
Note over Player,Skill: Skill is used with Player context
Player->>Skill: use_skill(Player, Skill, SkillData)
activate Skill
alt Skill has Skill->animation
Skill->>Animation: clone/copy transform (pos/flip/angle)
activate Animation
Animation-->>Skill: started animation instance
Skill->>SkillList: append(animation instance)
else no animation
Skill-->>Player: perform non-animated effect
end
deactivate Skill
Note over Player,SkillList: Per-frame loops
Player->>SkillList: update_skill_animations()
Player->>SkillList: render_skill_animations(Camera)
Note over Player,SkillList: On player destroy
Player->>SkillList: destroy_skill_animations()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
LiquidityC
force-pushed
the
new_animations
branch
from
7e9894c to
09f821c
Compare
The Explosion and tripple swing animations still need to be implemented.
LiquidityC
changed the title
Also implements the flurry animation
![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 (2)
src/player.h (1)
104-105: Clarify ownership of Animation pointers stored hereAdd a short comment noting nodes hold borrowed Animation* owned by Skill; list frees nodes only.
src/player.c (1)
480-490: List cleanup is adequateNodes-only free is correct since Animation* is owned by Skill. Consider nulling caller’s head if reused, but not required here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/Extras/TrippleSwing.pngis excluded by!**/*.png
📒 Files selected for processing (3)
src/player.c(8 hunks)src/player.h(1 hunks)src/skill.c(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/player.h (2)
src/skill.h (4)
struct Player(29-29)struct Skill_t {(51-63)struct SkillData_t {(45-49)struct Skill_t(61-61)src/animation.h (1)
struct Animation(38-47)
src/player.c (3)
src/animation.c (3)
animation_update(45-76)animation_render(78-86)animation_run(96-100)src/linkedlist.c (1)
linkedlist_append(61-77)src/skill.h (1)
struct Skill_t {(51-63)
src/skill.c (2)
src/tooltip.c (1)
tooltip_create(122-129)src/animation.c (4)
animation_create(25-37)animation_load_texture(39-43)animation_set_frames(88-94)animation_destroy(110-116)
⏰ 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). (7)
- GitHub Check: Mac (GCC)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Ubuntu (Clang)
- GitHub Check: Mac (Clang)
- GitHub Check: Ubuntu (GCC)
- GitHub Check: Ubuntu (mingw)
- GitHub Check: Windows (MSVC)
🔇 Additional comments (11)
src/skill.c (4)
310-312: Good: initialize new animation memberSetting skill->animation = NULL avoids UB.
Confirm struct Skill in src/skill.h contains Animation *animation; otherwise this won’t compile.
1039-1040: Good: delegate FLURRY to cam-aware factoryKeeps tooltip/animation encapsulated.
1098-1100: Good: free animation on skill destructionPrevents leaks.
437-467: Flurry animation wiring verified: the texture file assets/Extras/TrippleSwing.png exists and matches the code.src/player.c (7)
445-467: Animation list updater is fineRemoves finished nodes; no double-free of Animation*. LGTM.
469-478: Render hook looks goodIterates and renders active skill animations. LGTM.
589-593: Sword swing frames updated to 32×32: LGTMClip sizes, loop=false, dim and rotation point match new asset.
Also applies to: 598-599
689-690: Init skillAnimationsExplicit NULL init is fine.
802-803: Top-layer render integrationRenders sword then per-skill animations. LGTM.
856-856: Update hook addedAdvances skill animations each tick. LGTM.
889-891: Cleanup on destroyFrees list nodes after skills/animations are destroyed. LGTM.
Ensure no code after this dereferences player->skillAnimations; if reused, set to NULL post-destroy.
| static void | ||
| use_skill(Player *player, Skill *skill, SkillData *skillData) | ||
| { | ||
| skill->active = false; | ||
| skill->use(skill, skillData); | ||
| if (skill->actionRequired) | ||
| action_spent(skillData->player); | ||
| skill->resetCountdown = skill->resetTime; | ||
|
|
||
| if (skill->animation) { | ||
| Animation *a = skill->animation; | ||
|
|
||
| // Copy the orientation and position of the sword animation for the skill animation | ||
| // \see player_turn | ||
| a->sprite->pos = player->swordAnimation->sprite->pos; | ||
| a->sprite->flip = player->swordAnimation->sprite->flip; | ||
| a->sprite->angle = player->swordAnimation->sprite->angle; | ||
|
|
||
| animation_run(a); | ||
| linkedlist_append(&player->skillAnimations, skill->animation); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix: don’t spend action/cooldown or start animation on failed skill use; also dedupe animation entries
Respect the boolean return from skill->use. Only on success: set active=false, spend action, set cooldown, and start/append animation. Also avoid appending the same Animation* multiple times (causes double update/render).
Apply:
static void
use_skill(Player *player, Skill *skill, SkillData *skillData)
{
- skill->active = false;
- skill->use(skill, skillData);
- if (skill->actionRequired)
- action_spent(skillData->player);
- skill->resetCountdown = skill->resetTime;
+ bool used = skill->use(skill, skillData);
+ if (!used) {
+ // Keep skill active on failure so the player can retry (esp. directional skills)
+ return;
+ }
+ skill->active = false;
+ if (skill->actionRequired)
+ action_spent(skillData->player);
+ skill->resetCountdown = skill->resetTime;
- if (skill->animation) {
+ if (skill->animation) {
Animation *a = skill->animation;
// Copy the orientation and position of the sword animation for the skill animation
// \see player_turn
a->sprite->pos = player->swordAnimation->sprite->pos;
a->sprite->flip = player->swordAnimation->sprite->flip;
a->sprite->angle = player->swordAnimation->sprite->angle;
animation_run(a);
- linkedlist_append(&player->skillAnimations, skill->animation);
+ // Avoid duplicate entries for the same animation instance
+ bool present = false;
+ for (LinkedList *it = player->skillAnimations; it; it = it->next) {
+ if (it->data == a) { present = true; break; }
+ }
+ if (!present) {
+ linkedlist_append(&player->skillAnimations, a);
+ }
}
}📝 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.
| static void | |
| use_skill(Player *player, Skill *skill, SkillData *skillData) | |
| { | |
| skill->active = false; | |
| skill->use(skill, skillData); | |
| if (skill->actionRequired) | |
| action_spent(skillData->player); | |
| skill->resetCountdown = skill->resetTime; | |
| if (skill->animation) { | |
| Animation *a = skill->animation; | |
| // Copy the orientation and position of the sword animation for the skill animation | |
| // \see player_turn | |
| a->sprite->pos = player->swordAnimation->sprite->pos; | |
| a->sprite->flip = player->swordAnimation->sprite->flip; | |
| a->sprite->angle = player->swordAnimation->sprite->angle; | |
| animation_run(a); | |
| linkedlist_append(&player->skillAnimations, skill->animation); | |
| } | |
| } | |
| static void | |
| use_skill(Player *player, Skill *skill, SkillData *skillData) | |
| { | |
| bool used = skill->use(skill, skillData); | |
| if (!used) { | |
| // Keep skill active on failure so the player can retry (esp. directional skills) | |
| return; | |
| } | |
| skill->active = false; | |
| if (skill->actionRequired) | |
| action_spent(skillData->player); | |
| skill->resetCountdown = skill->resetTime; | |
| if (skill->animation) { | |
| Animation *a = skill->animation; | |
| // Copy the orientation and position of the sword animation for the skill animation | |
| // \see player_turn | |
| a->sprite->pos = player->swordAnimation->sprite->pos; | |
| a->sprite->flip = player->swordAnimation->sprite->flip; | |
| a->sprite->angle = player->swordAnimation->sprite->angle; | |
| animation_run(a); | |
| // Avoid duplicate entries for the same animation instance | |
| bool present = false; | |
| for (LinkedList *it = player->skillAnimations; it; it = it->next) { | |
| if (it->data == a) { | |
| present = true; | |
| break; | |
| } | |
| } | |
| if (!present) { | |
| linkedlist_append(&player->skillAnimations, a); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/player.c around lines 492 to 513, the code ignores the boolean return
from skill->use and always spends action, sets cooldown, and starts/appends the
animation (which can be appended multiple times). Change the flow to call
skill->use(skill, skillData) and store its bool result; only if true: set
skill->active = false, call action_spent(... ) when actionRequired, set
resetCountdown, run the animation and append it. When appending, first check the
player's skillAnimations list to avoid adding the same Animation* twice (skip
append if already present). If skill->use returns false, do none of those side
effects.
1 participant
The Explosion and tripple swing animations still need to be implemented.
Summary by CodeRabbit
New Features
Chores