feat!: GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator #698

0xFA11 · 2021-04-01T20:36:44Z

we're replacing PrefabHash and PrefabHashGenerator string with GlobalObjectIdHash.

0xFA11 · 2021-04-01T20:37:42Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

-        internal ulong GlobalObjectIdHash64;
+        internal ulong GlobalObjectIdHash;


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

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.

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

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

mattwalsh-unity · 2021-04-02T00:32:13Z

com.unity.multiplayer.mlapi/Runtime/Configuration/NetworkPrefab.cs

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


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

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

mattwalsh-unity · 2021-04-02T00:35:02Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs

@@ -239,48 +239,27 @@ private void OnValidate()
                            NetworkLog.LogWarning($"{nameof(NetworkPrefab)} [{i}] does not have a {nameof(NetworkObject)} component");
                        }
                    }
-                    else


mattwalsh-unity · 2021-04-02T00:45:10Z

So as I was wondering "I wonder why we didn't just do it this way before", I am reminded that there is a use case where one would want to manually enter in that hash string, which is where you might want to have 2 different prefabs hash to the same value to support having the client and server have their own prefabs.

My questions are (@TwoTenPvP pls chime in)

does this 'deliberately making 2 prefabs have the same hash' pattern actually work / have been successfully used? If this was just a musing on how one might solve this (real) problem (because IIRC you would then need to write custom code to support it) then cool. Else, we need to have a conversation about the impact on customers / how to deal with that.

mattwalsh-unity · 2021-04-02T00:50:44Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

        }
 #endif

-        internal void ValidateHash()
-        {
-            if (string.IsNullOrEmpty(PrefabHashGenerator))


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

I have 2 questions:

what is the user-need?

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.

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. :)

I'm seeing 3 completely separate features here:

Network Object

Network Prefab

Network Addressable

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

mattwalsh-unity

Implementation looks clean, no objections, but before merging I'd like to make sure the 'force a hash' case is no longer desired

TwoTenPvP · 2021-04-03T14:17:08Z

So as I was wondering "I wonder why we didn't just do it this way before", I am reminded that there is a use case where one would want to manually enter in that hash string, which is where you might want to have 2 different prefabs hash to the same value to support having the client and server have their own prefabs.

My questions are (@TwoTenPvP pls chime in)

does this 'deliberately making 2 prefabs have the same hash' pattern actually work / have been successfully used? If this was just a musing on how one might solve this (real) problem (because IIRC you would then need to write custom code to support it) then cool. Else, we need to have a conversation about the impact on customers / how to deal with that.

Yes. This was a supported usecase, either server specific prefabs or separate projects.

NoelStephensUnity

This looks good to me.
Like the consolidation of the GlobalObjectHashId and related updates.
Based on Albin's response, it would seem it was for a similar type of situation as I outlined above where there could be differences in assets (due to platform or even potentially version) that have the same prefab hash.
To solve for this we can just implement something like the NetworkPrefab that will be able to handle this scenario.

0xFA11 added 6 commits April 1, 2021 20:53

GlobalObjectIdHash64 to GlobalObjectIdHash (drop 64 suffix)

e7f6dd0

GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator

4987ae5

remove GetPrefabHashFromGenerator

68c12c1

fix serialized assets

9007fd0

name tweak

ec20ca8

minor refactor

464953a

0xFA11 requested review from TwoTenPvP, mattwalsh-unity and NoelStephensUnity April 1, 2021 20:36

0xFA11 commented Apr 1, 2021

View reviewed changes

0xFA11 marked this pull request as draft April 1, 2021 20:37

simply WriteSpawnCallForObject

211af61

mattwalsh-unity reviewed Apr 2, 2021

View reviewed changes

mattwalsh-unity suggested changes Apr 2, 2021

View reviewed changes

0xFA11 marked this pull request as ready for review April 2, 2021 12:51

NoelStephensUnity approved these changes Apr 5, 2021

View reviewed changes

NoelStephensUnity requested a review from mattwalsh-unity April 5, 2021 15:31

0xFA11 added 7 commits April 7, 2021 12:00

revert UnityConnectSettings.asset changes

04cf094

make NetworkObjectEditor more fancy

0ffa18f

merge branch 'develop' into 'feature/prefabhash-to-gobjid'

2a78bca

fix naming error

0447eef

Merge branch 'develop' into feature/prefabhash-to-gobjid

0d31ef5

render DestroyWithScene property in the NetworkObject inspector view

1721ed6

render GlobalObjectIdHash as plain decimal instead of hexadecimal

2a6619c

mattwalsh-unity approved these changes Apr 8, 2021

View reviewed changes

Merge branch 'develop' into feature/prefabhash-to-gobjid

9ebe885

Merge branch 'develop' into feature/prefabhash-to-gobjid

4c00315

0xFA11 enabled auto-merge (squash) April 8, 2021 15:06

change but not a change really

a45bbb0

0xFA11 merged commit 186c31d into develop Apr 8, 2021

0xFA11 deleted the feature/prefabhash-to-gobjid branch April 8, 2021 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat!: GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator #698

feat!: GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator #698

Uh oh!

0xFA11 commented Apr 1, 2021 •

edited

Loading

Uh oh!

0xFA11 Apr 1, 2021

Uh oh!

NoelStephensUnity Apr 2, 2021

Uh oh!

0xFA11 Apr 2, 2021

Uh oh!

NoelStephensUnity Apr 5, 2021

Uh oh!

mattwalsh-unity Apr 2, 2021

Uh oh!

0xFA11 Apr 2, 2021

Uh oh!

mattwalsh-unity Apr 2, 2021

Uh oh!

mattwalsh-unity commented Apr 2, 2021

Uh oh!

mattwalsh-unity Apr 2, 2021

Uh oh!

0xFA11 Apr 2, 2021

Uh oh!

NoelStephensUnity Apr 2, 2021 •

edited

Loading

Uh oh!

0xFA11 Apr 2, 2021

Uh oh!

mattwalsh-unity left a comment

Uh oh!

TwoTenPvP commented Apr 3, 2021

Uh oh!

NoelStephensUnity left a comment •

edited

Loading

Uh oh!

Uh oh!

		internal ulong GlobalObjectIdHash64;
		internal ulong GlobalObjectIdHash;

feat!: GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator #698

feat!: GlobalObjectIdHash to replace PrefabHash and PrefabHashGenerator #698

Uh oh!

Conversation

0xFA11 commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity commented Apr 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

TwoTenPvP commented Apr 3, 2021

Uh oh!

NoelStephensUnity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xFA11 commented Apr 1, 2021 •

edited

Loading

NoelStephensUnity Apr 2, 2021 •

edited

Loading

NoelStephensUnity left a comment •

edited

Loading