search

vrchat-community / UdonSharp

A compiler for compiling C# to Udon assembly
https://udonsharp.docs.vrchat.com
MIT License
470 stars 50 forks source link

Fixed implements of instantiate #17

Closed ikuko closed 2 years ago

ikuko commented 2 years ago

Fixed a bug in the implementation of Instantiate in U#1.0.

We have prepared the following simple test code, which compares the results of C# and U# (InstantiationShim) behavior. https://gist.github.com/ikuko/49b8275946e9404cac4551b0ee7f36e9 This is on gist as I wasn't sure how to properly include it in the repository.

github-actions[bot] commented 2 years ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

ikuko commented 2 years ago

I have read the CLA Document and I hereby sign the CLA

MerlinVR commented 2 years ago

I'm still confused at the purpose of this like with the original PR. The tests are calling into InstantiationShim from C# which doesn't actually work and will just throw null refs or return null if they don't outright exception. Due to floating point imprecision, it's also quite likely that the equality checks won't pass even if the tests did run.

Can you please give the exact cases that fail with the current instantiation logic? I tested calling public static GameObject Instantiate(GameObject original, Transform parent, bool worldPositionStays) with true and false for worldPositionStays, I also tested calling public static GameObject Instantiate(GameObject original, Transform parent) and as far as I could tell, in all cases they behave the same as their C# counterparts when you spawn something under a parent transform.

Currently the only thing I can confirm that this change does is that it breaks instantiation behavior in-game. In particular do the following to see what I mean:

  1. Make a Cube gameobject
  2. Translate, rotate, scale it off of defaults
  3. Save it as a prefab
  4. Make a U# script that calls Instantiate(thePrefabReference) where thePrefabReference is some field populated with a reference to the prefab
  5. Upload the scene in Build & Test
  6. Observe that the cube is instantiated at the scene origin with an identity rotation when instead it should be spawned with the offset and rotation stored in the prefab's transform

This is what the logic you removed from the Instantiate method in InstantiationShim fixes.

MerlinVR commented 2 years ago

I don't think this is relevant any longer considering the code has changed and VRChat has fixed the underlying issue. If there are still issues please make a new pull request outlining exactly what situations do not act like Unity Instantiate calls.