feat: Prefab QoL [MTT-5116] #2322
Conversation
Moves prefab list to a ScriptableObject and makes it publicly accessible to be programmable. Credit for most of the work goes to @JesseOlmer, I provided some refinements and testing (including unit tests)
| @@ -35,5 +43,39 @@ internal static void SetAutoAddNetworkObjectSetting(bool autoAddSetting) | |||
| { | |||
| EditorPrefs.SetBool(AutoAddNetworkObjectIfNoneExists, autoAddSetting); | |||
| } | |||
|
|
|||
| public static NetcodeForGameObjectsSettings GetOrCreateSettings() | |||
There was a problem hiding this comment.
Comment: It seems a little odd to me that this class is a mixture of editor prefs and persistent prefs.
Suggestion: If you want to stay consistent with Unity's asset serialization and also be able to take advantage of things like [FormerlySerializedAs] you can use these APIs to read/write scriptable object assets to the project folder:
UnityEditorInternal.InternalEditorUtility.SaveToSerializedFileAndForget();
UnityEditorInternal.InternalEditorUtility.LoadSerializedFileAndForget();
Or alternatively, you can use ScriptableSingleton which does most of that for you automatically.
There was a problem hiding this comment.
I think I see what you're saying... but this doesn't actually need to be persistent prefs, it can use EditorPrefs everywhere. The mismatch was the result of merging two different implementations of this together and not noticing that they were implemented in different ways.
There was a problem hiding this comment.
I think they actually need to be separate - the existing editorprefs are really editor preferences. how should this editor on this PC behave. The prefab prefs are project-level... you want that behavior consistent on everyone's PC who is working on the project.
There was a problem hiding this comment.
Ah, fair. I didn't realize EditorPrefs were shared between projects.
There was a problem hiding this comment.
I had the same knee jerk reaction when looking at the original implementation, but there are advantages to having the NetworkPrefabs configuration serialized in reasonably "readable" json formatted text file. This could be easily converted over to a editor preferences via EditorPrefs.SetString, but I am not sure a netcode project's NetworkPrefab settings (specific to the game/product) qualifies as an editor preference.
Might be worth investigating the ScriptableObject path... just to determine if we are gaining or losing potential future functionality (either side of the fence).
The good news is that the json text can be easily saved to and read from whichever medium/location turns out to be the best place to store this information. 👍
There was a problem hiding this comment.
Switched it over to ScriptableSingleton. That eased some of the logic and it's still a plain text format. :)
| } | ||
| private static NetworkPrefabsList s_PrefabsList; | ||
|
|
||
| // Unfortunately this method is required by the asset pipeline to be static |
There was a problem hiding this comment.
Nitpick: This comment seems extraneous, it doesn't explain what's unfortunate about this pattern or why it shouldn't be static. As an engineer without context about this code, I think this actually detracts from it rather than improves it
There was a problem hiding this comment.
That comment came from @JesseOlmer so I won't speak for him as to the intention behind it.
There was a problem hiding this comment.
| // Unfortunately this method is required by the asset pipeline to be static |
This was mostly a note to myself during experimentation... It's unfortunate because static things are harder to mock, override, disable, etc. I didn't actually write ANY tests for any of this code so not sure how much this being static actually made things harder ;)
There was a problem hiding this comment.
Had a feeling that was your thought process here... and yeah, it made testing harder... tests are having to set static variables and make sure they unset them at the end because if they don't, the actual editor settings get changed... it's yucky.
There was a problem hiding this comment.
Yeah...it is what it is:
https://docs.unity3d.com/2022.2/Documentation/ScriptReference/AssetPostprocessor.OnPostprocessAllAssets.html
One of those scenarios where creating an internal set of callbacks might help (?) with testing (and be valid due to it being static and all).
| { | ||
| if (m_NetworkManager.NetworkConfig.Prefabs.NetworkPrefabsList == null) | ||
| { | ||
| EditorGUILayout.HelpBox("You have no prefab list selected. You will have to add your prefabs manually at runtime for netcode to work.", MessageType.Warning); |
There was a problem hiding this comment.
Question: Should this be Info? It seems like having no prefabs is a valid use case.
There was a problem hiding this comment.
It is a valid use case, but not an easy or intuitive one. People have to write their own code to manage the prefabs list on both sides. I felt warning was more appropriate to call out "this isn't going to work unless you're really putting effort into making it work"... but not opposed to changing it to info
com.unity.netcode.gameobjects/Runtime/Configuration/NetworkPrefabsList.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Editor/Configuration/NetcodeSettingsProvider.cs
Outdated
Show resolved
Hide resolved
- When performing a migration from the old prefab list to the new one, the path used to generate the new prefab list will be: - Next to the prefab containing the NetworkManager component if the NetworkManager is part of a prefab - Next to the scene the NetworkManager is in if it's not part of a prefab - NetworkManager can now have multiple prefab lists attached to it Also, I moved the code from NetworkAnimator's OnBeforeSerialize into the OnValidate event, as OnBeforeSerialize gets called in various circumstances that result in Unity throwing an error now that a NetworkAnimator can be a part of a NetworkPrefabsList.
# Conflicts: # com.unity.netcode.gameobjects/CHANGELOG.md # com.unity.netcode.gameobjects/Editor/Configuration/NetcodeSettingsProvider.cs
Moves prefab list to a ScriptableObject and makes it publicly accessible to be programmable. Credit for most of the work goes to @JesseOlmer, I provided some refinements and testing (including unit tests)
Changelog
Testing and Documentation