fix(npc): restore behaviour lifecycle across runtimes - #258
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesCross-runtime reflection and dealer lifecycle updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NPCDealer
participant NativeRecruitmentDelegate
participant DealerStaticEvent
NPCDealer->>NativeRecruitmentDelegate: convert callback when required
NPCDealer->>DealerStaticEvent: subscribe platform-specific delegate
DealerStaticEvent-->>NPCDealer: invoke recruitment callback
NPCDealer->>DealerStaticEvent: remove platform-specific delegate
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
ifBars
changed the title
![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 `@S1API/Entities/NPCDealer.cs`:
- Around line 846-848: Update the event subscription logic around
_dealerRecruitedHandlers so every add operation creates and stores a distinct
native wrapper, including duplicate Action values. Ensure remove deletes only
one matching registration while retaining any remaining duplicates, and add a
compatibility test covering duplicate add followed by a single remove.
- Around line 748-762: Update the fallback creation in the
DealerAttendDealBehaviour setup to create its GameObject inactive, assign the
behavior’s beh owner and npcBehaviour.Npc references consistently with
NPCPrefabBuilder.SetBehaviourRefs, then activate it only after both references
are initialized. Preserve the existing activation state and naming/priority
assignments for the prefab path.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fdb5e36-b2b2-4c58-bc1c-79b1b689fcb6
📒 Files selected for processing (14)
S1API.Tests/Entities/DealerLifecyclePolicyTests.csS1API.Tests/Internal/Utils/ReflectionUtilsTests.csS1API/Entities/NPC.csS1API/Entities/NPCCustomer.csS1API/Entities/NPCDealer.csS1API/Entities/NPCPrefabBuilder.csS1API/Internal/Patches/NPCPatches.csS1API/Internal/Patches/TVPatches.csS1API/Internal/Utils/ReflectionUtils.csS1API/Items/ItemManager.csS1API/Law/LawController.csS1API/S1API.csS1API/S1API.csprojS1API/Vehicles/LandVehicle.cs
1 participant
Summary
NPCBehaviourmanages their internalEnabledandActivestatesNPCActionobjectsReflectionUtilsfor inherited instance/static members and backing fields3.1.14Root cause
Schedule I 0.4.6 moved customer deal attendance from the schedule-owned
NPCSignal_WaitForDeliverypath to theNPCBehaviourstack. S1API migrated the component types, but dealer, smoke-break, and graffiti behaviour objects were still created with the legacy schedule-action convention ofactiveSelf = false.The behaviour manager can select and tick a registered behaviour independently of its GameObject activation state. When
DealerAttendDealBehaviour.OnActiveTick()reached the handover, Unity rejected its coroutine because the component's GameObject was inactive.The adjacent dual-runtime audit also confirmed that several raw reflection paths assumed the Mono assembly shape. In the IL2CPP interop assemblies, those native fields are generated as managed properties, and native arrays/delegates require their IL2CPP representations.
The audit compared the 3.1.0 migration, current Mono and IL2CPP assemblies, and the upstream 0.4.6f11 change:
k073l/s1-codearchiver@3973cfc
Closes #257.
Cross-runtime audit
All raw
GetFieldcall sites in the API project were reviewed. Applicable game-wrapper mismatches were corrected for:npcandschedulereferencesRemaining raw field access is Mono-only, explicitly paired with an IL2CPP property path, or targets S1API/managed Unity types rather than generated game wrappers.
Validation
Mono
IL2CPP
Additional checks
git diff --checkpassed<Version>andMelonInfoboth report3.1.14Compatibility
NPCBehaviourstack and broken cross-runtime member accessRelease policy
After this PR merges into
stable, fast-forwardreleases/3.1.14to the stable merge commit and tag that exact shared commit asv3.1.14.Summary by CodeRabbit
Bug Fixes
Chores