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.

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

Conversation

PitouGames
Copy link
Contributor

@PitouGames PitouGames commented Apr 30, 2023

Resolve #2539

This PR add a check that m_NetworkBehaviour is not null before calling the CanClientWrite function.

I did it exactly the same way it's already done for NetworkVariable.

With this PR, operations on the list are done and throws the anyoning warning ( NetworkVariable<T> do the same)

image

Changelog

Testing and Documentation

  • No tests have been added.
  • No documentation changes or additions are necessary.

@PitouGames PitouGames requested a review from a team as a code owner April 30, 2023 21:41
@PitouGames PitouGames force-pushed the fix/offline-network-variable branch from 6081b6e to b1feb94 Compare May 2, 2023 20:57
@0xFA11 0xFA11 requested a review from jeffreyrainy May 2, 2023 21:10
@@ -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

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

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

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.

Can't do operations on NetworkList with no NetworkManager (offline)
2 participants