Implement RFC #1: Standard RPC to replace Convenience and Performance RPCs #408
Conversation
- Fixed an issue where on latest 21.1 you could not build ILPP without some PrivateCorelib issues - Removed UTP package as it causes issues with burst and ILPP that needs to be resolved at some future point - Updated Burst to latest 1.43 and Mono to 1.10.1-preview.1 (this is the legit package not the shim package) All this is compat back to 2018.4
…d moved some core around to be more clean and consistent.
| /// <summary> | ||
| /// XXHash implementation. | ||
| /// </summary> | ||
| internal static class XXHash |
There was a problem hiding this comment.
Do we really need another hash method?
The MLAPI already has Fowler–Noll–Vo which it uses for everything internally.
There was a problem hiding this comment.
Maybe we don't need another hash method but we probably need xxHash instead of FNV.
xxHash is very efficient, well-known, highly-adopted non-cryptographic hashing algorithm.
You could see that over RPC RFC in the "Cross Compatibility" section but let me link those here too for you:
Section in the RFC: https://github.com/Unity-Technologies/com.unity.multiplayer.rfcs/blob/rfc/std-rpc-api/text/0000-std-rpc-api.md#cross-compatibility
xxHash website: https://cyan4973.github.io/xxHash/
Also we use xxHash in other places around Unity (e.g. DOTS Entities uses xxHash). I think xxHash excels in all aspects compared to our MLAPI implementation of FNV.
I'm happy to have another PR that replaces FNV with xxHash around the whole MLAPI codebase but for now I think I won't be doing that in this PR since xxHash will be used in the Editor only for now (other parts of hashing would touch runtime as well, so let's have a separate PR and discussion just for that).
There was a problem hiding this comment.
Ok, if nothing else, let's put in some verbiage on how we made this choice as well as something like "xxHash is also used by DOTs, chore task to remove this when we can import from standard Unity, etc."
…other related types and implementations
| - 2020.1 | ||
| - 2020.2 |
There was a problem hiding this comment.
yeah, this needs a little bit more of a discussion :)
| @@ -64,8 +60,8 @@ internal static class MLAPIConstants | |||
| "", | |||
| "", | |||
| "", | |||
| "", | |||
| "", | |||
| "MLAPI_STD_SERVER_RPC", | |||
There was a problem hiding this comment.
I guess we wanted for sure these to be 30 & 31
| ulong hash = 0; | ||
| switch (NetworkingManager.Singleton.NetworkConfig.RpcHashSize) | ||
| { | ||
| case HashSize.VarIntTwoBytes: |
There was a problem hiding this comment.
If you look at ILSpy, does this case statement get optimized out?
There was a problem hiding this comment.
no, NetworkingManager.Singleton.NetworkConfig.RpcHashSize is not a compile-time constant so the check happens at runtime still.
|
|
This implements RFC#1: Standard RPC to replace Convenience and Performance RPCs (tracking issue MLAPI#406).