Manual Combine Unloader — Call Unloader button for player-driven combines#1246
Manual Combine Unloader — Call Unloader button for player-driven combines#1246antler22 wants to merge 18 commits into
Conversation
Grain cart manual call button, US units, CP unload threshold dropped to 20%, pathfinding for grain cart
AIParameterSettingList.lua unit conversion stays in the personal fork/main but is not part of the manual combine unloader feature for upstream. Co-Authored-By: Claude Sonnet 4-6 <noreply@anthropic.com>
…f, remove unnecessary guards - Rename cpIsCallGrainCartActive -> cpIsManualCombineCallingUnloader per pvaiko's naming suggestion - Rename cpToggleCallGrainCart -> cpToggleManualUnloader for consistency - Rename CallGrainCartEvent -> CpManualUnloaderEvent (class + file) - Update translation key CP_callGrainCart -> CP_callManualUnloader; visible text now reads 'Call Unloader' - Rebase PurePursuitController.lua on current upstream base so PR diff shows only our actual additions (~35 lines) instead of the full file - Remove unnecessary if CollisionAvoidanceController / if ProximityController guards in AIDriveStrategyUnloadCombine.setAIVehicle() — these classes are always loaded - Revert MotorController.lua to upstream (changes were unrelated to this feature and crept in by mistake) Co-Authored-By: Claude Sonnet 4-6 <noreply@anthropic.com>
|
@pvaiko — reopening as a clean feature-only PR replacing #1243. All of your review feedback has been addressed:
Happy to discuss anything further or make adjustments. |
…C, use ImplementUtil.isChopper - Revert PathfinderConstraints.lua to upstream (getFruitCheckDimensions removed; change was too restrictive and geometrically unsound) - Revert translation_cz/ru/uk.xml to upstream (unintended edits) - Restore nil guard for spec.courseDisplay in CpCourseManager.lua (accidentally dropped) - Replace manual numAutoAimingStates chopper check with ImplementUtil.isChopper() in CpAIWorker.lua and CpFieldworkHudPage.lua - Add explanatory comment to CpAIFieldWorker:onUpdate() for the CP-active auto-deactivation logic - Simplify PPC off-track grace period: use g_time directly, remove onOffTrackShutdown() callback (pvaiko: AI slop) - Remove ppc.offTrackGracePeriodMs override and onOffTrackShutdown() from AIDriveStrategyUnloadCombine.lua; rely on existing disableStopWhenOffTrack() calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, clean up guards - CpManualCombineProxy: replace callUnloader() stub with isActiveCpCombine() returning true (correct approach per reviewer) - AIDriveStrategyCombineCourse: revert to upstream — combine strategy not installed when manually driving - PurePursuitController: revert to upstream — disableStopWhenOffTrack(5000) in the strategy is sufficient, grace period is redundant - PathfinderUtil: revert to upstream — swath detection unconfirmed, defer to future iteration - AIDriveStrategyUnloadCombine: remove do/end wrapper and self.ppc guard from disableStopWhenOffTrack call, fold into isManualProxy check; revert fill-level block to upstream; remove nil guards from harvesterStrategy calls in handleChopperTurn; remove redundant or-6 fallback on getWorkWidth(); handle manual blocker inline at blocking vehicle check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| -- combine stopped in the meanwhile, like for example end of course | ||
| if combineStrategy:willWaitForUnloadToFinish() then | ||
| self:debug('change to unload stopped combine') |
There was a problem hiding this comment.
can we please revert this to keep logging consistent (also, state is logged already)
| self:isInFrontAndAlignedToMovingCombine(true) | ||
| self:info('not in a good position to unload, cancelling rendezvous, trying to recover') | ||
| -- for some reason (like combine turned) we are not in a good position anymore then set us up again | ||
| self:info('unloadMovingCombine: EXIT - not in a good position (behind=%s, inFront=%s), startWaitingForSomethingToDo', |
There was a problem hiding this comment.
can we please revert this to keep logging consistent (also, state is logged already)
|
Thank you for addressing the comments so far, will continue the review as work permits... |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| @@ -61,25 +61,28 @@ function CollisionAvoidanceController:findPotentialCollisions() | |||
| if AIDriveStrategyCombineCourse.isActiveCpCombine(vehicle) then | |||
There was a problem hiding this comment.
and not vehicle:cpIsManualCombineCallingUnloader() here would be a lot cleaner and more readable than the check for strategy.
- Fix debug strings: 'Call grain cart' -> 'manual unloader' (CpAIFieldWorker) - getCombineStrategy(): proxy-first order, remove misleading comment - driveBesideCombine(): update doc comment; use ppc:getNormalLookaheadDistance() getter - PurePursuitController: add getNormalLookaheadDistance() getter - Add AGENTS.md: 'always use getters to access member variables' - setupFollowCourse(): move manual-combine placeholder logic here, use Course.createStraightForwardCourse - startCourseFollowingCombine(): remove now-inlined placeholder block and unused followCourseRefreshTime init - Revert startPathfindingToWaitingCombine to near-upstream (no isManualCombine param/tuning) - Revert call site and onPathfindingDoneToWaitingCombine redirect-timer init - Revert driveToCombine() straight-line redirect block - CollisionAvoidanceController: exclude manual combines from course-intersection check; revert to direct getCpDriveStrategy() call - getDriveData(): disable PPC off-track check unconditionally when manual proxy is active; remove redundant call from unloadMovingCombine Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the combine moves ≥30 m from the position recorded at pathfinding start, trigger a fast A* re-path to its new location. Guards prevent unnecessary stops: - checks only every 5 s - skips if <20 m of course remains (almost there) - skips if already within 25 m of combine (proximity handling takes over) - caps maxIterations at HybridAStar.defaultMaxIterations for speed - excludes the combine from obstacles so the path aims behind it Uses onPathfindingFailedToMovingTarget so a failure is a soft retry, not a job-stopper. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per pvaiko's review: CP-driven combines have their own rendezvous/waypoint system and must not be affected by the periodic re-pathfinding logic. Add isManualProxy guard so the re-dispatch only fires when the target is a CpManualCombineProxy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| -- • only trigger when the combine has actually moved ≥ 30 m (genuine relocation) | ||
| -- Uses a capped iteration count (defaultMaxIterations) so the A* search completes in | ||
| -- well under a second on any field size, minimising the WAITING_FOR_PATHFINDER stop. | ||
| local combineStrategyForRepath = self:getCombineStrategy() |
There was a problem hiding this comment.
I see this pattern in several variations in this file to check if this is a manually driven combine. Can we extract this into a function like isManualCombine or something for consistency?
| -- Uses a capped iteration count (defaultMaxIterations) so the A* search completes in | ||
| -- well under a second on any field size, minimising the WAITING_FOR_PATHFINDER stop. | ||
| local combineStrategyForRepath = self:getCombineStrategy() | ||
| if combineStrategyForRepath and combineStrategyForRepath.isManualProxy and |
There was a problem hiding this comment.
Extract this in a separate function to keep driveToCombine clean and consise.
…ndRepath() Per pvaiko review: - Add isManualCombine() to AIDriveStrategyUnloadCombine — single place to check whether the target is a CpManualCombineProxy; replaces five inconsistent isManualProxy field-access patterns across the file - Extract the driveToCombine re-path block into checkCombineRelocatedAndRepath() so driveToCombine() stays clean and concise - All callers updated to use self:isManualCombine() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| self:debug('Pathfinding to waiting combine successful') | ||
| course:adjustForReversing(math.max(1, -AIUtil.getDirectionNodeToReverserNodeOffset(self.vehicle))) | ||
| self:startCourse(course, 1) | ||
| -- Record the combine's position now so driveToCombine() can detect if it has moved |
There was a problem hiding this comment.
Can we move this to checkCombineRelocatedAndRepath() so everything is in one place?
There was a problem hiding this comment.
On a second thought, I don't think this is even needed, the logic is already in checkCombineRelocatedAndRepath
There was a problem hiding this comment.
Off-track shutdown should now be disabled for manual unloads.
|
For adding Translations, please refer to our automatic translation: |
One thing I found thats a bit problematic. |
- CpManualCombineProxy:update() now disables the PPC off-track check continuously while an unloader is registered, since the proxy's placeholder course is static and the combine moves dynamically. - AIDriveStrategyCourse:getPPC() getter added so the proxy can access the unloader's PPC from the combine side. - Removed the redundant isManualCombine() guard from AIDriveStrategyUnloadCombine:getDriveData() — proxy now owns this.
|
When call an unloader: |
…al combines - Guard unloaderVehicle.getCpDriveStrategy before calling in proxy update() to fix 'attempt to call missing method' error reported by Tensuko - Suppress PPC off-track shutdown in AIDriveStrategyUnloadCombine:update() whenever servicing a manual combine, covering the approach phase where the unloader is not yet registered with the proxy and drifts from the static dynamic course line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
2026-05-28 20:40:21.738 :21 [ppc controller lp1946] 7R 350 (Ernten 21)/211: PPC: prev 8 curr 9 |
|
Had a few hiccups with some of the fixes recently, as of this comment it should be repaired and ready to test again. I believe I have accounted for all comments above, except translations which will be handled at the end. |
Left the course issue is still there. |
The previous fix called disableStopWhenOffTrack() after AIDriveStrategyCourse.update(), but that parent call invokes ppc:update() which contains the cutout check — so the disable arrived one tick too late. Move the guard to the top of update(), before the parent call, so the flag is cleared before the PPC check runs each frame. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Still the same. |
|
You need to call it, drive far away enough and release it then. |
…mbine isManualCombine() returns false as soon as the proxy is deleted on release, so the disableStopWhenOffTrack() guard in update() stopped being called. The 2000ms timer then expired and PPC fired the cutout while the unloader was still far from the dynamic course during job wind-down. Fix: introduce a sticky self.servingManualCombine flag, set in call() when the combine has a manual proxy and cleared only in releaseCombine(). The update() guard now uses this flag so off-track stays suppressed for the full job lifetime regardless of proxy lifecycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Now it is working, nice. |
- Move unloader toggle from line 6 (spacer conflict) to line 1, left-aligned, sharing the row with the existing copy/paste icons on the right - Merge label + state into a single text element: "Unloader OFF" / "Unloader ACTIVE" - Fix color coding: switch from setColor() (no-op on text elements) to setTextColorChannels() — white when idle, green when actively calling - Hide unloader row while course cache text is displayed to avoid overlap - Rename label "Call Unloader" → "Unloader" in translation_en.xml - Add CP_callManualUnloader / Active / Inactive keys to MasterTranslations.xml with EN + DE so the auto-translation bot picks them up for all languages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Glad to hear it, I think I found a better setup for the HUD, moving it to the bottom. Also added translations now. |
|
Edit: If you did copy a course, the place is also used. |
|
On the copy course, yes. But I had the unloader text hidden if copy course is being used. Copy Course is priority, and often temporary, but Unloader text is supposed to hide in that case. I'll look into the MP, I don't play it myself. I assume no leads via errors? |
|
The log was gone because the Server did a complete restart. |
|
Please read, I'm hesitant to mess with the code outside of this feature, but this is what AI is finding for me... MP Crash DiagnosisConfidence: High. I can trace the crash to a specific code path. Root Cause: Infinite Event Echo Loop (Broadcast Storm)The crash is caused by a re-entrant call pattern between The loop, step by step: In a dedicated server or listen server with 3+ players, when Client B receives a broadcast of the toggle event from the server:
The proxy is toggled on/off at network speed, the event queue is flooded, and the server crashes. Why it triggers only in specific topologies
This explains why local testing (single player / listen server 2-player) seemed fine but a real multiplayer server crashed immediately. Secondary Bug: Host's Toggle Never Syncs to ClientsOn a listen server, when the host player presses the button:
The proxy works on the server but clients can't see it's active. Our Code Changes Are Not the SourceThe three code changes we made to the strategy and proxy are safe in MP:
Fix PlanAdd a -- CURRENT (buggy):
if not self.isServer then
CpManualUnloaderEvent.sendEvent(self)
end
-- FIXED:
if not noEventSend then
CpManualUnloaderEvent.sendEvent(self) -- sendEvent already routes: server→broadcast, client→send-to-server
endRemove the Pass function CpManualUnloaderEvent:run(connection)
if self.vehicle and self.vehicle.cpToggleManualUnloader then
self.vehicle:cpToggleManualUnloader(true) -- noEventSend=true: apply state, don't echo
end
if not connection:getIsServer() then
g_server:broadcastEvent(CpManualUnloaderEvent.new(self.vehicle), nil, connection, self.vehicle)
end
endThis is the same two-file pattern the upstream |
When a client received the toggle event as a broadcast from the server, CpManualUnloaderEvent:run() called cpToggleManualUnloader() which unconditionally called sendEvent() (guarded only by !isServer, which is false on any client). This re-sent the event to the server, which toggled the proxy back off and re-broadcast, creating an infinite ping- pong storm that crashed the server. Fix: add noEventSend parameter to cpToggleManualUnloader and pass true from the event handler, so the state is applied without triggering a network re-send. Also removes the !isServer guard so the listen-server host's toggle is now correctly broadcast to connected clients via the existing sendEvent() routing logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
This feature adds a "Call Unloader" button to manually-driven combines (i.e. combines that the player is driving themselves, not Courseplay-controlled). When activated, a nearby Courseplay-managed grain cart automatically:
The design goal is that the player only touches the button once — the grain cart handles everything until the pipe is closed.
Files Changed
scripts/ai/CpManualCombineProxy.luaAIDriveStrategyCombineCourseinterfacescripts/specializations/CpAIFieldWorker.luacpToggleManualUnloader/cpIsManualCombineCallingUnloadertoggle and proxy lifecyclescripts/specializations/CpAIWorker.luascripts/ai/strategies/AIDriveStrategyUnloadCombine.luascripts/ai/PurePursuitController.luascripts/pathfinder/PathfinderUtil.luahasFruit()config/VehicleSettingsSetup.xmlscripts/events/CpManualUnloaderEvent.luaKey Design Decisions
CpManualCombineProxyimplements the fullAIDriveStrategyCombineCourseinterface soAIDriveStrategyUnloadCombinecan interact with a manual combine using identical method calls — no nil checks or special-casing scattered through the unloader code.isManualProxy() → true— marker method used to gate manual-only behavior inAIDriveStrategyUnloadCombinewithout re-querying the vehicle.getFillLevelPercentage() → 1— always reports full so fill level never causes the cart to leave. The only valid exit isisUnloadFinished()(pipe closed for 2+ seconds).driveBesideCombine()direct goal point — for manual combines, a live goal point is computed every frame from the combine's AI direction node at the pipe's lateral offset,normalLookAheadDistancemeters ahead. As the combine turns, the node rotates and the cart follows naturally through curves and gentle S-bends. The placeholder course (built from combine's current heading) exists only to satisfy PPC initialisation and is never consulted for steering.PPC soft-recovery hook —
onOffTrackShutdown()inAIDriveStrategyUnloadCombinetransitions the cart to IDLE instead of callingstopCurrentAIJob. The proxy'scallUnloaderWhenNeeded()re-summons within ~2.5 s. The user never has to manually restart the grain cart.Off-track grace period —
offTrackGracePeriodMs = 10000(class default, unchanged). During active unloading,disableStopWhenOffTrack(5000)is called each tick, fully suppressing off-track detection while the cart is under the pipe.Forage harvesters excluded — button and keybind are hidden when
pipeSpec.numAutoAimingStates > 0. Their auto-aim spout geometry requires a separate implementation.Backward Compatibility
All changes are backward-compatible with CP-driven combines:
combineStrategy:isManualProxy()callUnloaderPercentslider change (min 60% → 20%) applies to all combinesKnown Limitations