Skip to content

fix: Fixed NetworkVariables being truncated to short.MaxValue when NetworkConfig.EnsureNetworkVariableLengthSafety is enabled. #1811

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
- Fixed issue where NetworkManager would continue starting even if the NetworkTransport selected failed. (#1780)
- Fixed issue when spawning new player if an already existing player exists it does not remove IsPlayer from the previous player (#1779)
- Fixed lack of notification that NetworkManager and NetworkObject cannot be added to the same GameObject with in-editor notifications (#1777)
- Network variable updates are no longer limited to 32,768 bytes when NetworkConfig.EnsureNetworkVariableLengthSafety is enabled. The limits are now determined by what the transport can send in a message. (#1811)
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit-pick)
Could you update your entry to this: "- Fixed network..." and move this entry to the top of the list so we keep a descending order on the PR numbers?


## [1.0.0-pre.6] - 2022-03-02

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public void Serialize(FastBufferWriter writer)
{
if (!shouldWrite)
{
writer.WriteValueSafe((ushort)0);
BytePacker.WriteValueBitPacked(writer, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😻

}
}
else
Expand All @@ -69,15 +69,15 @@ public void Serialize(FastBufferWriter writer)
{
if (NetworkBehaviour.NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
var tempWriter = new FastBufferWriter(MessagingSystem.NON_FRAGMENTED_MESSAGE_MAX_SIZE, Allocator.Temp, short.MaxValue);
networkVariable.WriteDelta(tempWriter);
var tempWriter = new FastBufferWriter(MessagingSystem.NON_FRAGMENTED_MESSAGE_MAX_SIZE, Allocator.Temp, MessagingSystem.FRAGMENTED_MESSAGE_MAX_SIZE);
NetworkBehaviour.NetworkVariableFields[i].WriteDelta(tempWriter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, can still use the networkVariable temporary here. More consistent if we create the temporary to use it throughout

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was actually a merge thing. That local variable was introduced in another change that was made to develop while I was working on this and the merge naturally didn't update this line. I'll update it.

BytePacker.WriteValueBitPacked(writer, tempWriter.Length);

if (!writer.TryBeginWrite(FastBufferWriter.GetWriteSize<ushort>() + tempWriter.Length))
if (!writer.TryBeginWrite(tempWriter.Length))
{
throw new OverflowException($"Not enough space in the buffer to write {nameof(NetworkVariableDeltaMessage)}");
}

writer.WriteValue((ushort)tempWriter.Length);
tempWriter.CopyTo(writer);
}
else
Expand Down Expand Up @@ -135,10 +135,10 @@ public void Handle(ref NetworkContext context)
{
for (int i = 0; i < networkBehaviour.NetworkVariableFields.Count; i++)
{
ushort varSize = 0;
int varSize = 0;
if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
m_ReceivedNetworkVariableData.ReadValueSafe(out varSize);
ByteUnpacker.ReadValueBitPacked(m_ReceivedNetworkVariableData, out varSize);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😻


if (varSize == 0)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ internal uint GetMessageType(Type t)
}

public const int NON_FRAGMENTED_MESSAGE_MAX_SIZE = 1300;
public const int FRAGMENTED_MESSAGE_MAX_SIZE = int.MaxValue;
public const int FRAGMENTED_MESSAGE_MAX_SIZE = BytePacker.BitPackedIntMax;

internal struct MessageWithHandler
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Unity.Netcode
public static class BytePacker
{
#if UNITY_NETCODE_DEBUG_NO_PACKING

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void WriteValuePacked<T>(FastBufferWriter writer, T value) where T: unmanaged => writer.WriteValueSafe(value);
#else
Expand Down Expand Up @@ -277,10 +277,21 @@ public static void WriteValuePacked(FastBufferWriter writer, string s)


#if UNITY_NETCODE_DEBUG_NO_PACKING

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void WriteValueBitPacked<T>(FastBufferWriter writer, T value) where T: unmanaged => writer.WriteValueSafe(value);
#else

public const ushort BitPackedUshortMax = (1 << 15) - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😻

public const short BitPackedShortMax = (1 << 14) - 1;
public const short BitPackedShortMin = -(1 << 14);
public const uint BitPackedUintMax = (1 << 30) - 1;
public const int BitPackedIntMax = (1 << 29) - 1;
public const int BitPackedIntMin = -(1 << 29);
public const ulong BitPackedULongMax = (1L << 61) - 1;
public const long BitPackedLongMax = (1L << 60) - 1;
public const long BitPackedLongMin = -(1L << 60);

/// <summary>
/// Writes a 14-bit signed short to the buffer in a bit-encoded packed format.
/// The first bit indicates whether the value is 1 byte or 2.
Expand All @@ -307,7 +318,7 @@ public static void WriteValuePacked(FastBufferWriter writer, string s)
public static void WriteValueBitPacked(FastBufferWriter writer, ushort value)
{
#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (value >= 0b1000_0000_0000_0000)
if (value >= BitPackedUshortMax)
{
throw new ArgumentException("BitPacked ushorts must be <= 15 bits");
}
Expand Down Expand Up @@ -356,7 +367,7 @@ public static void WriteValueBitPacked(FastBufferWriter writer, ushort value)
public static void WriteValueBitPacked(FastBufferWriter writer, uint value)
{
#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (value >= 0b0100_0000_0000_0000_0000_0000_0000_0000)
if (value > BitPackedUintMax)
{
throw new ArgumentException("BitPacked uints must be <= 30 bits");
}
Expand Down Expand Up @@ -396,7 +407,7 @@ public static void WriteValueBitPacked(FastBufferWriter writer, uint value)
public static void WriteValueBitPacked(FastBufferWriter writer, ulong value)
{
#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (value >= 0b0010_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000)
if (value > BitPackedULongMax)
{
throw new ArgumentException("BitPacked ulongs must be <= 61 bits");
}
Expand Down