Skip to content

feat(NetworkIdentity): Reuse Network IDs #3734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

MrGadget1024
Copy link
Collaborator

@MrGadget1024 MrGadget1024 commented Jan 12, 2024

Keep netId's small for CompressVarUInt.

  • Uses a Queue (FIFO) with delay time in seconds
  • Configurable in Network Manager (Enabled & Delay)
  • Queue shown in NI Info Panel

Justification

Imagine the server changes scenes from one with many networked objects to another with many networked objects, or loads and unloads additives somewhat frequently that have a lot of net objects...you want that delay low enough that the id's get reused promptly without risk of reference collision race condition, and we can't just reset to zero on server scene change because DDOL is a thing.

Second example is swarms of mobs that get spawned and destroyed in large-ish numbers, over and over. Their netId's would get reused in every wave.

The other obvious example is networked projectiles, which we all know isn't a good idea, but if a user does that, they're short lived and their id's should recycle quickly.

Pooling doesn't effect any of the above, because unspawn / spawn still gets a new netId assigned.

Ideally most of the time there will be less than 241 networked objects alive at a time, reducing the uint netId to 1 byte on the wire instead of 4. Even if it's over 240 and less than 67824, which really would be the 99.99% case in the real world, that still gets it down to 2 bytes instead of 4 and the more network updates they have the more benefit you get from reusing ids.

image

image

image

- Uses a Queue (FIFO) with delay time in seconds
- Configurable in Network Manager (Enabled & Delay)
- Pool shown in NI Info Panel
@MrGadget1024 MrGadget1024 added enhancement New feature or request Awaiting Review labels Jan 12, 2024
@MrGadget1024 MrGadget1024 requested a review from miwarnec January 12, 2024 16:40
@Ikalou
Copy link

Ikalou commented Feb 23, 2024

I realized talking to someone on Discord this PR may have an effect on the guarantee that OnStartClient be called on the client during initial spawn in the order in which objects where spawned on the server:

foreach (NetworkIdentity identity in spawned.Values.OrderBy(uv => uv.netId))
. I don't know how important that is I just thought I'd mention it.

@MrGadget1024
Copy link
Collaborator Author

on the guarantee that OnStartClient be called on the client during initial spawn in the order in which objects where spawned on the server

Wouldn't make any difference than current state of affairs today...scene objects would still be the lowest ID's because they get theirs first before spawned objects and players. Obviously if a scene object got unspawned or destroyed, the ID would be recycled with this PR, and given to the next spawned thing, and if the scene object were respawned it would get a much higher ID in either case, separating it from the initial block of ID's of the remaining scene objects.

Also, if player objects are carried through a scene change in DDOL, the scene objects in the new scene would all have ID's higher than that of the player object in current Mirror and would be more likely to get lower ID's if recycled.

Bottom line is that while that OrderBy attempts to spawn players after scene objects so attempts to get references to them from the player object succeed, that is breakable by circumstances described above, with or without this PR.

@miwarnec
Copy link
Collaborator

miwarnec commented Apr 9, 2025

very smart solution.
I think it's too smart though - if we ever hit the 2 billion uint limit, then let's just switch to ulong.
keep it simple & stupid, as usual

@miwarnec miwarnec closed this Apr 9, 2025
@MrGadget1024
Copy link
Collaborator Author

if we ever hit the 2 billion uint limit, then let's just switch to ulong.

That wasn't the point...the point was to keep lower id's for best results from VarInt compression.

@MrGadget1024 MrGadget1024 reopened this Apr 17, 2025
@JesusLuvsYooh
Copy link
Contributor

JesusLuvsYooh commented Apr 17, 2025

if we ever hit the 2 billion uint limit, then let's just switch to ulong.

That wasn't the point...the point was to keep lower id's for best results from VarInt compression.

I think the tooltip needs to add the upsides and downsides, such as this ^
One second sounds very frequent for the check, surely people are not going through netid's that fast.
Every 60s? Slider feels overkill (just int value, allows them to set higher then than slider allows).

@MrGadget1024
Copy link
Collaborator Author

One second sounds very frequent for the check, surely people are not going through netid's that fast.
Every 60s? Slider feels overkill (just int value, allows them to set higher then than slider allows).

You're misinterpreting that...it's not any sort of interval. It's how long to lock an id after destroy before it's eligible for reassignment. Basically long enough that anything else that might've been referencing the object by its id has cleaned up and sufficient frames have advanced, coroutines have run their due course, etc.

@JesusLuvsYooh
Copy link
Contributor

One second sounds very frequent for the check, surely people are not going through netid's that fast.
Every 60s? Slider feels overkill (just int value, allows them to set higher then than slider allows).

You're misinterpreting that...it's not any sort of interval. It's how long to lock an id after destroy before it's eligible for reassignment. Basically long enough that anything else that might've been referencing the object by its id has cleaned up and sufficient frames have advanced, coroutines have run their due course, etc.

ah okay! add an explanation or something along those lines to the tooltip, or comments in the code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants