Conversation
Schedule 1 v0.4.4f6 introduced a new `ScheduleOne.Core` assembly, splitting base item/entity types out of `Assembly-CSharp`.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request migrates the S1API codebase from S1ItemFramework to S1CoreItemFramework for item category and legal status enums, removes public API methods for label color and keyword customization, updates NPC revival mechanics to use local reflection-based state changes instead of networked calls, adds ScheduleOne.Core dependencies, and updates audio management to use MusicManager instead of MusicPlayer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@S1API/Entities/NPC.cs`:
- Around line 1722-1723: The public Panic() should check the server context
before calling the server-only method: modify NPC.Panic() to call SafeIsServer()
and only invoke S1NPC.SetPanicked_Server() when SafeIsServer() returns true
(otherwise no-op or early return); locate the Panic() method and replace the
direct call to S1NPC.SetPanicked_Server() with this guard to prevent client-side
invocation from reaching server-only code.
In `@S1API/Internal/Patches/NPCPatches.cs`:
- Around line 1582-1605: The patch currently always returns false (skipping
original revive) even when the local revive operations fail; change the method
so it only returns false when all local operations succeed. Capture the boolean
results from Utils.ReflectionUtils.TrySetFieldOrProperty calls for
"<Health>k__BackingField", "IsDead", and "IsKnockedOut", and check that
baseNpc.Behaviour.DeadBehaviour?.Disable(),
baseNpc.Behaviour.UnconsciousBehaviour?.Disable(), and
__instance.onRevive?.Invoke() execute without error; if any reflection call
returns false or an exception is thrown, log the failure and return true to
allow the original revive path to run, otherwise return false to skip the
original. Use the existing symbols (__instance,
Utils.ReflectionUtils.TrySetFieldOrProperty, baseNpc.Behaviour,
__instance.onRevive) when implementing the checks.
In `@S1API/S1API.csproj`:
- Around line 58-59: The Il2Cpp reference for Il2CppScheduleOne.Core.dll
currently uses a bare Reference Include ("Il2CppScheduleOne.Core.dll") and needs
an explicit HintPath like the Mono entries; update the Reference element that
uses $(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll (the Reference Include
for Il2CppScheduleOne.Core) to the same pattern used by the Mono references by
adding a nested HintPath that points to
$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll so MSBuild can reliably
resolve the assembly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07660fae-102a-433f-81fc-f30181430b6a
📒 Files selected for processing (12)
S1API/Entities/NPC.csS1API/Growing/SeedCreator.csS1API/Internal/Patches/LoadingScreenPatches.csS1API/Internal/Patches/NPCPatches.csS1API/Items/AdditiveDefinitionBuilder.csS1API/Items/BuildableItemDefinition.csS1API/Items/BuildableItemDefinitionBuilder.csS1API/Items/ClothingItemDefinitionBuilder.csS1API/Items/ItemDefinition.csS1API/Items/StorableItemDefinitionBuilder.csS1API/S1API.csprojS1API/Shops/Shop.cs
💤 Files with no reviewable changes (1)
- S1API/Items/BuildableItemDefinition.cs
S1API/Entities/NPC.cs
Outdated
| public void Panic() => | ||
| S1NPC.SetPanicked(); | ||
| S1NPC.SetPanicked_Server(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites of NPC.Panic() =="
rg -n --type=cs -C3 '\.Panic\s*\('
echo
echo "== Direct uses of SetPanicked_Server() =="
rg -n --type=cs -C4 '\bSetPanicked_Server\s*\('
echo
echo "== Legacy SetPanicked() usages (if any remain) =="
rg -n --type=cs -C2 '\bSetPanicked\s*\('Repository: ifBars/S1API
Length of output: 87
🏁 Script executed:
find . -name "NPC.cs" -type fRepository: ifBars/S1API
Length of output: 79
🏁 Script executed:
wc -l ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 84
🏁 Script executed:
sed -n '1720,1730p' ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 415
🏁 Script executed:
rg -n --type=cs 'SafeIsServer' -A2 -B2Repository: ifBars/S1API
Length of output: 270
🏁 Script executed:
sed -n '1535,1545p' ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 334
🏁 Script executed:
rg -n 'Panic()' ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 787
🏁 Script executed:
rg -n --type=cs '\.Panic\(\)' --glob='!NPC.cs'Repository: ifBars/S1API
Length of output: 38
🏁 Script executed:
rg -n 'Panic\(' --type=cs --glob='!NPC.cs'Repository: ifBars/S1API
Length of output: 38
🏁 Script executed:
rg -n 'SetPanicked_Server' --type=csRepository: ifBars/S1API
Length of output: 122
🏁 Script executed:
rg -n 'SetPanicked_Server' --type=cs -A2 -B2Repository: ifBars/S1API
Length of output: 307
🏁 Script executed:
grep -n 'SafeIsServer()' ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 103
🏁 Script executed:
rg -n '_Server\(\)' --type=cs ./S1API/Entities/NPC.cs | head -20Repository: ifBars/S1API
Length of output: 100
🏁 Script executed:
sed -n '1700,1750p' ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 2103
🏁 Script executed:
sed -n '1700,1780p' ./S1API/Entities/NPC.csRepository: ifBars/S1API
Length of output: 3363
🏁 Script executed:
rg -n 'SafeIsServer\(\)' --type=cs -B5 -A5Repository: ifBars/S1API
Length of output: 722
🏁 Script executed:
rg -n 'SafeIsServer()' --type=cs -v 'private static bool'Repository: ifBars/S1API
Length of output: 115
Add server-context guard to Panic() method to prevent client-side breakage.
The Panic() method is a public API that directly calls SetPanicked_Server(), a server-only method. This creates a mismatch: the public method can be invoked from client contexts, but the underlying implementation will fail outside server execution. The codebase already provides SafeIsServer() for exactly this purpose.
Suggested safeguard
- public void Panic() =>
- S1NPC.SetPanicked_Server();
+ public void Panic()
+ {
+ if (!SafeIsServer())
+ return;
+
+ S1NPC.SetPanicked_Server();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void Panic() => | |
| S1NPC.SetPanicked(); | |
| S1NPC.SetPanicked_Server(); | |
| public void Panic() | |
| { | |
| if (!SafeIsServer()) | |
| return; | |
| S1NPC.SetPanicked_Server(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@S1API/Entities/NPC.cs` around lines 1722 - 1723, The public Panic() should
check the server context before calling the server-only method: modify
NPC.Panic() to call SafeIsServer() and only invoke S1NPC.SetPanicked_Server()
when SafeIsServer() returns true (otherwise no-op or early return); locate the
Panic() method and replace the direct call to S1NPC.SetPanicked_Server() with
this guard to prevent client-side invocation from reaching server-only code.
| try | ||
| { | ||
| // Local-only revive: set state flags via reflection (read-only properties) | ||
| Utils.ReflectionUtils.TrySetFieldOrProperty(__instance, "IsDead", false); | ||
| Utils.ReflectionUtils.TrySetFieldOrProperty(__instance, "IsKnockedOut", false); | ||
|
|
||
| // Set health backing field directly to bypass SyncVar setter | ||
| Utils.ReflectionUtils.TrySetFieldOrProperty( | ||
| __instance, "<Health>k__BackingField", __instance.MaxHealth); | ||
|
|
||
| // Disable behaviours locally (non-networked equivalent of Disable_Server) | ||
| baseNpc.Behaviour.DeadBehaviour?.Disable(); | ||
| baseNpc.Behaviour.UnconsciousBehaviour?.Disable(); | ||
|
|
||
| // Fire revive event so downstream listeners still react | ||
| __instance.onRevive?.Invoke(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| MelonLogger.Warning($"[S1API] Revive guard failed for custom NPC: {ex.Message}"); | ||
| } | ||
|
|
||
| return false; // skip original to avoid SyncVar/networking calls | ||
| } |
There was a problem hiding this comment.
Don’t skip original revive when the guard path fails.
On Line 1604 the patch always returns false, even if local mutations fail (exception or unsuccessful reflection set). That can leave the NPC unrecovered while still bypassing the original revive path.
💡 Suggested fail-safe patch
- try
+ bool localReviveApplied = false;
+ try
{
// Local-only revive: set state flags via reflection (read-only properties)
- Utils.ReflectionUtils.TrySetFieldOrProperty(__instance, "IsDead", false);
- Utils.ReflectionUtils.TrySetFieldOrProperty(__instance, "IsKnockedOut", false);
+ bool isDeadSet = Utils.ReflectionUtils.TrySetFieldOrProperty(__instance, "IsDead", false);
+ bool isKoSet = Utils.ReflectionUtils.TrySetFieldOrProperty(__instance, "IsKnockedOut", false);
// Set health backing field directly to bypass SyncVar setter
- Utils.ReflectionUtils.TrySetFieldOrProperty(
+ bool healthSet = Utils.ReflectionUtils.TrySetFieldOrProperty(
__instance, "<Health>k__BackingField", __instance.MaxHealth);
+ if (!(isDeadSet && isKoSet && healthSet))
+ {
+ MelonLogger.Warning("[S1API] Revive guard could not apply all local state updates; falling back to original Revive().");
+ return true;
+ }
+
// Disable behaviours locally (non-networked equivalent of Disable_Server)
- baseNpc.Behaviour.DeadBehaviour?.Disable();
- baseNpc.Behaviour.UnconsciousBehaviour?.Disable();
+ baseNpc?.Behaviour?.DeadBehaviour?.Disable();
+ baseNpc?.Behaviour?.UnconsciousBehaviour?.Disable();
// Fire revive event so downstream listeners still react
__instance.onRevive?.Invoke();
+ localReviveApplied = true;
}
catch (Exception ex)
{
MelonLogger.Warning($"[S1API] Revive guard failed for custom NPC: {ex.Message}");
+ return true;
}
- return false; // skip original to avoid SyncVar/networking calls
+ return !localReviveApplied ? true : false; // skip original only on successful local revive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@S1API/Internal/Patches/NPCPatches.cs` around lines 1582 - 1605, The patch
currently always returns false (skipping original revive) even when the local
revive operations fail; change the method so it only returns false when all
local operations succeed. Capture the boolean results from
Utils.ReflectionUtils.TrySetFieldOrProperty calls for "<Health>k__BackingField",
"IsDead", and "IsKnockedOut", and check that
baseNpc.Behaviour.DeadBehaviour?.Disable(),
baseNpc.Behaviour.UnconsciousBehaviour?.Disable(), and
__instance.onRevive?.Invoke() execute without error; if any reflection call
returns false or an exception is thrown, log the failure and return true to
allow the original revive path to run, otherwise return false to skip the
original. Use the existing symbols (__instance,
Utils.ReflectionUtils.TrySetFieldOrProperty, baseNpc.Behaviour,
__instance.onRevive) when implementing the checks.
S1API/S1API.csproj
Outdated
| <Reference Include="$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll" /> | ||
| <Reference Include="$(Il2CppAssembliesPath)\Il2Cppmscorlib.dll" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Core-related references in S1API.csproj =="
rg -n "ScheduleOne\\.Core|Il2CppScheduleOne\\.Core|MonoAssembliesPath|Il2CppAssembliesPath" S1API/S1API.csproj
echo
echo "== Build props that define assembly paths =="
fd -HI '.*build\.props$' -x rg -n "MonoAssembliesPath|Il2CppAssembliesPath|ScheduleOne\\.Core|0\\.4\\.4f6" {}
echo
echo "== Workflow references to assembly bundle/version wiring =="
fd -HI '.*\.(yml|yaml)$' .github -x rg -n "ScheduleOne|assemblies|MonoAssembliesPath|Il2CppAssembliesPath|0\\.4\\.4f6" {}Repository: ifBars/S1API
Length of output: 26202
Add HintPath for IL2CPP Il2CppScheduleOne.Core reference to match Mono reference pattern.
Line 58 uses a simple Include with a property path, which lacks explicit hint path resolution. The Mono configuration (lines 84–85) already demonstrates the correct pattern using HintPath. Update line 58 to follow the same structure for consistency and stable assembly resolution:
Suggested csproj adjustment
- <Reference Include="$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll" />
+ <Reference Include="Il2CppScheduleOne.Core">
+ <HintPath>$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll</HintPath>
+ </Reference>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Reference Include="$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll" /> | |
| <Reference Include="$(Il2CppAssembliesPath)\Il2Cppmscorlib.dll" /> | |
| <Reference Include="Il2CppScheduleOne.Core"> | |
| <HintPath>$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll</HintPath> | |
| </Reference> | |
| <Reference Include="$(Il2CppAssembliesPath)\Il2Cppmscorlib.dll" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@S1API/S1API.csproj` around lines 58 - 59, The Il2Cpp reference for
Il2CppScheduleOne.Core.dll currently uses a bare Reference Include
("Il2CppScheduleOne.Core.dll") and needs an explicit HintPath like the Mono
entries; update the Reference element that uses
$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll (the Reference Include for
Il2CppScheduleOne.Core) to the same pattern used by the Mono references by
adding a nested HintPath that points to
$(Il2CppAssembliesPath)\Il2CppScheduleOne.Core.dll so MSBuild can reliably
resolve the assembly.
ifBars
added
game update
|
I did not mean to close the PR, had to re-make the beta branch and it auto-closed the pr 😆 |
2 participants
Update S1API for Schedule 1 v0.4.4f6
Summary
Schedule 1 v0.4.4f6 introduced a new
ScheduleOne.Coreassembly, splitting base item/entity types out ofAssembly-CSharp. This PR updates S1API to compile against the new game version for both Il2CppMelon and MonoMelon configurations.Changes
Assembly References
ScheduleOne.Core.dll(Mono) andIl2CppScheduleOne.Core.dll(IL2CPP) references to the project file. Base types likeBaseItemDefinition,BaseItemInstance, and core enums now live in this assembly.Namespace Migrations
EItemCategoryandELegalStatusmoved fromScheduleOne.ItemFrameworktoScheduleOne.Core.Items.Framework. AddedS1CoreItemFrameworkusing alias across all 7 affected files.API Renames
MusicPlayer→MusicManagerinLoadingScreenPatches.cs(singleton class renamed in game).NPC.SetPanicked()→NPC.SetPanicked_Server()inNPC.cs(method now uses FishNet[ServerRpc]pattern).Removed Properties
KeywordsandLabelDisplayColorwere removed from item definitions in v0.4.4f6. Stripped all references fromItemDefinition,BuildableItemDefinition, and the Storable/Additive/Buildable/Clothing builder classes.Files Changed (12)
S1API.csprojItems/ItemDefinition.csItems/BuildableItemDefinition.csItems/StorableItemDefinitionBuilder.csItems/AdditiveDefinitionBuilder.csItems/BuildableItemDefinitionBuilder.csItems/ClothingItemDefinitionBuilder.csShops/Shop.csGrowing/SeedCreator.csEntities/NPC.csInternal/Patches/LoadingScreenPatches.csInternal/Patches/NPCPatches.csRelease Notes
ScheduleOne.Core.dllfor Mono andIl2CppScheduleOne.Core.dllfor IL2CPP)EItemCategoryandELegalStatusnow sourced fromScheduleOne.Core.Items.Frameworkinstead ofScheduleOne.ItemFramework, withS1CoreItemFrameworkalias introduced across 7 filesMusicPlayertoMusicManagerin loading screen patch handlingSetPanicked()replaced withSetPanicked_Server()for server-side consistencyKeywordsandLabelDisplayColorfromItemDefinitionand all builder classes (StorableItemDefinitionBuilder,AdditiveDefinitionBuilder,BuildableItemDefinitionBuilder,ClothingItemDefinitionBuilder)Changes by Author