small changes#60
Conversation
📝 WalkthroughWalkthroughThe changes update the game version to 5.46 and refactor the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 2210-2215: The auto-collect fallback is incorrectly reusing
self.collect_interval as a per-packet delay causing long stalls; add a new field
(e.g., collect_packet_delay_ms: u64 with default 0 or small value) to the
struct/initializers and use it for per-packet pacing inside collect() instead of
delay.unwrap_or(self.collect_interval), keep self.collect_interval strictly as
the tick cadence used by run(), and ensure the radius override uses .clamp(1,5)
on the incoming radius (replace
radius.unwrap_or(self.collect_radius_tiles.clamp(1,5)) with clamping the
provided radius) so user-supplied values are bounded; retain MAX_PER_TICK but
honor the new collect_packet_delay_ms between ItemActivateObjectRequest packets
to avoid blocking run().
- Around line 2289-2291: The loop sends packets while holding the cap check
stale and sleeps after each packet, causing up to MAX_PER_TICK+1 sends and large
run-loop stalls; update the cap condition to use >= (change checks that read `if
sent > MAX_PER_TICK` to `if sent >= MAX_PER_TICK`) so you never exceed
MAX_PER_TICK, and move the sleep out of the inner per-uid loop in
send_game_packet/sleep_ms usage: perform sleep_ms(delay) only once per
tile-group (i.e., after finishing the group of uids) or after the outer loop
iteration rather than after every individual uid to avoid per-packet stalls
while keeping sent increment logic (sent variable and send_game_packet calls)
unchanged.
- Around line 2492-2494: The handler for Req::Collect is unsafely casting
ScriptRequest::Collect.range (f32) to u8, which can wrap or silently truncate;
update the code in the Req::Collect match arm so you first clamp the float into
the allowed numeric bounds and then convert to u8 before calling self.collect:
e.g. compute let clamped = range.clamp(1.0_f32, 5.0_f32); let radius = clamped
as u8; then call Rep::CollectCount(self.collect(Some(radius),
Some(interval_ms))); alternatively, if fractional semantics are required, apply
an explicit rounding method (floor/ceil/round) to clamped before casting, or
change the ScriptRequest::Collect.range type to u8 and validate at the script
boundary instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a64898d6-2a24-4643-a6d2-ea2bd7aacc47
📒 Files selected for processing (2)
src/bot/core.rssrc/constants.rs
| let radius_tiles = radius.unwrap_or(self.collect_radius_tiles.clamp(1, 5)); | ||
| let r_px = radius_tiles as f32 * 32.0; | ||
| const MAX_PER_TICK: usize = 32; // cap packets per call | ||
|
|
||
| let delay = delay.unwrap_or(self.collect_interval); | ||
|
|
There was a problem hiding this comment.
Default delay reuses collect_interval (the inter-tick interval), causing auto-collect to stall the bot for seconds.
self.collect_interval is documented as “Interval in ms between auto-collect ticks (default 500)” (line 174–175), not a per-packet pacing delay. With this fallback:
run()callsself.collect(None, None)(line 743), which now doessleep_ms(500)after everyItemActivateObjectRequest.- With
MAX_PER_TICK = 32, a single auto-collect invocation can block for ~16 seconds. - During that block the run loop’s command intake, ping update, and
reconnect_afterchecks at the top ofrun()are skipped (onlyservice_onceruns insidesleep_ms). collect_timeris reset before the call (line 742), so the “every 500 ms” cadence is destroyed — the next tick effectively fires immediately after the previous one returns.
Also note the override path at line 2210 — radius.unwrap_or(self.collect_radius_tiles.clamp(1, 5)) — only clamps the configured value; a user-supplied radius is used unclamped (the Req::Collect path even truncates range as u8, so 256→0 silently).
Suggested direction: introduce a separate field (e.g. collect_packet_delay_ms, default 0 or a small value like 10–50) for the per-packet pacing, keep collect_interval as the tick cadence, and clamp the override.
🛠 Proposed fix sketch
- let radius_tiles = radius.unwrap_or(self.collect_radius_tiles.clamp(1, 5));
+ let radius_tiles = radius
+ .unwrap_or(self.collect_radius_tiles)
+ .clamp(1, 5);
let r_px = radius_tiles as f32 * 32.0;
const MAX_PER_TICK: usize = 32; // cap packets per call
- let delay = delay.unwrap_or(self.collect_interval);
+ // Per-packet pacing — separate from the auto-collect tick interval.
+ let delay = delay.unwrap_or(self.collect_packet_delay_ms);And in the struct/initializers, add a new collect_packet_delay_ms: u64 (default e.g. 0) instead of overloading collect_interval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/core.rs` around lines 2210 - 2215, The auto-collect fallback is
incorrectly reusing self.collect_interval as a per-packet delay causing long
stalls; add a new field (e.g., collect_packet_delay_ms: u64 with default 0 or
small value) to the struct/initializers and use it for per-packet pacing inside
collect() instead of delay.unwrap_or(self.collect_interval), keep
self.collect_interval strictly as the tick cadence used by run(), and ensure the
radius override uses .clamp(1,5) on the incoming radius (replace
radius.unwrap_or(self.collect_radius_tiles.clamp(1,5)) with clamping the
provided radius) so user-supplied values are bounded; retain MAX_PER_TICK but
honor the new collect_packet_delay_ms between ItemActivateObjectRequest packets
to avoid blocking run().
| self.send_game_packet(&pkt, true); | ||
| self.sleep_ms(delay); | ||
| sent += 1; |
There was a problem hiding this comment.
Per-packet sleep_ms(delay) runs inside the inner per-object loop while holding the cap check stale.
Two related concerns at this site:
- The cap check uses
if sent > MAX_PER_TICK(lines 2266, 2270). With>rather than>=, the loop can sendMAX_PER_TICK + 1 = 33packets before breaking. Pre-existing, but newly relevant because each excess packet now also sleeps fordelayms. - Sleeping after every packet here (rather than between groups, or at the end) magnifies the stall noted in the comment on lines 2210–2215. Even a modest
delayof 100 ms × 33 packets = 3.3 s of blocked run-loop work.
Consider >= for the cap, and/or sleeping only between distinct tile groups instead of after every individual uid.
♻️ Proposed cap fix
- if sent > MAX_PER_TICK { break; }
+ if sent >= MAX_PER_TICK { break; }
// fx, fy = the object actual coordinate
for (fx, fy, uid, item_id) in objs {
- if sent > MAX_PER_TICK { break; }
+ if sent >= MAX_PER_TICK { break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/core.rs` around lines 2289 - 2291, The loop sends packets while
holding the cap check stale and sleeps after each packet, causing up to
MAX_PER_TICK+1 sends and large run-loop stalls; update the cap condition to use
>= (change checks that read `if sent > MAX_PER_TICK` to `if sent >=
MAX_PER_TICK`) so you never exceed MAX_PER_TICK, and move the sleep out of the
inner per-uid loop in send_game_packet/sleep_ms usage: perform sleep_ms(delay)
only once per tile-group (i.e., after finishing the group of uids) or after the
outer loop iteration rather than after every individual uid to avoid per-packet
stalls while keeping sent increment logic (sent variable and send_game_packet
calls) unchanged.
| Req::Collect { range, interval_ms } => { | ||
| Rep::CollectCount(self.collect()) | ||
| Rep::CollectCount(self.collect(Some(range as u8), Some(interval_ms))) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ScriptRequest::Collect to see the declared type of `range` and `interval_ms`.
fd -t f script_channel.rs --exec rg -nP -C2 'Collect\s*\{|CollectObject\s*\{' {}
echo "---"
# Show the full enum if found
fd -t f script_channel.rs --exec cat {}Repository: CLOEI/Mori
Length of output: 3338
🏁 Script executed:
#!/bin/bash
# Find and inspect the collect method definition
rg -n "fn collect" src/bot/core.rs -A 10Repository: CLOEI/Mori
Length of output: 1034
🏁 Script executed:
#!/bin/bash
# Search for collect_radius_tiles and related clamping logic
rg -n "collect_radius_tiles" src/bot/core.rs -B 2 -A 2
echo "---"
# Also search for SetCollectRange to see if there's validation
rg -n "SetCollectRange" src/bot/core.rs -B 2 -A 5Repository: CLOEI/Mori
Length of output: 2871
Verify field type and address the unsafe as u8 cast—a safety net exists but incomplete.
ScriptRequest::Collect.range is f32, but the handler at lines 2492–2494 casts it as u8. This loses fractional values (e.g., 5.7 → 5) and causes wrapping (e.g., 256 → 0). While line 2210 includes clamp(1, 5) on the resulting u8, so 0 becomes 1, this doesn't fully salvage the intent: a script sending range = 256 silently becomes radius = 1 instead of clamping first.
Either cast and clamp before passing:
Some((range as u8).clamp(1, 5))Or change the field to u8 with validation at the script boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/core.rs` around lines 2492 - 2494, The handler for Req::Collect is
unsafely casting ScriptRequest::Collect.range (f32) to u8, which can wrap or
silently truncate; update the code in the Req::Collect match arm so you first
clamp the float into the allowed numeric bounds and then convert to u8 before
calling self.collect: e.g. compute let clamped = range.clamp(1.0_f32, 5.0_f32);
let radius = clamped as u8; then call
Rep::CollectCount(self.collect(Some(radius), Some(interval_ms))); alternatively,
if fractional semantics are required, apply an explicit rounding method
(floor/ceil/round) to clamped before casting, or change the
ScriptRequest::Collect.range type to u8 and validate at the script boundary
instead.
|
Thank you for your hard work!! |
2 participants
Summary by CodeRabbit
New Features
Chores