Skip to content

feat!: GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator #698

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 17 commits into from
Apr 8, 2021
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
10 changes: 5 additions & 5 deletions com.unity.multiplayer.mlapi/Editor/NetworkManagerEditor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -169,14 +169,14 @@ private void OnEnable()
for (int i = 0; i < m_NetworkManager.NetworkConfig.NetworkPrefabs.Count; i++)
{
// Find the first playerPrefab
if (m_NetworkManager.NetworkConfig.NetworkPrefabs[i].PlayerPrefab)
if (m_NetworkManager.NetworkConfig.NetworkPrefabs[i].IsPlayer)
{
// Iterate over all other and set player prefab to false
for (int j = 0; j < m_NetworkManager.NetworkConfig.NetworkPrefabs.Count; j++)
{
if (j != i && m_NetworkManager.NetworkConfig.NetworkPrefabs[j].PlayerPrefab)
if (j != i && m_NetworkManager.NetworkConfig.NetworkPrefabs[j].IsPlayer)
{
m_NetworkManager.NetworkConfig.NetworkPrefabs[j].PlayerPrefab = false;
m_NetworkManager.NetworkConfig.NetworkPrefabs[j].IsPlayer = false;
}
}

Expand All @@ -200,7 +200,7 @@ private void OnEnable()

for (int i = 0; i < m_NetworkManager.NetworkConfig.NetworkPrefabs.Count; i++)
{
if (m_NetworkManager.NetworkConfig.NetworkPrefabs[i].PlayerPrefab)
if (m_NetworkManager.NetworkConfig.NetworkPrefabs[i].IsPlayer)
{
playerPrefabIndex = i;
break;
Expand All @@ -210,7 +210,7 @@ private void OnEnable()
using (new EditorGUI.DisabledScope(playerPrefabIndex != -1 && playerPrefabIndex != index))
{
EditorGUI.PropertyField(new Rect(rect.width - secondFieldWidth, rect.y, secondFieldWidth,
EditorGUIUtility.singleLineHeight), element.FindPropertyRelative(nameof(NetworkPrefab.PlayerPrefab)), GUIContent.none);
EditorGUIUtility.singleLineHeight), element.FindPropertyRelative(nameof(NetworkPrefab.IsPlayer)), GUIContent.none);
}
};

Expand Down
52 changes: 32 additions & 20 deletions com.unity.multiplayer.mlapi/Editor/NetworkObjectEditor.cs
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
using System.Collections.Generic;
using MLAPI;
using UnityEditor;
using UnityEngine;

namespace UnityEditor
namespace MLAPI.Editor
{
[CustomEditor(typeof(NetworkObject), true)]
[CanEditMultipleObjects]
public class NetworkObjectEditor : Editor
public class NetworkObjectEditor : UnityEditor.Editor
{
private bool m_Initialized;
private NetworkObject m_NetworkObject;
private bool m_ShowObservers;

private void Init()
private void Initialize()
{
if (m_Initialized)
{
Expand All @@ -25,7 +25,7 @@ private void Init()

public override void OnInspectorGUI()
{
Init();
Initialize();

if (!m_NetworkObject.IsSpawned && NetworkManager.Singleton != null && NetworkManager.Singleton.IsServer)
{
Expand All @@ -41,17 +41,26 @@ public override void OnInspectorGUI()
}
else if (m_NetworkObject.IsSpawned)
{
EditorGUILayout.LabelField("PrefabHashGenerator: ", m_NetworkObject.PrefabHashGenerator, EditorStyles.label);
EditorGUILayout.LabelField("PrefabHash: ", m_NetworkObject.PrefabHash.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("GlobalObjectIdHash64: ", m_NetworkObject.GlobalObjectIdHash64.ToString("X"), EditorStyles.label);
EditorGUILayout.LabelField("NetworkId: ", m_NetworkObject.NetworkObjectId.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("OwnerId: ", m_NetworkObject.OwnerClientId.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("IsSpawned: ", m_NetworkObject.IsSpawned.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("IsLocalPlayer: ", m_NetworkObject.IsLocalPlayer.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("IsOwner: ", m_NetworkObject.IsOwner.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("IsOwnedByServer: ", m_NetworkObject.IsOwnedByServer.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("IsPlayerObject: ", m_NetworkObject.IsPlayerObject.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("IsSceneObject: ", (m_NetworkObject.IsSceneObject == null ? "Null" : m_NetworkObject.IsSceneObject.Value.ToString()), EditorStyles.label);
var guiEnabled = GUI.enabled;
GUI.enabled = false;
EditorGUILayout.TextField(nameof(NetworkObject.GlobalObjectIdHash), m_NetworkObject.GlobalObjectIdHash.ToString());
EditorGUILayout.TextField(nameof(NetworkObject.NetworkObjectId), m_NetworkObject.NetworkObjectId.ToString());
EditorGUILayout.TextField(nameof(NetworkObject.OwnerClientId), m_NetworkObject.OwnerClientId.ToString());
EditorGUILayout.Toggle(nameof(NetworkObject.IsSpawned), m_NetworkObject.IsSpawned);
EditorGUILayout.Toggle(nameof(NetworkObject.IsLocalPlayer), m_NetworkObject.IsLocalPlayer);
EditorGUILayout.Toggle(nameof(NetworkObject.IsOwner), m_NetworkObject.IsOwner);
EditorGUILayout.Toggle(nameof(NetworkObject.IsOwnedByServer), m_NetworkObject.IsOwnedByServer);
EditorGUILayout.Toggle(nameof(NetworkObject.IsPlayerObject), m_NetworkObject.IsPlayerObject);
if (m_NetworkObject.IsSceneObject.HasValue)
{
EditorGUILayout.Toggle(nameof(NetworkObject.IsSceneObject), m_NetworkObject.IsSceneObject.Value);
}
else
{
EditorGUILayout.TextField(nameof(NetworkObject.IsSceneObject), "null");
}
EditorGUILayout.Toggle(nameof(NetworkObject.DestroyWithScene), m_NetworkObject.DestroyWithScene);
GUI.enabled = guiEnabled;

if (NetworkManager.Singleton != null && NetworkManager.Singleton.IsServer)
{
Expand All @@ -67,11 +76,11 @@ public override void OnInspectorGUI()
{
if (NetworkManager.Singleton.ConnectedClients[observerClientIds.Current].PlayerObject != null)
{
EditorGUILayout.ObjectField("ClientId: " + observerClientIds.Current, NetworkManager.Singleton.ConnectedClients[observerClientIds.Current].PlayerObject, typeof(GameObject), false);
EditorGUILayout.ObjectField($"ClientId: {observerClientIds.Current}", NetworkManager.Singleton.ConnectedClients[observerClientIds.Current].PlayerObject, typeof(GameObject), false);
}
else
{
EditorGUILayout.TextField("ClientId: " + observerClientIds.Current, EditorStyles.label);
EditorGUILayout.TextField($"ClientId: {observerClientIds.Current}", EditorStyles.label);
}
}

Expand All @@ -82,8 +91,11 @@ public override void OnInspectorGUI()
else
{
base.OnInspectorGUI();
EditorGUILayout.LabelField("PrefabHash: ", m_NetworkObject.PrefabHash.ToString(), EditorStyles.label);
EditorGUILayout.LabelField("GlobalObjectIdHash64: ", m_NetworkObject.GlobalObjectIdHash64.ToString("X"), EditorStyles.label);

var guiEnabled = GUI.enabled;
GUI.enabled = false;
EditorGUILayout.TextField(nameof(NetworkObject.GlobalObjectIdHash), m_NetworkObject.GlobalObjectIdHash.ToString());
GUI.enabled = guiEnabled;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,21 @@
namespace MLAPI.Configuration
{
/// <summary>
/// A class that represents a NetworkPrefab
/// Class that represents a NetworkPrefab
/// </summary>
[Serializable]
public class NetworkPrefab
{
/// <summary>
/// Asset reference of the network prefab
/// </summary>
public GameObject Prefab;

/// <summary>
/// Whether or not this is a player prefab
/// </summary>
public bool IsPlayer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth the breaking change to rename this from PlayerPrefab to IsPlayer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var variable1 = somePrefabReference.Prefab;
var variable2 = somePrefabReference.PlayerPrefab;

// guess what `variable1` and `variable2` mean and what are the differences

vs

var variable1 = somePrefabReference.Prefab;
var variable2 = somePrefabReference.IsPlayer;

// guess what `variable1` and `variable2` mean and what are the differences


internal ulong Hash
{
get
Expand All @@ -18,7 +28,7 @@ internal ulong Hash
{
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
{
NetworkLog.LogWarning($"{nameof(NetworkPrefab)} is not assigned");
NetworkLog.LogWarning($"{nameof(NetworkPrefab)} does not have a prefab assigned");
}

return 0;
Expand All @@ -29,24 +39,14 @@ internal ulong Hash
{
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
{
NetworkLog.LogWarning($"{nameof(NetworkPrefab)} {Prefab.name} does not have a {nameof(NetworkObject)}");
NetworkLog.LogWarning($"{nameof(NetworkPrefab)} {Prefab.name} does not have a {nameof(NetworkObject)} component");
}

return 0;
}

return networkObject.PrefabHash;
return networkObject.GlobalObjectIdHash;
}
}

/// <summary>
/// The gameobject of the prefab
/// </summary>
public GameObject Prefab;

/// <summary>
/// Whether or not this is a playerPrefab
/// </summary>
public bool PlayerPrefab;
}
}
47 changes: 11 additions & 36 deletions com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -246,48 +246,27 @@ private void OnValidate()
NetworkLog.LogWarning($"{nameof(NetworkPrefab)} [{i}] does not have a {nameof(NetworkObject)} component");
}
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

{
NetworkConfig.NetworkPrefabs[i].Prefab.GetComponent<NetworkObject>().ValidateHash();
}
}
}

// TODO: Show which two prefab generators that collide
var hashes = new HashSet<ulong>();

for (int i = 0; i < NetworkConfig.NetworkPrefabs.Count; i++)
{
if (hashes.Contains(NetworkConfig.NetworkPrefabs[i].Hash))
{
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
{
var prefabHashGenerator = NetworkConfig.NetworkPrefabs[i].Prefab.GetComponent<NetworkObject>().PrefabHashGenerator;
NetworkLog.LogError($"PrefabHash collision! You have two prefabs with the same hash ({nameof(NetworkObject.PrefabHashGenerator)} = {prefabHashGenerator}). This is not supported");
}
}

hashes.Add(NetworkConfig.NetworkPrefabs[i].Hash);
}

int playerPrefabCount = NetworkConfig.NetworkPrefabs.Count(x => x.PlayerPrefab);
int playerPrefabCount = NetworkConfig.NetworkPrefabs.Count(x => x.IsPlayer);

if (playerPrefabCount == 0 && !NetworkConfig.ConnectionApproval && NetworkConfig.CreatePlayerPrefab)
{
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
{
NetworkLog.LogWarning($"There is no {nameof(NetworkPrefab)} marked as a {nameof(NetworkPrefab.PlayerPrefab)}");
NetworkLog.LogWarning($"There is no {nameof(NetworkPrefab)} marked as a {nameof(NetworkPrefab.IsPlayer)}");
}
}
else if (playerPrefabCount > 1)
{
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
{
NetworkLog.LogWarning($"Only one {nameof(NetworkPrefab)} can be marked as a {nameof(NetworkPrefab.PlayerPrefab)}");
NetworkLog.LogWarning($"Only one {nameof(NetworkPrefab)} can be marked as a {nameof(NetworkPrefab.IsPlayer)}");
}
}

var networkPrefab = NetworkConfig.NetworkPrefabs.FirstOrDefault(x => x.PlayerPrefab);
var networkPrefab = NetworkConfig.NetworkPrefabs.FirstOrDefault(x => x.IsPlayer);

if (networkPrefab == null)
{
Expand Down Expand Up @@ -395,10 +374,6 @@ private void Init(bool server)
NetworkLog.LogError($"{nameof(NetworkPrefab)} (\"{NetworkConfig.NetworkPrefabs[i].Prefab.name}\") is missing a {nameof(NetworkObject)} component");
}
}
else
{
NetworkConfig.NetworkPrefabs[i].Prefab.GetComponent<NetworkObject>().ValidateHash();
}
}

NetworkConfig.NetworkTransport.OnTransportEvent += HandleRawTransportPoll;
Expand Down Expand Up @@ -1363,9 +1338,9 @@ internal void OnClientDisconnectFromServer(ulong clientId)
{
if (ConnectedClients[clientId].PlayerObject != null)
{
if (SpawnManager.CustomDestroyHandlers.ContainsKey(ConnectedClients[clientId].PlayerObject.PrefabHash))
if (SpawnManager.CustomDestroyHandlers.ContainsKey(ConnectedClients[clientId].PlayerObject.GlobalObjectIdHash))
{
SpawnManager.CustomDestroyHandlers[ConnectedClients[clientId].PlayerObject.PrefabHash](ConnectedClients[clientId].PlayerObject);
SpawnManager.CustomDestroyHandlers[ConnectedClients[clientId].PlayerObject.GlobalObjectIdHash](ConnectedClients[clientId].PlayerObject);
SpawnManager.OnDestroyObject(ConnectedClients[clientId].PlayerObject.NetworkObjectId, false);
}
else
Expand All @@ -1380,9 +1355,9 @@ internal void OnClientDisconnectFromServer(ulong clientId)
{
if (!ConnectedClients[clientId].OwnedObjects[i].DontDestroyWithOwner)
{
if (SpawnManager.CustomDestroyHandlers.ContainsKey(ConnectedClients[clientId].OwnedObjects[i].PrefabHash))
if (SpawnManager.CustomDestroyHandlers.ContainsKey(ConnectedClients[clientId].OwnedObjects[i].GlobalObjectIdHash))
{
SpawnManager.CustomDestroyHandlers[ConnectedClients[clientId].OwnedObjects[i].PrefabHash](ConnectedClients[clientId].OwnedObjects[i]);
SpawnManager.CustomDestroyHandlers[ConnectedClients[clientId].OwnedObjects[i].GlobalObjectIdHash](ConnectedClients[clientId].OwnedObjects[i]);
SpawnManager.OnDestroyObject(ConnectedClients[clientId].OwnedObjects[i].NetworkObjectId, false);
}
else
Expand Down Expand Up @@ -1525,7 +1500,7 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, ulong

if (!NetworkConfig.EnableSceneManagement || NetworkConfig.UsePrefabSync)
{
writer.WriteUInt64Packed(observedObject.PrefabHash);
writer.WriteUInt64Packed(observedObject.GlobalObjectIdHash);
}
else
{
Expand All @@ -1534,11 +1509,11 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, ulong

if (observedObject.IsSceneObject == null || observedObject.IsSceneObject.Value)
{
writer.WriteUInt64Packed(observedObject.GlobalObjectIdHash64);
writer.WriteUInt64Packed(observedObject.GlobalObjectIdHash);
}
else
{
writer.WriteUInt64Packed(observedObject.PrefabHash);
writer.WriteUInt64Packed(observedObject.GlobalObjectIdHash);
}
}

Expand Down
32 changes: 2 additions & 30 deletions com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,16 @@ public sealed class NetworkObject : MonoBehaviour
{
[HideInInspector]
[SerializeField]
internal ulong GlobalObjectIdHash64;
internal ulong GlobalObjectIdHash;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as per @TwoTenPvP's suggestion, we're dropping 64 suffix here and simplifying its name because ulong makes it obvious already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to leave a comment next to (above or to the side) that explains the data source that the hash was derived from?
// GlobalObjectIdHash is derived from the following pattern: (PrefabPath) : (GlobalObjectId Guid)

Or...something to that nature?
Just thinking it could be useful to have that when trying to figure out how this new pattern works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not derived from {PrefabHash}:{GlobalObjectId} pattern, it's literally just the GlobalObjectId being hashed and that's it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it! I see the removal of the ValidateHash and the usage of the UnityEditor.GlobalObjectId.
Makes sense!


#if UNITY_EDITOR
private void OnValidate()
{
var globalObjectIdString = UnityEditor.GlobalObjectId.GetGlobalObjectIdSlow(this).ToString();
GlobalObjectIdHash64 = XXHash.Hash64(globalObjectIdString);

// Set this so the hash can be serialized on Scene objects. For prefabs, they are generated at runtime.
ValidateHash();
GlobalObjectIdHash = XXHash.Hash64(globalObjectIdString);
}
#endif

internal void ValidateHash()
{
if (string.IsNullOrEmpty(PrefabHashGenerator))
Copy link
Contributor

Choose a reason for hiding this comment

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

Per my comment, assuming there is in fact a reason a user might want to force-choose a hash, we could change the default to be null, but still allow them to "expert mode" override it

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 have 2 questions:

  1. what is the user-need?
  2. should we serve that need with PrefabHashGenerator string?

to me, even if the user-need is justified and we think it's important enough and valid to keep around, I wouldn't serve/satisfy that user-need with PrefabHashGenerator string implementation — it'd be an implementation on its own, relying on top of this infrastructure.

Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity Apr 2, 2021

Choose a reason for hiding this comment

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

I too would be curious to know what the use case for having two different prefabs that translate to the same hash but could be different assets? (the last part I am assuming would be the only scenario to consider for this condition to be useful)

The only scenario where this could be useful (IMO) would be to have different assets per platform (i.e. mobile vs console typically have vast deltas between assets). Under this scenario, I could see where being able to directly assign the Prefab hash could be useful if you had no other "good options" that were minimal effort for the desired result.

If this is the only reason for having two different prefabs with the same prefab hash, then this might be another "pro Addressables" scenario that could solve for this type of situation.

Otherwise, for a near future solution I am sure we could figure out some pattern to handle having a way to "link GlobalObjectIdHash" values together. Maybe something like a new MLAPI aware component (i.e. "NetworkPrefabLink") that could be applied to a GameObject inside of a "master prefab" asset. You could then add as many prefabs as you wanted as children under the root GameObject containing the NetworkPrefabLink component. From there, linking the prefabs and the logic to determine which prefab to use could be completely customizable by the user. The GlobalObjectIdHash of the "master prefab" would then become the GlobalObjectIdHash for any of the prefabs defined within it.

That is one way we could approach any potential breaking features/intended functionality while also providing a more expansive/modular pattern to the users at the same time. :)

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'm seeing 3 completely separate features here:

  1. Network Object
  2. Network Prefab
  3. Network Addressable

we're going to have the first 2 and leave the 3rd for later.

{
PrefabHashGenerator = gameObject.name;
}

PrefabHash = XXHash.Hash64(PrefabHashGenerator);
}

/// <summary>
/// Gets the NetworkManager that owns this NetworkObject instance
/// </summary>
Expand Down Expand Up @@ -86,21 +73,6 @@ internal set

internal ulong? OwnerClientIdInternal = null;

/// <summary>
/// The Prefab unique hash. This should not be set my the user but rather changed by editing the PrefabHashGenerator.
/// It has to be the same for all instances of a prefab
/// </summary>
[HideInInspector]
[SerializeField]
public ulong PrefabHash;

/// <summary>
/// The generator used to change the PrefabHash. This should be set the same for all instances of a prefab.
/// It has to be unique in relation to other prefabs
/// </summary>
[SerializeField]
public string PrefabHashGenerator;

/// <summary>
/// If true, the object will always be replicated as root on clients and the parent will be ignored.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ internal static void HandleConnectionRequest(ulong clientId, Stream stream)
if (NetworkManager.Singleton.NetworkConfig.ConnectionApproval)
{
byte[] connectionBuffer = reader.ReadByteArray();
NetworkManager.Singleton.InvokeConnectionApproval(connectionBuffer, clientId, (createPlayerObject, playerPrefabHash, approved, position, rotation) => { NetworkManager.Singleton.HandleApproval(clientId, createPlayerObject, playerPrefabHash, approved, position, rotation); });
NetworkManager.Singleton.InvokeConnectionApproval(connectionBuffer, clientId,
(createPlayerObject, playerPrefabHash, approved, position, rotation) =>
NetworkManager.Singleton.HandleApproval(clientId, createPlayerObject, playerPrefabHash, approved, position, rotation));
}
else
{
Expand Down
Loading