fix: keep a custom NPC's avatar on until it is spawned

[r-melvin]

Summary

NPC.SetVisible(false) switches the Avatar object off. When an S1API NPC spawns, PrepareForNetworkSpawn checks that native Awake will find that Avatar, and refuses the spawn if not:

[ERROR] [NPC] Refusing to spawn custom NPC 'bp_escort_midnight_pres' because its native Awake reference graph is invalid: Avatar(active) ...

The Big Pimpin 1.0.11 creates its escort NPCs at scene load. On the second load of a session its escort setup hides them before S1API spawns them (trace: NPC.SetVisible <- EscortSlotBase.HideAtHiddenPosition, on Mono), so all four were refused.

The change. A new patch, NPCHideBeforeSpawnPatch, ignores SetVisible(false) on an S1API NPC whose own NetworkObject is not spawned yet. FinalizeNetworkSpawn already applies the intended visibility after the spawn, and the NPC can be hidden normally from then on. It's in its own file.

This relies on #338: without it, such NPCs have no NetworkObject of their own before spawn, and the patch can't tell they're unspawned.

Compatibility

• Public/protected API: none changed.
• Existing defaults and behavior: a hide on an unspawned S1API NPC is no longer applied. Showing, non-S1API NPCs and spawned NPCs are unaffected.
• Stable IDs, saves, and network payloads: none touched.

Validation

Mono

dotnet build S1API.sln -c MonoMelon --no-restore -p:AutomateLocalDeployment=false: 0 errors, 0 warnings. dotnet test ... -c MonoMelon: 743 passed (740 on beta plus 3 new: which hides are ignored, and that the patched game method exists).

IL2CPP

dotnet build S1API.sln -c Il2CppMelon --no-restore -p:AutomateLocalDeployment=false: 0 errors, 0 warnings. dotnet test ... -c Il2CppMelon: 723 passed (720 plus 3 new).

Runtime evidence

How it was tested. Automated runs on 0.4.7f7 load a save, teleport to each custom NPC, record whether its model is shown and its network state, return to the menu and load again. They used a combined build of #332 to #339.

• Same mods on both runtimes: IL2CPP and Mono "Alternate", with BigWillyMod, The Big Pimpin, Drug Expansion, S1MAPI and SteamNetworkLib. IL2CPP ran without Polyfill, on the game's own interop assembly.
• IL2CPP with about 60 mods: including Polyfill, Siesta and S1UMF.
• Saves: an early save and a late one where The Big Pimpin's intro is played through.

I also played the IL2CPP case by hand.

• Mono, second load: with fix: keep a custom NPC's NetworkObject when it is created before the network starts #338 alone the escorts were refused. With this change as well, the same hide calls happen and all four escorts spawn active on both loads.
• IL2CPP: the same result. fix: keep a custom NPC's NetworkObject when it is created before the network starts #338 alone was already enough there, so this change isn't separately shown on IL2CPP.

Documentation

XML <remarks> on the patch class.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 42 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ee645e81-7357-40ad-91bf-321753a8a23d

📥 Commits

Reviewing files that changed from the base of the PR and between db1de39 and 6159044.

📒 Files selected for processing (2)

• S1API.Tests/NPCs/NPCHideBeforeSpawnTests.cs
• S1API/Internal/Patches/NPCHideBeforeSpawnPatch.cs

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[r-melvin]

Depends on #338: without it, an NPC created before the network starts has no NetworkObject of its own, so this patch can't tell it hasn't spawned yet. Please merge #338 first.

[ifBars]

I can see why keeping the Avatar active prevents the reported spawn refusal, but I don't think dropping these visibility calls is the right fix as written.

The custom NPC guidance says to let S1API own instancing, configure defaults in ConfigurePrefab, and do runtime setup in OnCreated. Big Pimpin 1.0.11 instead manually constructs its four escorts from its Main-scene callback, then allows native manipulation based on registry presence. I've described that construction path in my review on #338.

There is also a concrete reload-order concern in its IL2CPP assembly: it constructs replacement escorts before releasing the previous display leases. Releasing an old lease calls HideAtHiddenPosition(), which resolves the native NPC by its reused slot ID. That can target a newly registered escort before it spawns. The method disables movement, navigation, schedules and behavior, then calls SetVisible(false, false), without a spawn-readiness guard. This is static evidence of an unsafe sequence; I haven't independently reproduced the exact Mono trace.

This prefix only masks the visibility part of that sequence. It returns false without recording the requested state, so it skips the native visibility update and callback. FinalizeNetworkSpawn uses IsPhysical and supplier meeting state; it does not replay the discarded hide request. A physical escort can consequently be made visible even though its owner requested that it remain hidden. The PR description's claim that the caller's intended visibility is applied later isn't supported by this implementation.

The guard also applies whenever IsSpawned is false, rather than establishing that the NPC is awaiting its initial S1API spawn or still needs native Awake. The tests check that predicate and the method's existence, but not preservation of the requested final visibility.

I'd prefer fixing Big Pimpin's instance ownership, reload cleanup and readiness checks. If there is a failure through S1API's documented lifecycle as well, please show a minimal reproduction. Any framework safeguard should protect that specific initialization phase and preserve the requested visibility instead of silently discarding it, with separate Mono/IL2CPP and host/client evidence.

I'm leaving this open and am happy to look at another approach. Successful spawning in the combined build isn't enough to justify changing visibility semantics for every unspawned S1API NPC.

[r-melvin]

You're right, and the PR description was wrong on a key point. It says FinalizeNetworkSpawn applies the intended visibility after the spawn. It doesn't: it calls SetVisible(ShouldBeVisibleAfterSpawn(), ...), which looks only at IsPhysical and the supplier meeting state. So a hide this prefix discards is never replayed, and a physical escort its owner wanted hidden ends up visible. Sorry for stating that without checking it.

I also checked your reload-order point against Big Pimpin 1.0.11's decompiled IL2CPP assembly, and it holds:

1. ResetForSceneReload() constructs the replacement escorts first.
2. EscortLocationDisplayController.HideAll() then releases the old leases. Each ReleaseLease calls slot.HideAtHiddenPosition().
3. That resolves NPCManager.GetNPC(SlotId), by then the newly registered, unspawned escort. It disables movement, the agent, schedules and behaviour, then calls SetVisible(false, false), with no spawn-readiness check.

The minimal reproduction I posted on #338 has no hide before spawn: one documented NPC, unmodified beta, IL2CPP and Mono, initial load and reload, host only. So there's no framework failure to guard against, and changing visibility semantics for every unspawned S1API NPC isn't justified. Closing this.

The fix belongs in Big Pimpin's escort code, which I'll do in S1UMF:

• release the old leases before constructing replacements;
• defer a hide on an escort that hasn't spawned until it has, keeping the requested state rather than dropping it.