Skip to content

fix: avoid animation phase edge-case crashes#1683

Open
Kizuno18 wants to merge 2 commits intoopentibiabr:mainfrom
Kizuno18:fix/animation-guards
Open

fix: avoid animation phase edge-case crashes#1683
Kizuno18 wants to merge 2 commits intoopentibiabr:mainfrom
Kizuno18:fix/animation-guards

Conversation

@Kizuno18
Copy link
Copy Markdown
Contributor

@Kizuno18 Kizuno18 commented Apr 3, 2026

Hey folks!

This is a small safety pass to avoid a couple of nasty edge cases that can crash the client when assets/thing types are incomplete or in a weird state.

  • Guard null ThingType* before using it
  • Guard getAnimationPhases() / getSpeed() against 0 to avoid divide-by-zero
  • Clamp computed ticksPerFrame to at least 1
  • Enable CRASH_HANDLER for MSVC builds so we get crashreport.log on Windows

No functional changes intended beyond not crashing in those scenarios.

Cheers!

Summary by CodeRabbit

  • New Features

    • Added crash handler support for improved stability and error recovery
  • Bug Fixes

    • Made animation phase logic robust with null-safety checks to avoid crashes
    • Hardened animation timing calculations to prevent invalid timing, division/modulo issues, and rendering glitches

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 956a7f1e-d051-42e1-a902-903a6747f937

📥 Commits

Reviewing files that changed from the base of the PR and between e188f3c and c981ca2.

📒 Files selected for processing (2)
  • src/client/attachedeffect.cpp
  • src/client/paperdoll.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/client/paperdoll.cpp
  • src/client/attachedeffect.cpp

📝 Walkthrough

Walkthrough

Adds an MSVC compile definition CRASH_HANDLER for the otclient_core target and hardens animation-phase calculations in multiple client files by adding null checks and guarded, clamped timing arithmetic to prevent invalid/zero divisions.

Changes

Cohort / File(s) Summary
Build Configuration
src/CMakeLists.txt
Added target_compile_definitions(otclient_core PRIVATE CRASH_HANDLER) for MSVC builds.
Animation Phase Calculations
src/client/attachedeffect.cpp, src/client/creature.cpp, src/client/paperdoll.cpp
getCurrentAnimationPhase(): early-return when thing type is null; validate animationPhases and speed; clamp ticksPerFrame to a minimum of 1; compute animationPeriod safely and use guarded modulo/division to derive phase. EOF newline fixes in two files.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nibble through bytes with a careful hop,

Nulls checked, timers clipped, no more flop.
Crashes hushed and frames now glide,
A tiny patch, a confident stride.
—signed, the debugging rabbit 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: avoid animation phase edge-case crashes' directly and accurately describes the main objective of the changeset—adding safety guards and null checks to prevent client crashes in animation phase calculations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@majestyotbr majestyotbr changed the title Fix: avoid animation phase edge-case crashes fix: avoid animation phase edge-case crashes Apr 3, 2026
@vllsystems
Copy link
Copy Markdown
Contributor

vllsystems commented Apr 4, 2026

Wouldn't it be good to add some warning logs to this PR?

getSpeed() returns float (m_speed / 100.f), but was being stored as
const int, truncating values < 1.0 to 0. This triggered the speed <= 0
guard and killed animations for any attached effect with speed < 100
(e.g. wings/auras configured with speed = 0.5).

Also fixes thingTye typo to thingType in attachedeffect.cpp.
@Kizuno18
Copy link
Copy Markdown
Contributor Author

Kizuno18 commented Apr 4, 2026

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants