Skip to content

fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project#905

Merged
NoelStephensUnity merged 3 commits intodevelopfrom
fix/issue_904
Jun 15, 2021
Merged

fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project#905
NoelStephensUnity merged 3 commits intodevelopfrom
fix/issue_904

Conversation

@NoelStephensUnity
Copy link
Member

This resolves the following issue:
Whilst upgrading Boss Room to MLAPI develop, we encountered an issue with the Network Manager and its NetworkPrefabs container, where if we delete any of the prefabs or variants that were added to the NetworkPrefabs list, then save (apply changes to) the NetworkingManager prefab, then shut down Unity and load it up again, the NetworkPrefabs list will be invalidated, most of the elements will have a null value.
Github Issue 904

Additional Notes:
This issue only arises if you make changes to a prefab instance and then apply those changes to the source prefab using Overrides->Apply All. This only applies to the NetworkPrefabs list generation while in the editor as this list is rebuilt during runtime and the only time you would want to generate the list while in the editor is if the currently opened scene is dirty and the asset database is not being updated.

Resolves the following github issue:
NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project #904
#904

Checking to make sure the scene is dirty and that the asset database is not currently updating resolves this issue.
Updated the comment for better clarity and assuring we have at least two commits in order to pass Yamato requirement of >1 commit
@NoelStephensUnity NoelStephensUnity changed the title Fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project Jun 14, 2021
@0xFA11
Copy link
Contributor

0xFA11 commented Jun 15, 2021

I'd like to defer this one to @Cosmin-B

I think I'll co-review :)

Comment on lines 264 to 265
// If the scene is dirty and the asset database is not currently updating then we can update NetworkPrefab information
if (activeScene.isDirty && !UnityEditor.EditorApplication.isUpdating)
Copy link
Contributor

Choose a reason for hiding this comment

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

IDK why Git is confused about the diff but this part is the only diff we actually have — and it looks good to me! 🚀

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it is because all of the indentions and line numbers changed when I enclosed all of the Network Prefab list stuff within that if statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks good to me, the only thing I am slightly worried about, is whether we can use a conditional early return/guard instead of enclosing all the code below, it makes it less readable given the Identations.

Comment on lines 264 to 265
// If the scene is dirty and the asset database is not currently updating then we can update NetworkPrefab information
if (activeScene.isDirty && !UnityEditor.EditorApplication.isUpdating)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks good to me, the only thing I am slightly worried about, is whether we can use a conditional early return/guard instead of enclosing all the code below, it makes it less readable given the Identations.

Updated logic to exit OnValidate as opposed to wrapping the NetworkPrefab related code for easier readability.
@NoelStephensUnity NoelStephensUnity merged commit 7e19026 into develop Jun 15, 2021
@NoelStephensUnity NoelStephensUnity deleted the fix/issue_904 branch June 15, 2021 22:33
SamuelBellomo added a commit that referenced this pull request Jun 22, 2021
* develop: (21 commits)
  feat: NetworkObject Parenting (#855)
  refactor: move RpcMethodId serialization from ILPP to Core (#910)
  fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project (#905)
  feat!: OnNetworkSpawn / OnNetworkDespawn (#865)
  feat: Add missing XMLdoc comment (#897)
  refactor: upgrade ILPP backend, drop 2019.4 support, rename types/fields (#895)
  fix: do not access/render runtime info if not playing in the editor (#898)
  feat: Add name property for network variables (#891)
  chore: delete PhilTestResults.xml (#894)
  feat: MultiInstanceHelpers to use fixed FrameRate by default (#893)
  test: General MultiInstanceHelper improvements (#885)
  refactor: isKinematic set to true for rigid bodies of non-authorized instances (#886)
  docs: adding more info to help debug on network transform error message (#892)
  feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (#875)
  fix: remove OnClientConnectedCallback registration from StatsDisplay (#882)
  feat: Add profiling decorator pattern (#878)
  refactor: Removing dead code for NETWORK_VARIABLE_UPDATE (#880)
  fix: update package version to 0.2.0 because of unity minversion change (#881)
  docs: Fix typo in changelog version title
  docs: Hotfix Changelog for 0.1.1 and manual update (#873)
  ...

# Conflicts:
#	com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs
#	com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs
#	com.unity.multiplayer.mlapi/Tests/Runtime/BaseMultiInstanceTest.cs
#	com.unity.multiplayer.mlapi/Tests/Runtime/BaseMultiInstanceTest.cs.meta
SamuelBellomo added a commit that referenced this pull request Jun 22, 2021
* develop: (67 commits)
  feat: NetworkObject Parenting (#855)
  refactor: move RpcMethodId serialization from ILPP to Core (#910)
  fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project (#905)
  feat!: OnNetworkSpawn / OnNetworkDespawn (#865)
  feat: Add missing XMLdoc comment (#897)
  refactor: upgrade ILPP backend, drop 2019.4 support, rename types/fields (#895)
  fix: do not access/render runtime info if not playing in the editor (#898)
  feat: Add name property for network variables (#891)
  chore: delete PhilTestResults.xml (#894)
  feat: MultiInstanceHelpers to use fixed FrameRate by default (#893)
  test: General MultiInstanceHelper improvements (#885)
  refactor: isKinematic set to true for rigid bodies of non-authorized instances (#886)
  docs: adding more info to help debug on network transform error message (#892)
  feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (#875)
  fix: remove OnClientConnectedCallback registration from StatsDisplay (#882)
  feat: Add profiling decorator pattern (#878)
  refactor: Removing dead code for NETWORK_VARIABLE_UPDATE (#880)
  fix: update package version to 0.2.0 because of unity minversion change (#881)
  docs: Fix typo in changelog version title
  docs: Hotfix Changelog for 0.1.1 and manual update (#873)
  ...

# Conflicts:
#	com.unity.multiplayer.mlapi/Tests/Runtime/com.unity.multiplayer.mlapi.runtimetests.asmdef
#	testproject/ProjectSettings/EditorBuildSettings.asset
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.

3 participants