fix(fob): reconnect troopPickupAtFOB to the F10 troop pickup path - #136
Merged
Conversation
troopPickupAtFOB (default true) sets a per-FOB flag read only by isInFOBTroopZone, which has zero callers in src/ - the F10 troop pickup path only consults CTLDZoneManager's TRZ_ zones, never FOBs. Undeclared legacy-parity regression, invisible to every test level. Grilled with the user 2026-08-26: fix mirrors the existing registerFOBAsLogistic/unregisterLogistic pattern for troop zones.
Single AFK ticket: registerFOBAsTroopZone/unregisterTroopZone, wiring into _registerDeployedFOB/_destroyFOB, extended deploy_managers_spec.lua tests, CHANGELOG entry, developer docs. Granularity confirmed with the user - one cohesive slice, no artificial docs-only split.
troopPickupAtFOB (default true) set a per-FOB flag read only by isInFOBTroopZone, which had zero callers in src/ - the F10 "Load Troops" menu only consults CTLDZoneManager's TRZ_ zones, never FOBs. Undeclared legacy-parity regression, invisible to every test level (migration/source/CTLD.lua:9676-9677, :10750-10761 fully wired the equivalent check via ctld.builtFOBS + ctld.inPickupZone()). Mirrors the existing registerFOBAsLogistic/unregisterLogistic pattern: CTLDZoneManager:registerFOBAsTroopZone/unregisterTroopZone synthesize and remove an unlimited-stock CTLDTroopZone for the FOB, wired into CTLDFOBManager:_registerDeployedFOB (gated by troopPickupAtFOB, same guard as the existing dead flag, which is left untouched for isInFOBTroopZone's own callers) and _destroyFOB (no ghost zone). Reuses the existing fobTroopPickupRadius setting - no new config. FIX-FOB-TROOP-PICKUP, grilled with the user 2026-08-26.
Reviewer's GuideRestores the default Sequence diagram for FOB troop pickup zone lifecyclesequenceDiagram
participant FOB as CTLDFOBManager
participant Zones as CTLDZoneManager
participant F10 as F10 Load Troops
FOB->>FOB: _registerDeployedFOB(scene)
alt troopPickupAtFOB is enabled
FOB->>Zones: registerFOBAsTroopZone(fobName, centroid, fobTroopPickupRadius, coalitionId)
Zones->>Zones: CTLDTroopZone:new(...)
end
F10->>Zones: getTroopZoneAtPoint(point, coalition)
Zones-->>F10: troop zone or nil
FOB->>FOB: _destroyFOB(fob, killerUnit, killerCoalition, integrityPercent)
FOB->>Zones: unregisterTroopZone(fob.name)
Zones-->>FOB: zone removed or no-op
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/CTLD_zone.lua" line_range="1205-1214" />
<code_context>
+-- @param point vec3
+-- @param radius number (default 150)
+-- @param coalitionId number
+function CTLDZoneManager:registerFOBAsTroopZone(fobName, point, radius, coalitionId)
+ local zone = CTLDTroopZone:new({
+ zoneName = fobName,
+ coalition = coalitionId or 0,
+ center = point,
+ radius = radius or 150,
+ pickMaxStock = 0, -- unlimited
+ active = true,
+ })
+ self._troopZones[fobName] = zone
+ ctld.utils.log("INFO", "CTLDZoneManager: FOB troop zone '%s' r=%dm", fobName, radius or 150)
+end
</code_context>
<issue_to_address>
**issue (broader_impact):** The FOB name is used as the key in `_troopZones` without checking whether a mission-defined or script-created troop zone already has the same name. Registering such an FOB overwrites the existing zone, and destroying the FOB then unconditionally removes that unrelated zone as well.
**Triggers:** When a mission-defined or scripted troop zone is named `Deployed FOB #<n>` and a deployed FOB receives the same generated name.
**Suggested fix:** Reject or namespace colliding FOB troop-zone names, or preserve and restore any pre-existing zone when registering and unregistering the FOB zone.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1205
to
+1214
| function CTLDZoneManager:registerFOBAsTroopZone(fobName, point, radius, coalitionId) | ||
| local zone = CTLDTroopZone:new({ | ||
| zoneName = fobName, | ||
| coalition = coalitionId or 0, | ||
| center = point, | ||
| radius = radius or 150, | ||
| pickMaxStock = 0, -- unlimited | ||
| active = true, | ||
| }) | ||
| self._troopZones[fobName] = zone |
There was a problem hiding this comment.
issue (broader_impact): The FOB name is used as the key in _troopZones without checking whether a mission-defined or script-created troop zone already has the same name. Registering such an FOB overwrites the existing zone, and destroying the FOB then unconditionally removes that unrelated zone as well.
Triggers: When a mission-defined or scripted troop zone is named Deployed FOB #<n> and a deployed FOB receives the same generated name.
Suggested fix: Reject or namespace colliding FOB troop-zone names, or preserve and restore any pre-existing zone when registering and unregistering the FOB zone.
Review findings (code-review, 8-angle pass on PR #136): - Reuse ctldTestSettings.borrow/:restore instead of a hand-rolled ctld.gs monkey-patch (tests/ci/helpers/settings.lua already exists for this, added by FIX-SPEC-ISOLATION). - Share one FOB_CENTROID constant between scene() and the new assertions instead of a second, independently-hardcoded literal. - Reset CTLDZoneManager.getInstance()._troopZones between the two halves of the isInFOBTroopZone regression test, matching the existing FOB-state reset - no stale zone left in the singleton after the file's tests run.
Two findings from the 8-angle review of PR #136, fixed here at the user's request rather than deferred: - registerFOBAsLogistic/registerFOBAsTroopZone now refuse (WARN log) to overwrite an existing zone sharing the FOB's name, matching the guard createExtractZone/createTroopZoneAtObject already have. Both gained a boolean return value for this. Previously a Mission-Editor zone with the same literal name as an auto-generated FOB could be silently clobbered with no log line. - CTLDTroopZone gained an optional displayName field (F10 label override, defaults to "TRZ_"..zoneName when absent, so every existing TRZ_/createTroopZoneAtObject/createExtractZone zone is unaffected). registerFOBAsTroopZone sets it to fobName, so the F10 "Load from ..." entry for a FOB shows its own name instead of a fabricated "TRZ_Deployed FOB #N". New tests: two collision-guard cases in deploy_managers_spec.lua, two menu-label cases in menu_gating_spec.lua (reusing its existing getTroopZonesForCoalition-stub seam).
5 tasks
FullGas1
added a commit
that referenced
this pull request
Aug 26, 2026
8-angle review of PR #137, findings applied: - CTLDSceneManager:packScene destroyed a FARP's objects with no cleanup of any troop zone/watcher registered for it, relying on an unverified DCS behavior (does destroying a packed static also flip a separately-resolved Airbase handle's isExist()?) that this suite cannot check. Only Countryside FARP supports packing; its onRepack (already called by packScene before destruction) now calls unregisterTroopZone/CTLDStaticWatcher:unwatch explicitly first. - CTLDStaticWatcher:watch now logs a WARN when it silently overwrites a still-live entry for the same id - a pre-existing gap in the shared cross-feature registry, cheap and consistent with the collision guard registerFOBAsTroopZone already has. New static_watcher_spec.lua covers watch/unwatch/tick and the WARN. - registerFARPTroopPickupFromScene: documented why it lives on CTLDZoneManager rather than a dedicated CTLDFARPManager (no such entity exists or is needed); removed a redundant ab:getName() call; added a WARN on its two previously-silent failure branches. - Tests: replaced a hand-rolled ctld.gs monkey-patch with ctldTestSettings.borrow/:restore (the same fix already applied on PR #136, missed again here); shared fakeHelipad/fakeAirbase/ctxFor across describe blocks instead of duplicating; trimmed unused unit/_params fields from the ctx fixture; fixed a miscounted step total in a test description string. FEAT-FARP-TROOP-PICKUP, PRD post-review addendum.
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
1 participant
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
troopPickupAtFOB(defaulttrue, schema: "Allow troop pickup at built FOBs") has had no in-game effect since the v2 rewrite: it sets a per-FOB flag read only byCTLDFOBManager:isInFOBTroopZone, which has zero callers anywhere else insrc/— the F10 "Load Troops" menu only consultsCTLDZoneManager'sTRZ_zones, never FOBs. Legacy (migration/source/CTLD.lua:9676-9677,:10750-10761) fully wires the equivalent check. Undeclared legacy-parity regression, invisible to every test level (grepped — none exercised the behavior).registerFOBAsLogistic/unregisterLogisticpattern: newCTLDZoneManager:registerFOBAsTroopZone/unregisterTroopZone, wired intoCTLDFOBManager:_registerDeployedFOB(sametroopPickupAtFOBguard, existingfob._troopPickupflag left untouched) and_destroyFOB(no ghost zone). Reuses the existingfobTroopPickupRadiussetting — no new config..backlog/FIX-FOB-TROOP-PICKUP/PRD.md); a related generic "zone ↔ owner link" idea was deliberately deferred todev/roadmap.mdrather than bundled here.Test plan
tests/ci/unit/deploy_managers_spec.lua's existingCTLDFOBManager deploy + destroyseam (TDD: written red against the missing methods, confirmed green after the fix): zone registered whentroopPickupAtFOB=true, none whenfalse, removed on FOB destruction (no ghost zone),isInFOBTroopZonebehavior unchanged.busted --pattern=_spec --helper=tests/ci/helpers/init.lua tests/ci→ 1328 passed / 0 failed / 1 pending (pre-existing, DCS-live gated).luac -p CTLD.lua— Lua 5.1 syntax OK (rebuilt viamerge_CTLD.ps1).CLAUDE.md, relies on CI).Summary by Sourcery
Reconnect built FOB troop pickup to the standard F10 troop-zone path while preserving existing FOB APIs and preventing stale or conflicting zones.
Bug Fixes:
troopPickupAtFOBis enabled, including cleanup when FOBs are destroyed.Enhancements:
Documentation:
Tests:
Chores: