Skip to content

feat: per axis networktransform state sync (+bitwise state comp) #1042

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 3 commits into from
Aug 11, 2021

Conversation

0xFA11
Copy link
Contributor

@0xFA11 0xFA11 commented Aug 11, 2021

this PR introduces:

  • SyncPositionX, SyncPositionY, SyncPositionZ for position
  • SyncRotAngleX, SyncRotAngleY, SyncRotAngleZ for rotation/eulerAngles
  • SyncScaleX, SyncScaleY, SyncScaleZ for scale
  • (note: Inspector UX is still not finalized yet)

also, we no longer send all properties in one go whenever any of them or axes of any prop changes, we look at position/rotation/scale/inlocalspace props granularly depending on sync flags — which reduces overall bandwidth caused by networktransform state synchronization by a lot.


public bool InLocalSpace
{
get => (Bitset & (1 << InLocalSpaceBit)) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha, we could, only if we were to have BitField16 :P

/// <summary>
/// The base amount of sends per seconds to use when range is disabled
/// </summary>
[SerializeField, Range(0, 120), Tooltip("The base amount of sends per seconds to use when range is disabled")]
public float FixedSendsPerSecond = 30f;

private Transform m_Transform; // cache the transform component to reduce unnecessary bounce between managed and native
private readonly NetworkVariable<NetworkState> m_NetworkState = new NetworkVariable<NetworkState>(new NetworkState());
private NetworkState m_PrevNetworkState;
internal readonly NetworkVariable<NetworkState> ReplNetworkState = new NetworkVariable<NetworkState>(new NetworkState());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Replicated instead of Repl

@0xFA11 0xFA11 changed the title feat: per axis networktransform state synchronization (+bitwise state compression) feat: per axis networktransform state sync (+bitwise state comp) Aug 11, 2021
ReplNetworkState.SetDirty(true);
}

// TODO: temporary! the function body below probably needs to be rewritten later with interpolation in mind
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd put this onto your plate @SamuelBellomo when you start working on interpolator integration & polish.

Copy link
Contributor

@LukeStampfli LukeStampfli left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'd refer to @SamuelBellomo here to ensure that this is compatible with his work.

serializer.Serialize(ref Position);
serializer.Serialize(ref Rotation);
serializer.Serialize(ref Scale);
// InLocalSpace + HasXXX Bits
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we already had stateful serialization so that we could just send "same bools as previously" to save on bandwidth. But I guess this will come with snapshot and delta encoding.

public class NetworkTransformStateTests
{
[Test]
public void TestSyncAxes(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice


namespace Unity.Netcode.RuntimeTests
{
public class NetworkTransformStateTests
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be interested to see a multiinstance test where we check that values that aren't supposed to change actually didn't change client side. ex: server side shouldn't pos.x, updates pos.x and pos.y, make sure only receive pos.y and pos.x stays the same (could have initial value != 0 to make sure it doesn't get reset to 0). I'm sure it works with your current implementation, this test would be for regression testing, making sure that future work doesn't break this.

@SamuelBellomo
Copy link
Contributor

Looks good to me but I'd refer to @SamuelBellomo here to ensure that this is compatible with his work.

My work already breaks some of this, but nothing too major, that's expected. I'll deal with the merge conflict.

I added a few nit comments. It'd be great if we had a bit more tests, but beside that I'm good.
I'm guessing the custom editor work we discussed will come in a followup PR?

@0xFA11
Copy link
Contributor Author

0xFA11 commented Aug 11, 2021

Looks good to me but I'd refer to @SamuelBellomo here to ensure that this is compatible with his work.

My work already breaks some of this, but nothing too major, that's expected. I'll deal with the merge conflict.

I added a few nit comments. It'd be great if we had a bit more tests, but beside that I'm good.
I'm guessing the custom editor work we discussed will come in a followup PR?

UX will come later yes :)

@0xFA11 0xFA11 merged commit 87f9ec9 into develop Aug 11, 2021
@0xFA11 0xFA11 deleted the feat/mtt833 branch August 11, 2021 18:10
SamuelBellomo added a commit that referenced this pull request Aug 23, 2021
…nsform

* develop: (21 commits)
  test: adding more details to multiprocess readme (#1050)
  refactor!: convert NetworkTransform.NetworkState to `struct` (#1061)
  fix: networkmanager destroy on app quit (#1011)
  feat: snapshot, using unreliable packets, now that the underlying foundation supports it. Only merge after #1062 (#1064)
  feat: snapshot. Fully integrated despawn, mtt-1092, mtt-1056 (#1062)
  fix: eliminate bad use-after-free(destroy) pattern (#1068)
  chore: cleanup meta files for empty dirs (#1067)
  chore: minor MLAPI to Netcode rename (#1065)
  feat: report network behaviour name to the profiler (#1058)
  fix: player movement (#1063)
  test: Add unit tests for NetworkTime properties (#1053)
  chore: remove authority & netvar perms from NetworkTransform (#1059)
  feat: networktransform pos/rot/sca thresholds on state sync (#1055)
  feat: expose network behaviour type name internally (#1057)
  chore: remove all the old profiling code (#1048)
  fix: if-guard `NetworkManager.__rpc_name_table` access (#1056)
  fix: Disabling fixedupdate portion of SpawnRpcDespawn test because it's failing for known reasons that will be fixed in the IMessage refactor. (#1049)
  feat: Implement metrics for the new network profiler (#960)
  chore!: change package name & asmdefs (#1026)
  feat: per axis networktransform state sync (+bitwise state comp) (#1042)
  ...

# Conflicts:
#	com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs
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.

5 participants