-
Notifications
You must be signed in to change notification settings - Fork 450
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
Changes from all commits
e7f6dd0
4987ae5
68c12c1
9007fd0
ec20ca8
464953a
211af61
04cf094
0ffa18f
2a78bca
0447eef
0d31ef5
1721ed6
2a6619c
9ebe885
4c00315
a45bbb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,48 +246,27 @@ private void OnValidate() | |
NetworkLog.LogWarning($"{nameof(NetworkPrefab)} [{i}] does not have a {nameof(NetworkObject)} component"); | ||
} | ||
} | ||
else | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
{ | ||
|
@@ -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); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,29 +22,16 @@ public sealed class NetworkObject : MonoBehaviour | |
{ | ||
[HideInInspector] | ||
[SerializeField] | ||
internal ulong GlobalObjectIdHash64; | ||
internal ulong GlobalObjectIdHash; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? Or...something to that nature? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
#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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have 2 questions:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm seeing 3 completely separate features here:
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> | ||
|
@@ -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> | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vs