Bug / Crash IdentifyMovingColliders.Execute OutOfBound crash

[Guillaume]

Hi!

We recently had a crash in a obi job "IdentifyMovingColliders.Execute" (see attached picture to have the exact line).
We are using Obi Rope 6.4 but the code I'll talks about remains the same in 6.5

After few investigation, the crash seems to happen when for some reasons we loaded, then unloaded some scenes containing colliders without letting them do the UpdateIfNeeded on the tracker. By doing this, the shapes remaings zeroed (just allocated).

I wanted to check what was the initial cause of the bug by reading a bit more the sourcecode, and there is something I'm missing.

1. BurstColliderWorld.cellSpans will always grow to at least match the colliderCount (as seen in BurstColliderWorld.SetColliders)
   
2. When preparing the IdentifyMovingColliders parralel for job (inside BurstColliderWorld.UpdateWorld), we transfer ObiColliderWorld.colliderShapes into a NativeArray by doing so: 
   shapes = world.colliderShapes.AsNativeArray<BurstColliderShape>(cellSpans.count).
   We then have a shapes native array matching the cellSpans.count... but not world.colliderShapes.count
   
3. Then we schedule the parrallel for with cellSpans.count as parameter, so we are iterating on non initialized or removed shapes sometimes (since cellSpans.count >= colliderShapes.count)
   
4. Since we loaded/unloaded the shapes without them having time to UpdateIfNeeded, the shape.materialIndex stay at 0 (and the other parameters of the shape are zero-ed too!)
   
5. We then test shapes[i].materialIndex >= 0 which is true (since we are not using material, we should have put a -1 in the skipped UpdateIfNeeded)
   
6. Then we crash on the line velocityBounds.Expand(collisionMaterials[shapes[i].materialIndex].stickDistance); since collisionMaterials is empty (that make me think we should use materials for our ropes!)
   

I don't understand why we are basing the parrallel for on cellSpans.count rather than world.colliderShapes.count, because here this parrallel for will always access some "old shape data" and process them while it's not necessary.

But maybe I'm missing something here!

The code I would see in BurstColliderWorld.UpdateWorld:

Code:

            var identifyMoving = new IdentifyMovingColliders
            {
                movingColliders = movingColliders.AsParallelWriter(),
                shapes = world.colliderShapes.AsNativeArray<BurstColliderShape>(), // <--- Match the world.colliderShapes count
                // instead of: shapes = world.colliderShapes.AsNativeArray<BurstColliderShape>(cellSpans.count),
                rigidbodies = world.rigidbodies.AsNativeArray<BurstRigidbody>(),
                collisionMaterials = world.collisionMaterials.AsNativeArray<BurstCollisionMaterial>(),
                bounds = world.colliderAabbs.AsNativeArray<BurstAabb>(), // <--- same here
                // instead of: bounds = world.colliderAabbs.AsNativeArray<BurstAabb>(cellSpans.count),
                cellIndices = cellSpans.AsNativeArray<BurstCellSpan>(),
                colliderCount = colliderCount,
                dt = deltaTime
            };
            JobHandle movingHandle = identifyMoving.Schedule(colliderCount, 128); // <--- instead of (cellSpans.count, 128)

Then in the Execute code I think we will not go out of bounds since "i" is only used on:

1. bounds[i] which be the size of shapes 
   
2. shapes[i] since we matched the size of the initial shapes array
   
3. shapes[i].materialIndex >= 0 test will always be relevent since we are not iterating on a "ghost" shape that could have been added to the shapes, then removed before doing the UpdateIfNeeded thing (since UpdateIfNeeded is called in the ObiColliderWorld.GetInstance().UpdateColliders(); during the ObiUpdater.BeginStep)
   

Code:

        protected void BeginStep(float stepDeltaTime)
        {
            using (m_BeginStepPerfMarker.Auto())
            {
                // Update colliders right before collision detection:
                ObiColliderWorld.GetInstance().UpdateColliders(); // <----- This is where UpdateIfNeeded are call, where shapes are filled with correct values
                ObiColliderWorld.GetInstance().UpdateRigidbodies(solvers,stepDeltaTime);
                ObiColliderWorld.GetInstance().UpdateWorld(stepDeltaTime); // <------ This is where the IdentifyMovingColliders parrallel for takes place

Do you also think that this parrallel for is doing too much loop/work over time?
(maybe there are also other jobbed thing that do the same, but maybe I'm missing why)

Thanks!

[Guillaume]

Well, I realize that there is still a place in the IdentifyMovingColliders.Execute that is testing if we are a "removed" shape/collider by testing "i >= colliderCount".

And that the cellSpans count is indeed reduced if it doesn't match the colliderCount, so it's not only growing in count (only in capacity!)

Code:

            // remove tail from the current spans array:
            if (colliderCount < cellSpans.count)
                cellSpans.count -= cellSpans.count - colliderCount;

So I guess the only issue we had is that we manage to have a zeroed shape that was still doing some computation and accessing an out of bound index in the collisionMaterials array that is the only unprotected access in the code, and we should have not noticed the out of bound access if we had at least one collision material !

It was not the point of this topic, but the question that came to me is: is it important for the simulation to have collisionMaterials? Or the default values without material are okay?

[josemendez]

Hi Guillaume!

The reason why UpdateWorld iterates over cell spans instead of colliders is because we want to remove invalid colliders.

When a collider is invalidated for some reason (for instance, it has been destroyed), it is moved to the end of the collider list, using a typical swapback operation while keeping the capacity of the list intact.

So at any given time, all invalid colliders are past colliders.count but before cellSpans.count. It is fine to access past colliders.count, since a lists' backing array is always larger than its count. (capacity >= count)

If you look at the end of the IdentifyMovingColliders method, you'll see a comment explaining this:

// if the collider is at the tail (removed), we will only remove it from its current cellspan.
// if the new cellspan and the current one are different, we must remove it from its current cellspan and add it to its new one.
if (i >= colliderCount || cellIndices[i] != newSpan)
{
        // Add the collider to the list of moving colliders:
        movingColliders.Enqueue(new MovingCollider()
        {
                        oldSpan = cellIndices[i],
                        newSpan = newSpan,
                        entity = i
        });

      // Update previous coords:
      cellIndices[i] = newSpan;
}

I see no images attached to your email, and no stacktrace/description of the error you're getting, other than it happens in "IdentifyMovingColliders.Execute". Could you share more information?

kind regards,

[Guillaume]

Well... I must be dumb or something.

I was saying that Obi 6.5 was the same as our Obi 6.4 issue... but in fact Obi 6.5 will fix the issue we have because the Shape initialization changed!

Obi 6.4

Code:

colliderShapes.Add(new ColliderShape());

Obi 6.5

Code:

colliderShapes.Add(new ColliderShape() { materialIndex = -1, rigidbodyIndex = -1});

So that was just it

Sorry for all the noise. Next time I'll do more researches before asking around!

[josemendez]

So I guess the only issue we had is that we manage to have a zeroed shape that was still doing some computation and accessing an out of bound index in the collisionMaterials array that is the only unprotected access in the code, and we should have not noticed the out of bound access if we had at least one collision material !

Access to the collisionMaterials array is protected, at least in Obi 6.5.4 which is the current version:

Code:

// Expand bounds by collision material's stick distance:
if (shapes[i].materialIndex >= 0)
    velocityBounds.Expand(collisionMaterials[shapes[i].materialIndex].stickDistance);

There should be no case where a destroyed collider has a materialIndex other than -1, let me know if this is what you're getting.

It was not the point of this topic, but the question that came to me is: is it important for the simulation to have collisionMaterials? Or the default values without material are okay?

You can have no collisionMaterial. In that case, friction, stickiness, etc will all be considered to be zero.

[josemendez]

Well... I must be dumb or something.

Quite the contrary, that was a pretty in-depth analysis of the issue!

So that was just it

Sorry for all the noise. Next time I'll do more researches before asking around!

No problem. Let me know if I can be of any help!

[Guillaume]

I see no images attached to your email, and no stacktrace/description of the error you're getting, other than it happens in "IdentifyMovingColliders.Execute". Could you share more information?

I was pretty sure I joined the picture the first time... I missed everything I guess!

Here is the line.

But I think that the new initialization of shapes in Obi 6.5 will prevent this issue to ever happen!

Thanks for your reply