Bug / Crash Obi adding huge forces to rigidbodies when you delete ropes... (Repro project)

[Hatchling]

So, this time I found that when you delete ropes, it can cause NaNs and absurd velocity assignments to rigidbodies when you destroy a rope's GameObject.

The repro package can be found here: (Expires in 30 days)
https://ufile.io/45i4log9

To reproduce:

1. Start a new Unity project (I'm using 2020.2.0f1).
   
2. Add Burst (version 1.4.8) and Jobs (version 0.8.0-preview.23) through the Package Manager.
   
3. Import the package linked above.
   
4. Open SampleScene.unity and press Play.
   
5. Click and drag to attach a rope. The point where you click (mouse down) will be the first point where a rope gets attached. The point where you let go (mouse up) will be the second point where a rope gets attached. Add several ropes.
   
6. Press [R] to delete all of the ropes you created.
   
7. Repeat 4 and 5 until the bug is reproduced.
   

You'll see this when you reproduce the bug:

• The rigidbodies in the scene may start behaving abnormally, with ghost forces pushing them around even though all of the ropes have been removed.
  
• You'll see errors spamming the console regarding invalid forces being applied to one or more of the rigidbodies. For example: rigidbody.velocity assign attempt for 'Floating Platform' is not valid. Input velocity is { 2.468001, NaN, 55.692116 }.
  
• Some of the rigidbodies may explode into outer space from the absurd forces.
  

Seems related to the previous bug I posted, with very similar symptoms.

Other symptoms I've noticed in fighting with this bug:

• When I start SampleScene with a rope in the scene already, (drag ObiPrefabs/Rope into the scene and activate the GameObject before pressing play) any ropes you add to the scene will be frozen and will not simulate.
  
• After reproducing the above bug: When switching to the scene "RopeNet" (which uses a modified example script to use the global solver), pressing play, and then unpressing play, the console will spam about some phantom Obi.ObiLateFixedUpdater trying to poke at various destroyed Obi components. When this occurs, you'll find that there are no ObiLateFixedUpdater instances visible in the hierarchy.
  

Code modifications to note:

• ObiStatic.cs added, which enables a global solver.
  
• ObiStaticSettings.cs added, which allows you to define the global solver using a prefab reference.
  
• When an ObiRope is created, it will check for a solver in one of its ancestors. If one isn't found, it'll use ObiStatic.WorldSolver, which will get instantiated if needed.
  
• RopeNet.cs will use ObiStatic.WorldSolver instead of creating its own solver.

[josemendez]

Hi,

Thanks for the repro!

First thing that caught my eye in your scene is that the ropes are not children of the solver, yet they seem to work. Debugging trough the code to see why, I see you've modified ObiActor's SetSolver() method like this:

protected void SetSolver(ObiSolver newSolver)
        {
            if(newSolver == null)
                newSolver = ObiStatic.WorldSolver;

            // In case the first solver up our hierarchy is not the one we're currently in, change solver.
            if (newSolver != m_Solver)
            {
                RemoveFromSolver();

                m_Solver = newSolver;

                AddToSolver();
            }
        }

When ropes are not children of any solver, they'll pick up your static solver instead regardless of where's located in the scene. This *might* not work in all cases, because lots of operations that convert back and forth between solver and actor space assume the solver is a parent of the actor. This is specially true for inertial forces. Can't really say what side effects you'll hit down the road by doing this, would have to look at it in detail.

As for the bug, I've narrowed it down: the solver was trying to read rigidbody velocities from uninitialized memory positions. Modify ObiSolver's EnsureRigidbodyArraysCapacity() method to look like this:

Code:

public void EnsureRigidbodyArraysCapacity(int count)
{
            if (count >= rigidbodyLinearDeltas.count)
            {
                rigidbodyLinearDeltas.ResizeInitialized(count);
                rigidbodyAngularDeltas.ResizeInitialized(count);

                if (initialized)
                    m_SolverImpl.SetRigidbodyArrays(this);
            }
}

That will fix it.

Let me know if I can be of further help. cheers!

[Hatchling]

When ropes are not children of any solver, they'll pick up your static solver instead regardless of where's located in the scene. This *might* not work in all cases, because lots of operations that convert back and forth between solver and actor space assume the solver is a parent of the actor. This is specially true for inertial forces. Can't really say what side effects you'll hit down the road by doing this, would have to look at it in detail.

The global solver prefab gets instantiated with an identity transformation (pos 0,0,0, rotation 0,0,0,1, scale 1,1,1) and doesn't move, so I dunno if it'd actually show up as an issue. BUT, I could look at modifying the code to work when those assumptions can't be applied.

Also, do you think fixing this issue would also fix some of the side effects of the DontDestroyOnLoad() problem mentioned in the other post?

---

As for the bug, I've narrowed it down: the solver was trying to read rigidbody velocities from uninitialized memory positions. Modify ObiSolver's EnsureRigidbodyArraysCapacity() method to look like this:

Code:

public void EnsureRigidbodyArraysCapacity(int count)
{
            if (count >= rigidbodyLinearDeltas.count)
            {
                rigidbodyLinearDeltas.ResizeInitialized(count);
                rigidbodyAngularDeltas.ResizeInitialized(count);

                if (initialized)
                    m_SolverImpl.SetRigidbodyArrays(this);
            }
}

That will fix it.

Are there any other places in the code where a similar issue will take place?

It appears like some sort of initialization takes place in an unexpected order, judging from the change. I'm not sure what my approach to using ObiSolver would screw up outside of this.

I was considering having it be the case that all ropes that are meant to be controlled by the global solver are parented to it. However, this approach might not work well since some prefabs will also contain ropes, and reparenting child objects could have problems:

• If a prefab uses GetComponentInChildren to find obi rope references, they'd stop working unexpectedly. Or when writing new behaviours, it might be easy to forget that that rope which was originally in the prefab is no longer there after being instantiated.
  
• If a prefab is destroyed at runtime as a whole, you'd have ropes left-over that don't get destroyed with the rest of the prefab.
  

[josemendez]

The global solver prefab gets instantiated with an identity transformation (pos 0,0,0, rotation 0,0,0,1, scale 1,1,1) and doesn't move, so I dunno if it'd actually show up as an issue. BUT, I could look at modifying the code to work when those assumptions can't be applied.

Also, do you think fixing this issue would also fix some of the side effects of the DontDestroyOnLoad() problem mentioned in the other post?

Been trying to reproduce the DontDestroyOnLoad() issue, but I'm only able to in your project. The "standard" Obi does not exhibit this, so I'm guessing this could be related to the issue.

Are there any other places in the code where a similar issue will take place?

Lots of places in Obi assume actors are children of solvers, since it's designed to work that way. The code does not check for this, since it can't happen by design: if an actor can't find a solver up its hierarchy, it won't be added to any solver. I'm not 100% sure what would happen otherwise.

It appears like some sort of initialization takes place in an unexpected order, judging from the change. I'm not sure what my approach to using ObiSolver would screw up outside of this.

Me neither. It is just not a supported use case. Doesn't mean it is not possible to get it to work in that way, but it's not covered by any of our unit tests and most of the code does not even consider the possibility of an actor not having a solver up its hierarchy.

I was considering having it be the case that all ropes that are meant to be controlled by the global solver are parented to it. However, this approach might not work well since some prefabs will also contain ropes, and reparenting child objects could have problems:

• If a prefab uses GetComponentInChildren to find obi rope references, they'd stop working unexpectedly. Or when writing new behaviours, it might be easy to forget that that rope which was originally in the prefab is no longer there after being instantiated.
  
• If a prefab is destroyed at runtime as a whole, you'd have ropes left-over that don't get destroyed with the rest of the prefab.
  

It's not needed for actors to be immediate children of a solver. They just need to be inside its hierarchy. Typically, you'd have prefabs with ObiRope components in them, at different hierarchy depths. At runtime you instantiate the prefab, place it under a solver, and you're done.

Say you have a character with chains (ropes) parented to and hanging from different parts of his body. The entire character is a prefab, you just instantiate the prefab and then put the entire character under a solver. If the character uses GetComponentInChildren() to get a hold of his chains, it will just work. If the character is destroyed, all chains will be too. No left-over stuff or tedious handling of object references.

This is also how UI in Unity works: Graphics must be under a Canvas for them to be updated and rendered / Actors must be under a Solver for them to be updated and rendered. It's the exact same pattern: you can build entire UI widgets as prefabs, instantiate them inside a canvas and have them working.

[Hatchling]

Well, it sounds like the only supported solution to have every rope collide with one another is to have the entire game parented to a ObiSolver GameObject? And to write our own Object.Instantiate() method that parents everything we instantiate to that object, in case it contains a rope?

It'd be very difficult to work with PhysX if it had those kinds of inflexibilities.

[josemendez]

Well, it sounds like the only supported solution to have every rope collide with one another is to have the entire game parented to a ObiSolver GameObject?

Having every rope collide with every other rope does require to have all your ropes under the same solver.  However that's not usually what you want, unless everything in your game is made of ropes and can potentially move to anywhere else in the world.

And to write our own Object.Instantiate() method that parents everything we instantiate to that object, in case it contains a rope?

That's literally 2 lines of code, doesn't seem like much of a drawback to me.

It'd be very difficult to work with PhysX if it had those kinds of inflexibilities.

Actually PhysX works *exactly* the same way: you need to include Actors in a Scene for them to be simulated. Actors in different Scenes do not interact with each other. From their docs:

https://docs.nvidia.com/gameworks/conten...ction.html

The basic concepts of the world within a PhysX simulation are easy to visualize:
- The PhysX world comprises a collection of Scenes, each containing objects called Actors;
- Each Scene defines its own reference frame encompassing all of space and time;
- Actors in different Scenes do not interact with each other;

Unity creates a single Scene for you under the hood and puts all physics objects inside it for you. This actually gets in the way sometimes and has been a huge source of frustration for many people (including me) over the years. You didn't have explicit control over scenes or a way to have more than one physics scene until recently, and even now it's done in a quite clunky way:

https://learn.unity.com/tutorial/multi-scene-physics

This is the same architectural pattern used by Flex, Bullet, ODE, Box2D, Havok, and all other physics engines I've ever used or heard of. It's is not an inflexibility imho, but quite the opposite: it allows you to explicitly partition your world into independent simulations, allows you to perform local-space simulation (which is an absolute must for large game worlds) and it does so in a way consistent with how Unity works.

[Hatchling]

I agree that having the ability to have independent scenes that do not interact, when desired, is a big plus. I've encountered situations in Unity where I wanted to run a copy of parts of the scene and step ahead several iterations to predict where things will land for things like AI. In a general context, yes, this is desirable flexibility.

However, in the context of Unity, where the hierarchy and the way components and game objects are parented can have meaning that varies for different projects and prefabs, it is an inflexibility. Sometimes, this is fine, as inflexibilities allow for consistency. Going back to PhysX and how it is used in Unity, the ability for a rigidbody to have a collider that isn't parented to the rigidbody wouldn't make much sense. 

On the other hand, if Unity had used a similar philosophy for Colliders as Obi, where in order for two colliders to interact they'd need to be parented to the same game object, there would be things that'd be a lot more difficult to do. For example, if you used additive scene loading, the two scenes' contents wouldn't collide unless everything that contained a Collider migrated over to another game object. Depending on how the scene and prefabs are set up, this could break existing relationships.

In your defense, I can understand how most use cases for Obi - e.g. cloth on characters - this would make sense. In these use cases it is a bit like a skinned mesh with fancier math attached. If Obi were more often used in the same way PhysX is, where players can play with it arbitrarily in some cases and very few assumptions about what the players may do can be made, I hope you can see why it wouldn't work. This is why I made the comparison.

A suggestion I'd put forth is to use parenting by default for the majority of simpler use cases. But in cases where game object parenting relationships are reserved for other purposes, it'd be a good option to allow actors to simply reference which scene they're a part of in a separate field. (Perhaps scenes could even be scriptable objects?) Otherwise, they'd fall back to a default scene when no scene is specified.

Requiring that developers cannot use the built-in Instantiate method provided in Unity for a feature to work correctly can be an issue. In the case of third-party assets, it'd at best be annoying to have to modify all of their code for compliance (assuming compatibility), and at worst, it'd be potentially unworkable without serious hack-arounds in case where you do not have source code access. Our project is early on enough that we could just ban the use of Object.Instantiate outside of our special replacement method that ensures compliance with Obi. But, as our project grows and other features are added - especially third party features - and they compete for control over how objects are laid out, it could become a headache that doesn't need to happen.

This all being said, no, not everything in our game is made of ropes, cloth, or fluid. But there will be plenty of objects that do have these things, and they can be spawned in arbitrarily. They'll need to be able to interact, and can move anywhere in the scene.

[josemendez]

In your defense, I can understand how most use cases for Obi - e.g. cloth on characters - this would make sense. In these use cases it is a bit like a skinned mesh with fancier math attached. If Obi were more often used in the same way PhysX is, where players can play with it arbitrarily in some cases and very few assumptions about what the players may do can be made, I hope you can see why it wouldn't work. This is why I made the comparison.

I get what you mean. However, the typical use case for Obi is for it to be restricted to a relatively small spatial area: character cloth, a crane, a fluid container, some softbody eye candy, just to name a few. In these cases, it's natural for the solver to be at the root of the prefab (character, crane, container, etc). Information about the "outside world" (Unity colliders and rigidbodies) is converted to the solver's local space, so things can still collide with the environment and/or be attached to it regardless of where the solver resides. Only actor inter-collisions are restricted by this.

In this regard, Obi uses parenting as a merely organizational tool. It does not make sense to parent a rope to another rope, or cloth to a fluid emitter, just like it does not make (much) sense to parent a rigidbody to another rigidbody.

A suggestion I'd put forth is to use parenting by default for the majority of simpler use cases. But in cases where game object parenting relationships are reserved for other purposes, it'd be a good option to allow actors to simply reference which scene they're a part of in a separate field.

That's how Obi v1 and v2 worked: all actors had a "solver" field in the inspector, that got populated by the first solver found in their hierarchy by default. You were able to select a different solver if needed. Simulation could be performed either in the solver's local space (if parented) or world space (if not part of the same hierarchy). However this had a few drawbacks:

- Supporting world space simulation only is not an option, so code paths for world and local space had to coexist, making the code more verbose, less efficient (in world space's case) and harder to maintain.
- A single solver could have some actors in world space, and some actors in local space, which made things confusing. Also, inertial controls in the solver only affected local space actors which made things even more wonky.

I took a look at how Unity solves this relationship between a "manager" object and "managed" objects. In most cases  (UI, Animation, Rigging, etc) this is done by parenting the managed objects to the manager. This approach solves the above issues single-handedly: unified code path for everything, efficient simulation, and consistent behavior in all cases.

Requiring that developers cannot use the built-in Instantiate method provided in Unity for a feature to work correctly can be an issue.

The built-in Instantiate method can take a parent as argument, so you can use  Instantiate(Object original, Transform parent); to set the parent right away. So instead of:

Code:

Instantiate(myPrefab);

you'd do:

Code:

Instantiate(myPrefab, mySolver);

If you had to assign a solver explicitly, you'd need to do something like:

Code:

var instance = Instantiate(myPrefab);
instance.GetComponentInChildren<ObiActor>().solver = mySolver;

I'd argue that this second version is awkward. It would get uglier if you had more than one actor inside your prefab, as you'd have to iterate trough them all.

In the case of third-party assets, it'd at best be annoying to have to modify all of their code for compliance (assuming compatibility), and at worst, it'd be potentially unworkable without serious hack-arounds in case where you do not have source code access. But, as our project grows and other features are added - especially third party features - and they compete for control over how objects are laid out, it could become a headache that doesn't need to happen.

This could be said of virtually any third-party asset/middleware. Only trivial stuff fits right into an existing codebase. Integrating a new physics engine into your game is often a quite painful experience, we've tried to minimize that within reasonable effort.

Our project is early on enough that we could just ban the use of Object.Instantiate outside of our special replacement method that ensures compliance with Obi.

I don't think you need to write a special replacement, the default Instantiate would work fine as long as you set the correct parent.

This all being said, no, not everything in our game is made of ropes, cloth, or fluid. But there will be plenty of objects that do have these things, and they can be spawned in arbitrarily. They'll need to be able to interact, and can move anywhere in the scene.

Will think of a way to reduce setup hassle for something like this. Quite often, less hassle == less flexibility, so this has to be carefully considered.

[Hatchling]

I see.

Well, I'll consider the approach you've suggested, though for now I'll see where my hackier world solver stuff takes me. It might just work as is anyway. If I run into more problems that are blatantly obvious such that you'd probably have caught them, I'll assume it is due to the world solver approach.

I'm guessing as far as the spatial math goes, since the global solver is always using a identity transform, it'll most likely be identical to having the object parented to it.
As far as how Unity's events are sent out (Awake, OnDestroy, etc.) I can see this potentially being an issue. Child objects, I think, are always initialized after parent objects, whereas with how the global solver is set up, the solver comes after the first obi actor comes into existence. And they are destroyed in the order they appear in the hierarchy.

If I had modified the code to safely handle scenarios where the parenting assumption does not hold, and these changes were tested with unit tests, weren't an obstacle to the direction you were planning to take it, didn't raise any red flags, didn't harm performance, etc. would you be willing to adopt them? IMO anything that makes something robust to more unforseen in-the-wild use cases, provided it isn't at the cost of simplicity or other valuable qualities, is probably a worthwhile contribution. (This doesn't mean you need to advertise it as a supported feature, but who knows when another Hatchling will come along and try to throw convention under the bus.)