FirstGearGames / FishNet

FishNet: Unity Networking Evolved.
Other
1.39k stars 150 forks source link

Spawning a child NOB by instantiate uses global position and rotation instead of local #660

Closed AhonenAJ closed 6 months ago

AhonenAJ commented 7 months ago

General Unity version: 2022.3.23f1 Fish-Networking version: 4.2.2R Discord link: None (seems like a straightforward bug to me)

Description When you spawn a child NOB from a prefab, the child's initial coordinates are treated as global instead of local, causing it to position incorrectly in relation to its parent.

I'm not sure if there are other problem spots but at least this change fixed it for me in DefaultObjectPool.RetrieveObject.GetFromInstantiate:

NetworkObject result = Instantiate(prefab, pos, rot, parent);
result.transform.localScale = scale;

->

NetworkObject result = Instantiate(prefab, parent);
var tf = result.transform;
tf.localPosition = pos;
tf.localRotation = rot;
tf.localScale = scale;

Replication Steps to reproduce the behavior:

  1. Spawn an object at non-zero coordinates in the game world
  2. Immediately spawn a child object (with no network transform) for the first spawn at (0, 0, 0) local position

Expected behavior On the client, the child object is spawned at (0, 0, 0) local position under the parent and not at (0, 0, 0) global position.

Note that, with a network transform added to the child object, it worked for me as long as the client was already joined when the spawn triggered. If the spawn had triggered before the client joined, then the child position ended up being wrong again and the network transform did nothing to fix the issue.

FirstGearGames commented 6 months ago

That could have other consequences but this does sound like a bug. Thanks for the report.

FirstGearGames commented 6 months ago

I reviewed further and the default is to write local properties. Your fix should do the trick! I'll review further but I think we're okay with it.

FirstGearGames commented 6 months ago

Tested and working!

                    prefab.transform.OutLocalPropertyValues(nullableLocalPosition, nullableLocalRotation, nullableLocalScale, out Vector3 pos, out Quaternion rot, out Vector3 scale);                    
                    if (parent != null)
                    {
                        //Convert pos and rot to world values for the instantiate.
                        pos = parent.TransformPoint(pos);
                        rot = (parent.rotation * rot);
                    }
                    NetworkObject result = Instantiate(prefab, pos, rot, parent);
                    result.transform.localScale = scale;

Resolved in 4.3.0

AhonenAJ commented 1 week ago

Hello again,

It's been a while since I posted the original issue but I've only now had the chance to update and test the official fix in v4.5.5.

There seems to be something funky going on with setting the world position for the instiated object. I assume it has something to do with an animation I have running on one of the parent transforms but anyhow, I had to change the fix to the following to make it work:

if (localSpace)
{
    prefab.transform.OutLocalPropertyValues(nullablePosition, nullableRotation, nullableScale,
        out Vector3 pos, out Quaternion rot, out scale);
    result = Instantiate(prefab, parent);
    result.transform.SetLocalPositionAndRotation(pos, rot);
}

For some reason that I cannot fathom, setting the position as a world position puts it in the wrong spot instead of at (0, 0, 0) local. Even the animation I have playing in the parent is only moving on the Y axis but still the local position becomes non-zero on the X axis. And overall it's up to 0.5 units off from where it's supposed to be so it's pretty noticeable, although definitely not as broken as it was when I reported the original issue.

In any case, I'm making the above change for my own version of the object pool class. Please consider it for an upcoming release in case someone else runs into the same issue at some point.