Skip to content

Update NetworkCollider/2D entered state once per tick, not once per c… - #1085

Open
jimdroberts wants to merge 1 commit into
FirstGearGames:mainfrom
jimdroberts:fix/networkcollider-per-object-bookkeeping
Open

jimdroberts wants to merge 1 commit into
FirstGearGames:mainfrom
jimdroberts:fix/networkcollider-per-object-bookkeeping

Conversation

@jimdroberts

Copy link
Copy Markdown
Contributor

Problem

When a NetworkTrigger/NetworkCollision (or the 2D versions) is on an object with more than one enabled collider of the matching kind, for example a trigger made of two box colliders, callbacks go wrong:

  • Anything touched only by the second collider never gets OnEnter, OnStay or OnExit.
  • Anything touched by the first collider gets OnEnter every tick. It never gets OnStay, and it never gets OnExit when it leaves.
  • The per-tick HashSet is pushed into CollectionCaches<Collider> twice per tick while still in use. Two later RetrieveHashSet() callers anywhere in the process can then receive the same instance.

Objects with a single matching collider are not affected. We found this in a production MMO project.

Cause

Runtime/Generated/Component/Prediction/NetworkCollider.cs on main, CheckColliders L143-250 (NetworkCollider2D.cs L141-248 is identical). The enter/stay/exit bookkeeping sits inside foreach (Collider col in _colliders), but current is a single set shared by the whole loop. On the second matching collider:

  1. B's hits are added to current.
  2. _enteredColliders.TryGetValue(localTick) now finds current itself, which the first iteration stored. It is passed to CollectionCaches.Store, which clears it and pushes it onto the cache, and then removed.
  3. current is now empty, so the else branch stores it into the cache a second time.
  4. No entry for the tick survives. On the server, the first iteration also discarded the lastTick entry, so the next tick always sees "no colliders last tick" and invokes OnEnter.

Fix

Close the foreach after the overlap queries, so that current holds the union of all colliders' hits. Then run the existing bookkeeping block once per tick. No logic inside the block changed. The diff is mostly re-indentation (view with whitespace ignored: +1/-1 lines per file).

This does not touch NetworkCollision2D.cs, so it does not conflict with #1074. It builds on the lastTick key fix from #1041.

Evidence

repro.py is a line-by-line model of CheckColliders and of CollectionCaches.Store, which clears the set and pushes it onto a Stack. It runs an object with colliders A and B for five ticks. Y overlaps both colliders and X overlaps only B, for ticks 1-3; both leave at tick 4:

--- main: bookkeeping inside foreach (server)
tick 1: A=['Y'] B=['X', 'Y'] -> Enter(Y)   [double-stored: 1]
tick 2: A=['Y'] B=['X', 'Y'] -> Enter(Y)   [double-stored: 3]
tick 3: A=['Y'] B=['X', 'Y'] -> Enter(Y)   [double-stored: 5]
tick 4: A=[] B=[] -> -                     [double-stored: 7]
tick 5: A=[] B=[] -> -                     [double-stored: 9]
--- fix: bookkeeping once per tick (server)
tick 1: A=['Y'] B=['X', 'Y'] -> Enter(X) Enter(Y)   [double-stored: 0]
tick 2: A=['Y'] B=['X', 'Y'] -> Stay(X) Stay(Y)     [double-stored: 0]
tick 3: A=['Y'] B=['X', 'Y'] -> Stay(X) Stay(Y)     [double-stored: 0]
tick 4: A=[] B=[] -> Exit(X) Exit(Y)                [double-stored: 0]
tick 5: A=[] B=[] -> -                              [double-stored: 0]

The client-side (non-server) run produces the same sequences. The model is not Unity; that the Unity code behaves the same way is verified by reading. Compiled against Unity 6000.4 with no new errors or warnings.

Behavior change

  • Objects with several matching colliders get correct Enter/Stay/Exit callbacks.
  • The bookkeeping now also runs on a tick where none of the object's colliders was checked, because all were disabled or had a mismatched isTrigger. OnExit fires for whatever was entered on the previous tick. Previously the tick was skipped and those colliders never received OnExit.
  • Single-collider objects are otherwise unchanged.

…ollider

The enter/stay/exit bookkeeping ran inside foreach (_colliders) while every
iteration shared one per-tick HashSet. With more than one matching collider the
set was returned to CollectionCaches while still in use, and twice; hits on
later colliders got no callbacks, and hits on the first got OnEnter every tick
and never OnExit. Close the loop after the overlap queries and run the
bookkeeping once per tick on the union of hits. Applies to NetworkCollider and
NetworkCollider2D.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant