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: Exception with disabled GameObjects and NetworkTransforms #3243

Merged

Conversation

EmandM
Copy link
Collaborator

@EmandM EmandM commented Feb 7, 2025

MTTB-959

Changelog

  • Fixed: Log an appropriate error if the user has disabled a GameObject associated with a NetworkBehaviour. Do not throw an excpetion.

Testing and Documentation

  • Includes unit tests.

@EmandM EmandM requested a review from a team as a code owner February 7, 2025 18:07
EmandM and others added 7 commits February 7, 2025 13:08
Minor adjustment as it looks like DAHost might not have been properly forwarding messages of NetworkTransformMessages when the NetworkObject was hidden from it.
Some minor tweaks for extracting the reliability used.
updating the test LogAssert.Expect string used.
Just keeping the established pattern of using "nameof" within a string.
Also added the NetworkTransform's NetworkBehaviourId value as opposed to hard coding the value.
(minor tweaks)
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Had to add a fix for another issue I noticed while reviewing your PR (not due to your PR but that already existed).

Did a minor cosmetic update to the error message generation for LogAssert.Expect.

Other than that, it looks good to me!

As long as my fix didn't break something else! 😝

@@ -71,8 +71,21 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
if (isSpawnedLocally)
{
networkObject = networkManager.SpawnManager.SpawnedObjects[networkObjectId];
if (networkObject.ChildNetworkBehaviours.Count <= networkBehaviourId || networkObject.ChildNetworkBehaviours[networkBehaviourId] == null)
{
Debug.LogError($"[{nameof(NetworkTransformMessage)}][Invalid] Targeted {nameof(NetworkTransform)}, {nameof(NetworkBehaviour.NetworkBehaviourId)} ({networkBehaviourId}), does not exist! Make sure you are not spawning {nameof(NetworkObject)}s with disabled {nameof(GameObject)}s that have {nameof(NetworkBehaviour)} components on them.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice wordsmithing...

var transform = networkObject.ChildNetworkBehaviours[networkBehaviourId] as NetworkTransform;
if (transform == null)
{
Debug.LogError($"[{nameof(NetworkTransformMessage)}][Invalid] Targeted {nameof(NetworkTransform)}, {nameof(NetworkBehaviour.NetworkBehaviourId)} ({networkBehaviourId}), does not exist! Make sure you are not spawning {nameof(NetworkObject)}s with disabled {nameof(GameObject)}s that have {nameof(NetworkBehaviour)} components on them.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -81,8 +94,20 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
}
else
{
// Deserialize the state
reader.ReadNetworkSerializableInPlace(ref State);
ownerAuthoritativeServerSide = networkManager.DAHost;
Copy link
Collaborator

Choose a reason for hiding this comment

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

While reviewing your PR, I noticed a bug in DAHost mode.
Just including the fix for that here.

@@ -132,7 +151,10 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
ownerClientId = context.SenderId;
}

var networkDelivery = State.IsReliableStateUpdate() ? NetworkDelivery.ReliableSequenced : NetworkDelivery.UnreliableSequenced;
// Depending upon whether it is spawned locally or not, get the deserialized state
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the 2nd part of the fix where State was never being set and it wasn't adjusting based on whether the host, server, or DAHost had a NetworkTransform instance.

yield return WaitForConditionOrTimeOut(() => ObjectSpawnedOnAllClients(networkObjectInstance.NetworkObjectId));
AssertOnTimeout("Timed out waiting for object to spawn!");

var errorMessage = $"[Netcode] {nameof(NetworkBehaviour)} index {networkTransformInstance.NetworkBehaviourId} was out of bounds for {m_NonAuthorityPrefab.name}(Clone). " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor adjustments.

@EmandM EmandM merged commit cd6fd5b into develop-2.0.0 Feb 7, 2025
24 checks passed
@EmandM EmandM deleted the fix/mttb-959-exception-with-disabled-gameobjects branch February 7, 2025 23:22
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.

2 participants