NVIDIAGameWorks / PhysX-3.4

NVIDIA PhysX SDK 3.4
https://www.nvidia.com/
2.35k stars 274 forks source link

RigidIDTracker - id recyclation issue #17

Closed lukashrcka123 closed 5 years ago

lukashrcka123 commented 7 years ago

There is an issue with tracking IDs of the actors. The following function is used to obtain ID for every actor added to scene:

        PxU32   getNewID()
        {
            // If recycled IDs are available, use them
            const PxU32 size = mFreeIDs.size();
            if(size)
            {
                // Recycle last ID
                return mFreeIDs.popBack();
            }
            // Else create a new ID
            return mCurrentID++;
        }

If we take a closer look to this function we would see that ids are recycled from list of free ids. If there is a free id in mFreeIDs buffer , then new actor will obtain the id of the last removed actor (every id of the removed actor is added at the end of the mFreeIDs buffer (as we can see in void freeID(PxU32 id) method). Now if we have a situation when we remove actor from scene and then we add a new actor, the new actor takes an id of the the removed actor. According to this we can get _PxContactPairHeaderFlag::eREMOVED_ACTORx flag in PxContactPairHeader raised for both actors. As we can see:

PxU16 headerFlags = 0;
    if (RigidIDTracker.isDeletedID(aPair.getActorAID()))
        headerFlags |= PxContactPairHeaderFlag::eREMOVED_ACTOR_0;
    if (RigidIDTracker.isDeletedID(aPair.getActorBID()))
        headerFlags |= PxContactPairHeaderFlag::eREMOVED_ACTOR_1;

scene just simply checks if actor id is in deleted ids.

kstorey-nvidia commented 7 years ago

Is this in response to a bug that you've observed? If so, could you please provide more details so we can try and repro the issue.

I've had a quick look through the code and I don't see a problem yet. While you're absolutely right that things could go wrong if these IDs became confused, there's a strict order that they're processed in that should guarantee that this doesn't happen. Specifically, the IDs are not returned to the pool/recycled until after the code working out if the actor has been removed.

You can remove and add actors either buffered or non-buffered. Buffered means you called addActor and removeActor after you called simulate() but before you called fetchResults.

In the buffered case, the actors remain in a buffered add/remove state until fetchResults is called. However, the contact callbacks are issues before the actors are "synced", which means that they're not removed/added until after this callback so the check to see if the actor has been removed should be reliable.

If the actors were removed/added in a non-buffered way (i.e. before calling simulate()), the interactions are destroyed immediately so you should not get any contact reports about any removed actors. If, however, you destroy actors inside the code processing these contact reports, PhysX has no way to guard against this. It's unsafe to destroy actors inside callbacks so you need to buffer these operations yourself and perform them after all callbacks have been processed.

lukashrcka123 commented 7 years ago

Yeah this is response to bug. I am using non-buffered way and i dont destroy actors inside the code which process contact reports. I have a situation like this: untitled

All actors are kinematic. In my filter shader is something like this:

if (physx::PxGetFilterObjectType(attributes0) != physx::PxFilterObjectType::eARTICULATION && physx::PxGetFilterObjectType(attributes1) != physx::PxFilterObjectType::eARTICULATION)
{
if ((filterData0.word0 & filterData1.word2) || (filterData1.word0 & filterData0.word2))
{
pairFlags |= physx::PxPairFlag::eDETECT_DISCRETE_CONTACT | physx::PxPairFlag::eNOTIFY_TOUCH_FOUND | physx::PxPairFlag::eNOTIFY_TOUCH_LOST;
}
}

For actor A and B is value word0 in filter data set to 0 and word2 set to 1. For actor C is word0 set to 1 and word2 is set to 0.

At first only actor A and B are part of the scene. Then I do something like this:


physx::PxScene* _scene;
...
_scene->removeActor(*_actorA, true);
_scene->addActor(*_actorB);
...
_scene->simulate(_simulationInterval);
_scene->fetchResults(true);

After that I receive event _PxPairFlag::eNOTIFY_TOUCHLOST with _PxContactPairHeaderFlag::eREMOVED_ACTORx flag set, for actor "C" which is ok. But after that I receive event _PxPairFlag::eNOTIFY_TOUCHFOUND for pair B-C also with _PxContactPairHeaderFlag::eREMOVED_ACTORx flag raised, which seems to me a little bit odd. I noticed that when I switch remove/add sequence to:

...
_scene->addActor(*_actorB);
_scene->removeActor(*_actorA, true);
... 

everything works fine.

kstorey-nvidia commented 7 years ago

I'll try to set up a repro based on your description. I'm still not sure though. Investigating the code more, the ObjectIDTracker also handles buffering so I don't see why switching around those addActor and removeActor calls would have any influence on the IDs that are provided. The destroyed ID is pushed into the pendingReleaseIDs list, which is flushed to the freeID list in postReportsCleanup, which gets called in either the last stage of fetchResults(), when the scene is destroyed() or when flush() is called on the scene. Anyway, I'll try to set up a repro based on your description and hopefully I'll see what you're seeing.

kstorey-nvidia commented 7 years ago

I was able to repro an issue and I have a fix. I think what I reproduced was what you are describing. The issue is actually a map being looked up using a pointer that could be re-used in this case, rather than the IDs, which seem to be handled correctly.

lukashrcka123 commented 7 years ago

Thanks. Lets hope that was the issue :)