Fixes a bug where the player bleed would not match player pos

[LiquidityC]

Summary by CodeRabbit

• Refactor
  • Standardized collision handling by routing position updates through a unified movement flow.
  • No user-facing changes are expected from this update.

[coderabbitai]

Walkthrough

Collision handling in src/player.c now invokes a centralized position update function (player_update_pos) using negative TILE_DIMENSION multipliers instead of adjusting coordinates directly. A non-functional TODO comment was added in move regarding data flow.

Changes

Cohort / File(s)	Summary
Player movement and collision src/player.c	Refactors has_collided to call player_update_pos with negative TILE_DIMENSION offsets on collision; adds a TODO comment in move without functional impact.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor GameLoop
  participant Player
  participant Collision as CollisionSystem

  GameLoop->>Player: move(input)
  Player->>Collision: checkCollision()
  alt collision detected
    note over Player: Previously: direct coordinate adjustment
    Player->>Player: player_update_pos(-TILE_DIMENSION x/y)
    note right of Player: Centralized movement call
  else no collision
    Player-->>GameLoop: continue normal movement
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A hop, a stop—bonk! I realign,
Not nudged by paw, but one clean sign.
Central calls guide where I should be,
Negative tiles set me free.
I boop the wall, then boing in place—
Refactor done; onward I race! 🐇✨

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ 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 clearly states the specific bug being addressed and succinctly conveys the primary change of aligning the bleed emitter’s position with the player’s position. It is directly related to the player.c adjustments for collision handling and centralized movement updates.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch bleed_emitter_pos_fix

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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 (1)

src/player.c (1)

331-340: Function signature should accept signed integers.

The function signature declares Uint32 dx, Uint32 dy, but it's being called with negative values at line 260. Passing negative integers to unsigned parameters causes implicit conversion and relies on wraparound behavior, which is unclear and potentially non-portable.

Apply this diff to fix the type mismatch:

 void
-player_update_pos(Player *player, Uint32 dx, Uint32 dy)
+player_update_pos(Player *player, int dx, int dy)
 {
 	player->sprite->pos.x += dx;
 	player->sprite->pos.y += dy;

After making this change, verify that Position.x and Position.y are appropriate types that can handle signed arithmetic (either signed integers or unsigned with explicit handling).

🧹 Nitpick comments (1)

src/player.c (1)

345-346: Acknowledge TODO for future refactoring.

The TODO comment appropriately flags the data flow issue where the position is updated first (line 351), then potentially reverted on collision (line 260 via has_collided). This pattern could be refactored to check collision before moving, though the current approach works correctly with the fix applied.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 609f9df and 4e2aeaf.

📒 Files selected for processing (1)

• src/player.c (2 hunks)

⏰ 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 (Clang)
• GitHub Check: Windows (MSVC)
• GitHub Check: Ubuntu (Clang)
• GitHub Check: Ubuntu (mingw)
• GitHub Check: Mac (GCC)
• GitHub Check: Ubuntu (GCC)
• GitHub Check: Analyze (c-cpp)

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Good fix, but verify type safety.

Using player_update_pos correctly synchronizes both the player sprite position and the bleed emitter position when reverting after collision, which fixes the bug mentioned in the PR title.

However, there's a type safety concern: this line passes negative values to a function that expects Uint32 parameters (see line 331). While this may work due to wraparound arithmetic, it relies on implicit conversions and is error-prone. Consider updating the function signature to accept signed integers.

🤖 Prompt for AI Agents

In src/player.c around line 260 (and the function definition at line 331), the
call to player_update_pos passes negative offsets but the function currently
expects Uint32 which causes unsafe implicit wraparound; update player_update_pos
to accept signed integers (e.g., int32_t or int) for both parameters, change its
prototype in the header and definition at/near line 331, update all callers to
pass signed values (remove casts to unsigned), and run a quick compile to fix
any type mismatches and ensure logic still behaves correctly.