NVIDIAGameWorks / PhysX

NVIDIA PhysX SDK
Other
3.11k stars 793 forks source link

PxRigidDynamic::setRigidDynamicLockFlags ignored when applied to active rigidbody #573

Open TheDonsky opened 2 years ago

TheDonsky commented 2 years ago

Hello,

I noticed that Invoking PxRigidDynamic::setRigidDynamicLockFlags during simulation has no apparent effect. Those flags simply reset to the old value the next frame.

One thing that actually works, is detaching the PxRigidDynamic body from the scene, changing those flags and attaching it back, but even then, that has to happen over multiple frames (ei if I just detach, set the flags and reattach without calling fetchResults in-between, those flags are lost on the next physics update)

Is this an expected behavior? Can I do anything about it other than creating a convoluted scheme where I detach body before setRigidDynamicLockFlags and reattach after fetchResults()?

Here is a small recording of what I'm experiencing: https://youtu.be/spAW4VDDu-o Enabling/disabling component triggers add/remove actor calls and changing the "Lock" field invokes setRigidDynamicLockFlags; The video does not show it, but I checked and unless you call fetchResults() and simulate(), the lock flags will stay set (they return to old values after those; haven't tested what happens in between those two)

Here's GitHub link to the project, just in case: https://github.com/TheDonsky/Jimara

Thanks in advance

kstorey-nvidia commented 2 years ago

This sounds like a genuine bug. Will investigate and report back as soon as we have a chance.

TheDonsky commented 2 years ago

Thanks. I'll be waiting for the Update As a side note, it may be useful, I've built the SDK as static .lib-s (no DLL-s) and PhysX commit is from April 23, 2021 (I think it's the latest public one) Issue is present on both Debug and Release configurations; I don't use others, but I'd expect the same there too.

peter1745 commented 1 year ago

I'm currently experiencing this issue as well. It'd be great if a fix for this issue could be pushed. @TheDonsky any luck fixing this on your end?

peter1745 commented 1 year ago

@kstorey-nvidia Any news on this?

TheDonsky commented 1 year ago

I'm currently experiencing this issue as well. It'd be great if a fix for this issue could be pushed. @TheDonsky any luck fixing this on your end?

Not yet; still waiting. I can "hack it" by disabling & enabling on physics synch and the user will not know the difference, but I'd rather we have a cleaner solution from NVIDIA.

peter1745 commented 1 year ago

Not yet; still waiting. I can "hack it" by disabling & enabling on physics synch and the user will not know the difference, but I'd rather we have a cleaner solution from NVIDIA.

Yeah definitely, I wasn't able to get that hack working, so really hoping that they fix this

TheDonsky commented 1 year ago

Yeah definitely, I wasn't able to get that hack working, so really hoping that they fix this

The way I remember it, you need detach the body for at least one physx::PxScene::simulate call. Just detaching, changing the flags and attaching does not work. I have OnPrePhysicsSynch and OnPostPhysicsSynch events in my engine with the simulate() call in-between and that enables the hack in my case if I add a "dirty" flag for my Rigidbody component. Eventually I may add that bit of logic there, but yes, I hope they fix it before that.

peter1745 commented 1 year ago

The way I remember it, you need detach the body for at least one physx::PxScene::simulate call. Just detaching, changing the flags and attaching does not work. I have OnPrePhysicsSynch and OnPostPhysicsSynch events in my engine with the simulate() call in-between and that enables the hack in my case if I add a "dirty" flag for my Rigidbody component. Eventually I may add that bit of logic there, but yes, I hope they fix it before that.

That's good to know, I might try and get that working if Nvidia don't fix this issue, for our purposes I simply made it so we can switch between static and dynamic actors during runtime, which achieves the same behaviour that we're looking for, but having lock flags working is still a must obviously

jcarius-nv commented 1 year ago

Hi @TheDonsky , I looked at the video in your original post, indeed something is misbehaving there.

I tried to reproduce but in my tests the flags are actually set correctly, so I'm a bit confused what's different in your setup. I also tried it directly on one of the snippets with the Github source code and even there it looks fine. When I apply the following patch to the SnippetHelloWorld the lockFlags are applied after 100 steps to body number 19 and the body stays indeed locked when I shoot a ball at it (spacebar).

diff --git a/physx/snippets/snippethelloworld/SnippetHelloWorld.cpp b/physx/snippets/snippethelloworld/SnippetHelloWorld.cpp
index 1bb8246a..3a1a0273 100644
--- a/physx/snippets/snippethelloworld/SnippetHelloWorld.cpp
+++ b/physx/snippets/snippethelloworld/SnippetHelloWorld.cpp
@@ -114,7 +114,7 @@ void initPhysics(bool interactive)
    PxRigidStatic* groundPlane = PxCreatePlane(*gPhysics, PxPlane(0,1,0,0), *gMaterial);
    gScene->addActor(*groundPlane);

-   for(PxU32 i=0;i<5;i++)
+   for(PxU32 i=0;i<1;i++)
        createStack(PxTransform(PxVec3(0,0,stackZ-=10.0f)), 10, 2.0f);

    if(!interactive)
@@ -123,8 +123,41 @@ void initPhysics(bool interactive)

 void stepPhysics(bool /*interactive*/)
 {
+   const PxU32 bufferSize = 20;
+   PxActor* actorBuffer[bufferSize];
+   gScene->getActors(PxActorTypeFlag::eRIGID_DYNAMIC, actorBuffer, bufferSize);
+   PxRigidDynamic* dyn = actorBuffer[bufferSize-1]->is<PxRigidDynamic>();
+
+   const PxRigidDynamicLockFlags startFlags = dyn->getRigidDynamicLockFlags();
+   printf("Lockflags at beginning of step %u\n", PxU8(startFlags));
+
+   static PxU32 numSteps = 0;
+   numSteps++;
+
+   if(!startFlags && numSteps > 100)
+   {
+       PxRigidDynamicLockFlags setFlags(63);
+       dyn->setRigidDynamicLockFlags(setFlags);
+   }
+
+   {
+       const PxRigidDynamicLockFlags flags = dyn->getRigidDynamicLockFlags();
+       printf("Lockflags before simulate %u\n", PxU8(flags));
+   }
+
    gScene->simulate(1.0f/60.0f);
+
+   {
+       const PxRigidDynamicLockFlags flags = dyn->getRigidDynamicLockFlags();
+       printf("Lockflags after simulate %u\n", PxU8(flags));
+   }
+
    gScene->fetchResults(true);
+
+   {
+       const PxRigidDynamicLockFlags flags = dyn->getRigidDynamicLockFlags();
+       printf("Lockflags after fetchResults %u\n", PxU8(flags));
+   }
 }

 void cleanupPhysics(bool /*interactive*/)
TheDonsky commented 1 year ago

Hi @jcarius-nv, Thanks for the reply. I'll try to "break" one of the snippets in a few days, when I find more time and will let you know if I find something.

As for my setup, I've built PhysX as a static library instead of default dll, am using around 4 threads (using PxDefaultCpuDispatcher with quarter of std::hadware_concurrency) for simulation and am using a scratch buffer (size = (1 << 28)); CCD is enabled (although not active for all bodies).

TheDonsky commented 1 year ago

Hello again @jcarius-nv, I finally managed to find some time to play around with the snippet. It was quite simple to break the behavior. All we need to do is to set the flags between simulate() and fetchResults(). It's the way I do it in my engine (that way we enable update cycle to overlap with asynchronous physx simulations and from what I can gather from documentation, that is a valid way to do it).

jcarius-nv commented 1 year ago

ok, now I understand it and I can also reproduce it. The issue is that you're setting the flag between simulate() and fetchResults(), and I was just testing the lockflag setting before the simulate call.

~~Your usage is unfortunately not allowed because the simulation is still running until you call fetchResults(). Only after fetchResults() has completed API writes are allowed again. In the snippet and the tests I see warnings on the console invalid operation : PxRigidDynamic::setRigidDynamicLockFlags() not allowed while simulation is running. Call will be ignored. if I try to do what you're doing with checked builds.~~

I just checked the guide and there's actually a drawing that should make things clear in the Asynchronous Simulation section.

The good news is: The solution workaround to your problem is easier than detaching and re-inserting the body into the scene. Just set the lockflags when the simulation is not running, i.e., before you call simulate().

Edit: I still need to check the role of double-buffering in this situation, which should actually take care of your usecase. Will get back to you on that one, but I hope you can work with the different order already.

Edit2: Forget what I said above, it's only true without double-buffering. I'm checking why it isn't working as intended here.

TheDonsky commented 1 year ago

Thanks. Fortunately, this engine is just my passion project with no deadline or urgency (I'm mostly concentrated on other stuff for now), so I can wait for the answer before I make any changes.

I'm using Debug and Release modes and my initial assumption was that I should have been getting all the error messages Checked build would produce in Debug, but I may be mistaken there...

Anyway, from when I initially read the guide, I remember that double buffering used to let us modify velocity and pose while simulation was still running and there were precise rules about which value would be taken after fetchResults(). Don't remember if there were any exceptions listed, but it could be a special case, even if that sounds a little inconsistent.

@peter1745, is simulate() and fetchResults() order the same in your application?

peter1745 commented 1 year ago

We do simulate() and then fetchResults after, and we're definitely not running any code between those, and I'm still having this issue

jcarius-nv commented 1 year ago

I can confirm that there's a missing flush operation in the double-buffering scheme such that the RigidDynamicLockFlags set in the buffer never make it to the simulation. We've fixed it in the source and it will appear in the next release. For now, you can apply the patch yourself: In line ScbBody.h, Line 1061, add the line

flush<Buf::BF_RigidDynamicLockFlags>(buffer);

I hope this solves your problem! Thanks for reporting.

peter1745 commented 1 year ago

@jcarius-nv I'll be sure to give this a go this week and I'll you know how it goes!

TheDonsky commented 1 year ago

@jcarius-nv Thanks a lot! I'll give it a shot as soon as I can.

TheDonsky commented 1 year ago

@jcarius-nv I applied the patch and can confirm that the issue is gone on my side.

Do we close the issue "here and now" or after the new release?

(There's also the question of if the fix covers @peter1745's case and we should probably wait for him too)

peter1745 commented 1 year ago

@TheDonsky @jcarius-nv This did not appear to fix my issue. Basically here's the exact steps I'm taking:

For reference this is how I simulate the scene right now:

for (uint32_t i = 0; i < m_NumSubSteps; i++)
{
    m_PhysXScene->simulate(m_SubStepSize);
    m_PhysXScene->fetchResults(true);
}

Meaning there's no possible way that we're changing the state in-between those calls (since we're running all the relevant code on the same thread)

jcarius-nv commented 1 year ago

Hm, I don't know how I can help you further without an actual scene that can reproduce your issue.

I tried reproducing once again in the SnippetHelloWorld by adding

    body->setRigidDynamicLockFlags(PxRigidDynamicLockFlags(
        PxRigidDynamicLockFlag::eLOCK_LINEAR_X |
        PxRigidDynamicLockFlag::eLOCK_LINEAR_Y |
        PxRigidDynamicLockFlag::eLOCK_LINEAR_Z 
    ));

and then releasing the lock after a couple of steps but I didn't see any explosions.

Could it be related to that separate actor that gets destroyed immediately on collision ?

peter1745 commented 1 year ago

Could it be related to that separate actor that gets destroyed immediately on collision ?

Hi! Sorry for the late response, to be honest I won't rule out that the other actor is causing some problems, I will try to investigate it a bit this month, and I'll let you know what I find!