Skip to content

fix wrongfully kicking players when releasing a critter#3290

Open
sors89 wants to merge 1 commit into
Pryaxis:general-develfrom
sors89:general-devel
Open

fix wrongfully kicking players when releasing a critter#3290
sors89 wants to merge 1 commit into
Pryaxis:general-develfrom
sors89:general-devel

Conversation

@sors89
Copy link
Copy Markdown

@sors89 sors89 commented May 16, 2026

Fix critter release kicking issue

since lastVisualizedSelectedItem is not synced between server and client, it stays as default Item value. this causes makeNPC to always be 0, failing the check and making Bouncer interpret every critter release as an invalid action, resulting in a player kick

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 16, 2026

Greptile Summary

This PR fixes a regression in OnReleaseNPC where every legitimate critter release was incorrectly kicking players. The root cause was that lastVisualizedSelectedItem is a client-side field that is never synced to the server, so it always held a default Item value (with makeNPC == 0), causing the mismatch check to always fire.

  • Replaces args.Player.TPlayer.lastVisualizedSelectedItem with args.Player.SelectedItem (which reads TPlayer.inventory[TPlayer.selectedItem], the server-tracked held item), consistent with how every other OnXxx handler in Bouncer.cs reads the current item.
  • Removes the inline comment that explained the Explosive Bunny special-case logic inside the now-corrected branch.

Confidence Score: 4/5

The change is a focused one-line fix that aligns OnReleaseNPC with every other handler in Bouncer.cs, and the new code path has no side-effects beyond the existing kick/allow logic.

The core fix is correct and the SelectedItem pattern is already used consistently throughout the file. The only change worth a second look is the incidental removal of the Explosive Bunny comment, which reduces clarity without affecting behaviour.

No files require special attention beyond the removed comment inside OnReleaseNPC.

Important Files Changed

Filename Overview
TShockAPI/Bouncer.cs Fixes critter-release false kicks by switching from the unsynced lastVisualizedSelectedItem to SelectedItem (which reads from the server-tracked inventory slot); also removes the explanatory comment for the Explosive Bunny special-case branch.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Server
    participant Bouncer

    Client->>Server: PlayerUpdate (selectedItem index synced)
    Client->>Server: ReleaseNPC packet (type, style)
    Server->>Bouncer: OnReleaseNPC(args)
    Bouncer->>Bouncer: Read args.Player.SelectedItem
    alt "SelectedItem.makeNPC == type AND placeStyle == style"
        Bouncer->>Server: Allow critter release
    else "type == ExplosiveBunny AND ownedProjectileCounts > 0"
        Bouncer->>Server: Allow (delayed projectile release)
    else Mismatch
        Bouncer->>Client: Kick released critter was not from its item
    end
Loading

Comments Outside Diff (1)

  1. TShockAPI/Bouncer.cs, line 2366-2373 (link)

    P2 Explanatory comment removed alongside the bug fix

    The comment that explained the Explosive Bunny special case (why ownedProjectileCounts is checked instead of simply rejecting) was deleted as part of this change. Without it, the branch reads as an unexplained exception to the mismatch guard. Restoring the comment — "a delayed critter release from an Explosive Bunny projectile is a valid non-crafted packet" — would preserve the original rationale for future maintainers.

Reviews (1): Last reviewed commit: "use `SelectedItem` instead of `lastVisua..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant