Skip to content
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.

Sign up for GitHub

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

fix: avoid throwing exception when using NetworkList without a NetworkManager (#2539) #2540

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

fix: avoid throwing exception when using NetworkList without a NetworkManager (#2539) #2540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1feb94
fix: avoid throwing exception when using NetworkList without a Networ…
PitouGames Apr 30, 2023
aa83ec7
Merge branch 'develop' into fix/offline-network-variable
NoelStephensUnity May 12, 2023
8dc63f2
Merge branch 'develop' into fix/offline-network-variable
NoelStephensUnity May 12, 2023
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
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
### Fixed

- Fixed the "Generate Default Network Prefabs List" setting not loading correctly and always reverting to being checked. (#2545)
- Fixed usage of `NetworkList` throwing exception when used without a NetworkManager in scene. (#2539)
- Fixed the inspector throwing exceptions when attempting to render `NetworkVariable`s of enum types. (#2529)
- Making a `NetworkVariable` with an `INetworkSerializable` type that doesn't meet the `new()` constraint will now create a compile-time error instead of an editor crash (#2528)
- Fixed Multiplayer Tools package installation docs page link on the NetworkManager popup. (#2526)
Expand Down
12 changes: 6 additions & 6 deletions com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public IEnumerator<T> GetEnumerator()
public void Add(T item)
{
// check write permissions
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@PitouGames
What this effectively does is open the possibility that an instance can write to the NetworkList even if it won't have permissions when spawned. Upon spawning, the authority's values for this NetworkList will overwrite any values set which could lead to user confusion as to why the values aren't being applied.
(same for all cases below...but I am also struggling with the existing user confusion of not being able to set the values too)

It is a good idea... just need to noodle over it a bit more. 👍

Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Generally speaking, this was sort of why we allow for the assignment of values upon instantiating the instance (of course, that too can get overwritten)... which the way it currently is can be problematic if you want to expose it as a property in the inspector view and/or apply values before it is spawned but after it is instantiated.
(still pondering)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it opens the possibility that an instance writes to the NetworkList even if it won't have permissions when spawned, and this is what I need.

The issue is that today, doing an operation on the list throws an uninformative NullReferenceException instead of showing the warning you added specially for this case:
image

{
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
}
Expand All @@ -395,7 +395,7 @@ public void Add(T item)
public void Clear()
{
// check write permissions
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
{
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
}
Expand All @@ -421,7 +421,7 @@ public bool Contains(T item)
public bool Remove(T item)
{
// check write permissions
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
{
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
}
Expand Down Expand Up @@ -456,7 +456,7 @@ public int IndexOf(T item)
public void Insert(int index, T item)
{
// check write permissions
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
{
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
}
Expand Down Expand Up @@ -485,7 +485,7 @@ public void Insert(int index, T item)
public void RemoveAt(int index)
{
// check write permissions
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
{
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
}
Expand All @@ -508,7 +508,7 @@ public T this[int index]
set
{
// check write permissions
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
{
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
}
Expand Down