fix toggleable tile#59
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bot/core.rs (1)
2041-2045: Replaceaction_type == 31with a named constant or enum.The bare
31hides protocol meaning in the handler. Pulling it into a named item-action constant would make this logic much easier to audit and reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/core.rs` around lines 2041 - 2045, Replace the magic literal 31 in the match on item_info.action_type with a named constant or enum variant (e.g., an ItemAction::Switcheroo variant or ITEM_ACTION_SWITCHEROO constant) so the intent is explicit; update the match arm that currently uses the literal 31 to match the new enum variant or constant and keep the body (tile.flags.toggle(TileFlags::IS_ON)) unchanged, and add the constant/enum definition next to other item-action definitions so other handlers can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bot/core.rs`:
- Around line 2023-2046: The toggle logic currently mutates only world.tile_map
(tile.flags.toggle(TileFlags::IS_ON)) in the TOGGLEABLE_ITEM_BY_PUNCH branch and
in the action_type 31 branch, so update the same flag in the cached UI state:
compute the same index (idx) into self.state.tiles (the tile entry corresponding
to world.tile_map) and mirror the toggle there as well (toggle the IS_ON bit on
the corresponding self.state.tiles[idx] or set/clear its flag field accordingly)
right after each place that calls tile.flags.toggle(...) so the web/shared view
stays in sync with world changes.
---
Nitpick comments:
In `@src/bot/core.rs`:
- Around line 2041-2045: Replace the magic literal 31 in the match on
item_info.action_type with a named constant or enum variant (e.g., an
ItemAction::Switcheroo variant or ITEM_ACTION_SWITCHEROO constant) so the intent
is explicit; update the match arm that currently uses the literal 31 to match
the new enum variant or constant and keep the body
(tile.flags.toggle(TileFlags::IS_ON)) unchanged, and add the constant/enum
definition next to other item-action definitions so other handlers can reuse it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+2023
to
+2046
| let tile_map = &mut world.tile_map; | ||
| let idx = (pkt.int_y as u32) * tile_map.width + (pkt.int_x as u32); | ||
|
|
||
| let Some(tile) = tile_map.tiles.get_mut(idx as usize) else { | ||
| return; | ||
| }; | ||
|
|
||
| // TODO: find better way than this ugly thing | ||
| const TOGGLEABLE_ITEM_BY_PUNCH: [u32; 7] = [1226, 6172, 6398, 8348, 8350, 8806, 8808]; | ||
| if TOGGLEABLE_ITEM_BY_PUNCH.contains(&(tile.fg_item_id as u32)) { | ||
| tile.flags.toggle(TileFlags::IS_ON); | ||
| } | ||
|
|
||
| let Some(item_info) = self.items_dat.find_by_id(tile.fg_item_id as u32) else { | ||
| return; | ||
| }; | ||
|
|
||
|
|
||
| match item_info.action_type { | ||
| 31 => { // switcheroo | ||
| tile.flags.toggle(TileFlags::IS_ON); | ||
| } | ||
| _ => {} | ||
| } |
Contributor
There was a problem hiding this comment.
Mirror the toggle into self.state as well.
Line 2033 and Line 2043 only mutate self.world. Unlike the other tile handlers, this never updates self.state.tiles, so the web/shared view will keep the old on/off state until a full map reload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/core.rs` around lines 2023 - 2046, The toggle logic currently mutates
only world.tile_map (tile.flags.toggle(TileFlags::IS_ON)) in the
TOGGLEABLE_ITEM_BY_PUNCH branch and in the action_type 31 branch, so update the
same flag in the cached UI state: compute the same index (idx) into
self.state.tiles (the tile entry corresponding to world.tile_map) and mirror the
toggle there as well (toggle the IS_ON bit on the corresponding
self.state.tiles[idx] or set/clear its flag field accordingly) right after each
place that calls tile.flags.toggle(...) so the web/shared view stays in sync
with world changes.
Owner
|
haha i agree we need to find a better way instead of hard coded id, overall nice job! thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit