Stale aggregate self collision pairs cause removal of pairs belonging to the same actor

[NBG-Mindaugas]

Hello, we were experiencing a crash in Sc::Scene::unregisterInteraction. This assert was also firing PX_ASSERT(sceneArrayIndex != PX_INVALID_INTERACTION_SCENE_ID), which suggested an invalid interaction was being unregistered.

We also noticed the assert PX_ASSERT(&elementHi->getActor() != &elementLo->getActor()); firing in NPhaseCore::onOverlapRemoved.

I've tried upgrading to PhysX 5.4.2 but found it to be reproduceable there too.

This took a while, but I think I managed to find the root cause of this, detailed below.

Library and Version

PhysX 5.3, PhysX 5.4.2

Operating System

Windows 11

Steps to Trigger Behavior

1. Create 2 aggregates with multiple bodies jointed together. Have each body contain at least 2 shapes. The aggregates should have self collisions enabled.
2. Run the simulation. The jointed bodies should create self collision pairs inside each the aggregate. Lets say aggregate 0 creates a self collision pair between Element ID 5 and Element ID 6.
3. Set the eKINEMATIC flag on each body in aggregate 0. This will cause each shape to be re-inserted into the broadphase with new Element IDs.
4. On the same frame, immediately after Step 3, set eSIMULATION_SHAPE to false on each shape in aggregate 0.
5. This will cause PersistentPairs::updatePairs to no longer run on aggregate 0.
6. Run the simulation. The old Element IDs on aggregate 0 will be released into the ID pool for re-use.
7. Set the eKINEMATIC flag on each body in aggregate 1. This will cause each shape to be re-inserted into the broadphase with new Element IDs. Aggregate 1 will get assigned Element IDs that used to belong to Aggregate 0. Lets say that Aggregate 1 has an actor with two shapes. Lets say those shapes get assigned Element ID 5 and Element ID 6.
8. Set eSIMULATION_SHAPE back to true on each shape of aggregate 0. Set eKINEMATIC flag on each body of aggregate 0 back to false.
9. Run the simulation. Aggregate 0 runs PersistentPairs::updatePairs. Since no self collision pairs resolve to the existing pair of Element ID 5 and Element ID 6, this pair gets marked for removal. Notice that the shapes with Element ID 5 and Element ID 6 don't even belong to aggregate 0 anymore.
10. Since shapes with Element ID 5 and Element ID 6 now belong to the same actor on aggregate 1, we get this assert firing PX_ASSERT(&elementHi->getActor() != &elementLo->getActor());.

I've not tracked down the exact sequence of events that then leads to the crash in Sc::Scene::unregisterInteraction, but given that PersistentPairs::updatePairs outputs to AABBManagerBase::mDestroyedOverlaps, which is used to invoke unregisterInteraction - I can see how this might cause a crash in certain circumstances, given that we're trying to unregister an invalid overlap.

Fix/Workaround

I didn't try upgrading our project to latest PhysX to check whether this is still an issue or not as that takes decent effort, but from the diffs I couldn't spot changes that would suggest this issue has been fixed. I'm not an expert on the PhysX codebase though, so:

1. Is this issue fixed in later PhysX versions?
2. If not, this is what I've implemented locally as a workaround:

class Aggregate : public PxUserAllocated
{
        ...

	PX_FORCE_INLINE	bool							removeAggregated(BoundsIndex i)
        {
            removeSelfCollisionPairsWithId(mSelfCollisionPairs, i);

            return mAggregated.findAndReplaceWithLast(i);	
        }	// PT: TODO: optimize?

        ...
}

where:

void removeSelfCollisionPairsWithId(PersistentSelfCollisionPairs* pairs, BoundsIndex i)
{
    pairs->removePairsWithId(i);
}

where:

class PersistentSelfCollisionPairs : public PersistentPairs
{
    ...

    void removePairsWithId(BoundsIndex indexToRemove)
    {
        // MK_NOTE: Carbon copy of PersistentPairs::updatePairs, modified to only remove pairs with certain ID
        PxU32 i = 0;
        PxU32 nbActivePairs = mPM.mNbActivePairs;
        while (i < nbActivePairs)
        {
            InternalPair& p = mPM.mActivePairs[i];
            const PxU32 id0 = p.getId0();
            const PxU32 id1 = p.getId1();

            if (indexToRemove == id0 || indexToRemove == id1)
            {
                const PxU32 hashValue = hash(id0, id1) & mPM.mMask;
                mPM.removePair(id0, id1, hashValue, i);
                nbActivePairs--;
            }
            else
            {
                i++;
            }
        }
        mPM.shrinkMemory();
    }

    ...
}

This is probably far from ideal, so I would be grateful if someone could either point me to a proper fix if one exists, and if not - at least verifying whether what I've done here looks safe enough as a local workaround.

Let me know if I can provide any more info.

Sorry for the long read, but complex issues don't fit into few words. :)

Thanks,
Mindaugas

[vreutskyy]

Hi @NBG-Mindaugas. Thanks for the detailed report. We'll check it.