Record last "line guess" at the end of navigation link with segmentation#10
Record last "line guess" at the end of navigation link with segmentation#10Egezenn wants to merge 1 commit into
Conversation
- Silkroad-Developer-Community/Silkroad-NavLink#1 - Untested, don't have a client to work with 😅
📝 WalkthroughWalkthrough
ChangesTarget-Based Movement Generation
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Botbases/RSBot.Training/Bot/NavigationManager.cs`:
- Around line 259-265: Remove the forced full GC call after storing the new
path: inside the lock that updates _activePath and _targetPosition and clears
_linkage (protected by _linkageLock), delete the GC.Collect() invocation so the
runtime can perform collection naturally; leaving the existing assignment
_linkage = null is sufficient to allow reclamation without pausing the bot
thread.
- Around line 476-487: The generated interpolated waypoints always use
start.Region, so moves that cross regions end up targeting the wrong region;
update the waypoint region when building interpolatedPos in the loop (e.g.
choose end.Region for the final segment or determine the correct region based on
the interpolated coordinates) so the rbsLines.Add string uses the correct region
for each interpolatedPos (refer to Position, start, end, interpolatedPos,
segments, and rbsLines.Add to locate and change the code).
🪄 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 Plus
Run ID: 48328f4f-546d-4c00-a8ae-913155fe90c9
📒 Files selected for processing (1)
Botbases/RSBot.Training/Bot/NavigationManager.cs
| lock (_linkageLock) | ||
| { | ||
| _activePath = path; | ||
| _targetPosition = targetPos; | ||
| _linkage = null; // Clear linkage data once path is cached to save memory | ||
| } | ||
| GC.Collect(); |
There was a problem hiding this comment.
Remove the forced full GC from path calculation.
GC.Collect() here runs on every successful path build, which can pause the bot thread and usually will not reclaim the linkage graph yet because this method still has live references to it. Clearing _linkage is enough; let the runtime collect naturally.
Suggested change
lock (_linkageLock)
{
_activePath = path;
_targetPosition = targetPos;
_linkage = null; // Clear linkage data once path is cached to save memory
}
- GC.Collect();
return true;📝 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.
| lock (_linkageLock) | |
| { | |
| _activePath = path; | |
| _targetPosition = targetPos; | |
| _linkage = null; // Clear linkage data once path is cached to save memory | |
| } | |
| GC.Collect(); | |
| lock (_linkageLock) | |
| { | |
| _activePath = path; | |
| _targetPosition = targetPos; | |
| _linkage = null; // Clear linkage data once path is cached to save memory | |
| } | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Botbases/RSBot.Training/Bot/NavigationManager.cs` around lines 259 - 265,
Remove the forced full GC call after storing the new path: inside the lock that
updates _activePath and _targetPosition and clears _linkage (protected by
_linkageLock), delete the GC.Collect() invocation so the runtime can perform
collection naturally; leaving the existing assignment _linkage = null is
sufficient to allow reclamation without pausing the bot thread.
| var segments = (int)Math.Ceiling(distance / 50.0); | ||
| for (var i = 1; i <= segments; i++) | ||
| { | ||
| var t = (float)i / segments; | ||
| var x = start.X + (end.X - start.X) * t; | ||
| var y = start.Y + (end.Y - start.Y) * t; | ||
| var z = start.ZOffset + (end.ZOffset - start.ZOffset) * t; | ||
|
|
||
| var interpolatedPos = new Position(x, y, start.Region) { ZOffset = z }; | ||
| rbsLines.Add( | ||
| $"move {interpolatedPos.XOffset} {interpolatedPos.YOffset} {interpolatedPos.ZOffset} {interpolatedPos.Region.X} {interpolatedPos.Region.Y}" | ||
| ); |
There was a problem hiding this comment.
Don't segment moves across region boundaries.
Line 484 builds every interpolated waypoint with start.Region, so a long move that crosses into another region never emits end.Region. The last generated move still targets the source region, which can make the final hop wrong or unreachable.
Suggested guard
private static void AddMoveCommands(List<string> rbsLines, Position start, Position end)
{
var distance = start.DistanceTo(end);
if (distance < 1)
return;
+ if (start.Region.X != end.Region.X || start.Region.Y != end.Region.Y)
+ {
+ rbsLines.Add($"move {end.XOffset} {end.YOffset} {end.ZOffset} {end.Region.X} {end.Region.Y}");
+ return;
+ }
+
if (distance <= 50)
{
rbsLines.Add($"move {end.XOffset} {end.YOffset} {end.ZOffset} {end.Region.X} {end.Region.Y}");
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Botbases/RSBot.Training/Bot/NavigationManager.cs` around lines 476 - 487, The
generated interpolated waypoints always use start.Region, so moves that cross
regions end up targeting the wrong region; update the waypoint region when
building interpolatedPos in the loop (e.g. choose end.Region for the final
segment or determine the correct region based on the interpolated coordinates)
so the rbsLines.Add string uses the correct region for each interpolatedPos
(refer to Position, start, end, interpolatedPos, segments, and rbsLines.Add to
locate and change the code).
1 participant
Adresses Silkroad-Developer-Community/Silkroad-NavLink#1