[codex] Improve replanning and moving target following#1
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR extends Navigator:MoveTo with an opt-in “follow moving goal” mode while also fixing internal module require paths for packaged consumers and updating demos/tests to better exercise replanning behavior.
Changes:
- Add moving-goal repath logic (threshold + interval gated) and consume-once replanning reasons in
Navigator. - Fix internal
require(...)paths inPlanner,Coordinator, andNavigatorfor packaged runtime layouts. - Add targeted unit tests and refresh the demo scene setup for more reliable blocked-path replans.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/specs/NavigatorReplan.spec.luau | Adds coverage for consuming/clearing pending replan reasons. |
| test/specs/MovingGoalPolicy.spec.luau | Adds coverage for the moving-goal repath decision policy. |
| test/run.luau | Registers the new specs in the test runner. |
| src/Planner/init.luau | Updates internal Goal require path for packaged runtime. |
| src/Navigator/init.luau | Implements moving-goal repathing + replan-reason consumption integration. |
| src/Navigator/PrepareReplan.luau | Introduces helper to consume and clear pending replan reason. |
| src/Navigator/MovingGoalPolicy.luau | Adds a policy module to decide when a moving goal should trigger repathing. |
| src/Coordinator/init.luau | Updates internal Shared/* require paths for packaged runtime. |
| example/src/ServerScriptService/Bootstrap.server.luau | Refreshes demo NPC creation + blocked-scene layout and repath trigger. |
| README.md | Documents moving-target following options and tuning notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local currentGoalPosition, goalError = self:_resolveGoal(run) | ||
| if currentGoalPosition == nil then | ||
| return false, goalError | ||
| end | ||
|
|
||
| local shouldRepath = MovingGoalPolicy.shouldRepath({ | ||
| enabled = run.moveOptions.followMovingGoal == true, | ||
| goalType = typeof(run.goal), |
There was a problem hiding this comment.
_shouldRepathForMovingGoal calls _resolveGoal(run) unconditionally, even when followMovingGoal is disabled or the goal isn’t an Instance. This adds extra coordinator/spacing work on every poll and also changes behavior when an Instance goal becomes invalid mid-run (it will now surface during waypoint waits). Consider early-returning before resolving the goal unless moving-goal following is enabled and applicable, so static-goal navigation keeps its previous behavior/perf profile.
| local currentGoalPosition, goalError = self:_resolveGoal(run) | |
| if currentGoalPosition == nil then | |
| return false, goalError | |
| end | |
| local shouldRepath = MovingGoalPolicy.shouldRepath({ | |
| enabled = run.moveOptions.followMovingGoal == true, | |
| goalType = typeof(run.goal), | |
| if run.moveOptions.followMovingGoal ~= true or typeof(run.goal) ~= "Instance" then | |
| return false, nil | |
| end | |
| local currentGoalPosition, goalError = self:_resolveGoal(run) | |
| if currentGoalPosition == nil then | |
| return false, goalError | |
| end | |
| local shouldRepath = MovingGoalPolicy.shouldRepath({ | |
| enabled = true, | |
| goalType = "Instance", |
| replanCount = 0, | ||
| nextWaypointIndex = 1, | ||
| lastGoalPosition = nil, | ||
| lastObservedGoalPosition = nil, |
There was a problem hiding this comment.
lastObservedGoalPosition is written and initialized, but it’s never read anywhere in the codebase. This makes the run state harder to reason about and suggests either dead state or a missing use (e.g., exposing it via GetDebugSnapshot). Consider removing it entirely or wiring it into the intended consumer.
| lastObservedGoalPosition = nil, |
Summary
Navigator:MoveTowith drift and interval controlsWhy
maxReplansincorrectlyImpact
BasePartandAttachmentgoals without changing static-goal behaviorPlanner,Coordinator, andNavigatorValidation
stylua --check src test exampleselene src test examplelune run test/run.luaurojo build default.project.json --output /tmp/PathfindingPlus.rbxlx