Add product effect override callbacks for players/NPCs#56
Merged
ifBars merged 1 commit intoifBars:stablefrom Mar 15, 2026
Merged
Add product effect override callbacks for players/NPCs#56ifBars merged 1 commit intoifBars:stablefrom
ifBars merged 1 commit intoifBars:stablefrom
Conversation
Implement a callback system in ProductManager to allow registration, removal, and clearing of custom effect handlers for both player and NPC product effects. Add Harmony patches to intercept effect application and invoke registered callbacks, with optional fallback to base behavior. Update documentation with usage examples for the new override system, enabling modders to customize or extend product effect logic.
📝 WalkthroughWalkthroughThe pull request introduces a centralized product effect callback interception system. A new Harmony patch intercepts effect applications to players and NPCs, resolving effects and invoking registered callbacks before conditionally applying default behavior. ProductManager adds public callback registration APIs with control over default effect execution, and documentation describes the callback patterns. Changes
Sequence DiagramsequenceDiagram
participant Player/NPC as Player or NPC
participant Game as Game Code
participant Patch as ProductEffectPatches
participant Manager as ProductManager
participant Effect as Effect Application
Player/NPC->>Game: ApplyEffectsToPlayer/NPC
Game->>Patch: ApplyEffects_Prefix (Harmony intercept)
Patch->>Patch: ResolveEffects from ProductInstance
Patch->>Manager: TryInvokeEffectCallback/NpcCallback
Manager->>Manager: Lookup registered callback
alt Callback Registered
Manager->>Manager: Invoke callback (catch errors)
Manager-->>Patch: Return (callback_found, allowDefaultEffect)
else No Callback
Manager-->>Patch: Return (not_found, n/a)
end
alt allowDefaultEffect = true
Patch->>Effect: Apply default effect behavior
end
Patch-->>Game: Return false (suppress original)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/docs/products-api.md`:
- Around line 84-110: Update the docs to explicitly state that
ProductManager.SetEffectCallback (and removals via
ProductManager.RemoveEffectCallback / ClearEffectCallbacks) apply only to the
local player: note that the runtime routing in ProductEffectPatches only invokes
callbacks when the effect target equals the local player, so callbacks will not
run for other players or NPCs; clarify this behavior next to the Player
callbacks section and when describing allowDefaultEffect so modders do not
expect non-local targets to be intercepted.
In `@S1API/Internal/Patches/ProductEffectPatches.cs`:
- Around line 79-130: The patch currently always returns false after
ResolveEffects succeeds, altering base behavior even when no callbacks are
registered; modify the logic in the prefix (around ResolveEffects, effects, and
invokedEffectIds) to pre-scan effects and detect if any effect.ID has a
registered override by calling ProductManager.TryInvokeEffectCallback (for
players) and ProductManager.TryInvokeNpcEffectCallback (for NPCs) before
applying changes—if no effect IDs have any registered player/NPC callback for
this target, return true to leave the original method untouched; otherwise
proceed with the existing loop and return false only when at least one effect ID
is handled by a registered callback.
- Around line 42-61: TargetMethods() is returning duplicate MethodBase entries
because it scans all ProductItemInstance subtypes including abstract types and
uses GetMethod without DeclaredOnly so inherited base methods appear for every
child; update the scan to skip abstract types (check type.IsAbstract), call
GetMethod with BindingFlags.DeclaredOnly added to the existing flags for both
"ApplyEffectsToPlayer" and "ApplyEffectsToNPC", and deduplicate the resulting
sequence (e.g., call .Distinct()) before casting to MethodBase so each actual
declared implementation is patched only once.
In `@S1API/Products/ProductManager.cs`:
- Around line 264-298: The lookup methods TryInvokeEffectCallback and
TryInvokeNpcEffectCallback currently execute registration.Callback and thus can
let exceptions suppress the base effect; change them to only perform lookup and
expose the callback without invoking it: keep the early null/whitespace checks
and dictionary lookup against EffectCallbacks / NpcEffectCallbacks, set
allowDefaultEffect = registration.AllowDefaultEffect, and return the delegate
via a new out parameter (e.g., out Action<Player> playerCallback and out
Action<NPC> npcCallback) instead of calling registration.Callback inside those
methods; then update the caller ProductEffectPatches.ApplyEffects_Prefix to
invoke the returned delegate inside a try/catch so exceptions don’t affect
allowDefaultEffect or the decision to apply the default effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e72d58f-44a8-4f38-9043-08bba874cff2
📒 Files selected for processing (3)
S1API/Internal/Patches/ProductEffectPatches.csS1API/Products/ProductManager.csS1API/docs/products-api.md
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
2 participants
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.
Implement a callback system in ProductManager to allow registration, removal, and clearing of custom effect handlers for both player and NPC product effects. Add Harmony patches to intercept effect application and invoke registered callbacks, with optional fallback to base behavior. Update documentation with usage examples for the new override system, enabling modders to customize or extend product effect logic.
Release Notes
Product Effect Override Callbacks
ProductManagerto enable custom effect handlers for both player and NPC product effectsContributor Statistics