Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug with TGS + linear limit or linear drive between Kinematic and Dynamic body #409

Closed
alanjfs opened this issue Mar 31, 2021 · 19 comments
Closed

Comments

@alanjfs
Copy link

alanjfs commented Mar 31, 2021

Hi all,

Re-posting this from within a longer thread for visibility and clarity, given the problem is now pretty clearly a bug.

Original issue


Problem

With a PxD6Joint between a kinematic and dynamic rigid body, both limits and drives misbehave under TGS, but not PGS. The dynamic body appears to inherit too much velocity from the kinematic body.

TGS

Case Demo
TGS Linear Limit bug
PGS Linear Limit good
TGS Drive bugdrives

The problem is made much worse with a rotating kinematic body.

Case Demo
PGS Linear Drive kinematicrotating
TGS Linear Drive tgsrotation

Reproducible

Lines of interest are..

And the result as seen from PVD, where TGS appears to introduce additional velocity to the dynamic rigid when jointed to an animated kinematic rigid, whereas PGS does not.

bugtgs
pgsgood


PxDistanceJoint

The problem can also be seen with other joints, like the PxDistanceJoint

PxJoint* createJoint(PxRigidActor* parent, const PxTransform& parentTm,
                     PxRigidActor* child, const PxTransform& childTm) {

    PxDistanceJoint* joint = PxDistanceJointCreate(*gPhysics, parent, parentTm, child, childTm);
    joint->setDistanceJointFlag(PxDistanceJointFlag::eMAX_DISTANCE_ENABLED, true);
    joint->setDistanceJointFlag(PxDistanceJointFlag::eSPRING_ENABLED, true);
    joint->setMaxDistance(0.0f);
    joint->setStiffness(100.f);
    joint->setDamping(100.f);

    return joint;
}

distancealsobad

Any idea what a fellow like myself can do to fix this?

Thanks

@kstorey-nvidia
Copy link

Sorry for the late reply. Could you try looking at the following method:

setupSolverConstraintStep

This line:
if(isKinematic1)
s.velMultiplier += vel1;

should be

if(isKinematic1)
s.velTarget += vel1;

Could you please let me know if this helps at all?

@kstorey-nvidia
Copy link

Ah, sorry. I just realized that I had already given you this fix. I will try to set up a repro

@alanjfs
Copy link
Author

alanjfs commented Apr 9, 2021

There is also a repro linked above, amongst all of the gifs.

@kstorey-nvidia
Copy link

Thanks. I've downloaded it already and will hopefully be able to take a look at it probably first thing next week

@kstorey-nvidia
Copy link

Thanks for the repro. This actually works correctly in our development trunk. I'll do a bit of spot the difference and hopefully come back with a fix for you

@alanjfs
Copy link
Author

alanjfs commented Apr 16, 2021

Any new insights @kstorey-nvidia? I am at the edge of my seat. :)

@alanjfs
Copy link
Author

alanjfs commented Apr 22, 2021

Just to confirm, you meant:

  • It is working in your internal build
  • And you were able to reproduce the problem on the build from this repo?

Or could you not reproduce the problem using this repo?

@kstorey-nvidia
Copy link

Sorry for radio silence. I was able to fix the issue, but the fix was a bit too involved to describe here. I've fixed internally and the fix is included in the update that just hit GitHub moments ago.

@kstorey-nvidia
Copy link

Closing this issue. Reopen if you test and find the issue is not resolved.

@alanjfs
Copy link
Author

alanjfs commented Apr 22, 2021

Thank you so much! I will try this in the next 24 hours. If you don't hear from me, it's because I'm in paradise. ❤️

@alanjfs
Copy link
Author

alanjfs commented Apr 22, 2021

Works like a charm. :)

Before

beforetgs

After

fixedtgs

@alanjfs
Copy link
Author

alanjfs commented Apr 23, 2021

Oh no! This change drastically changes unrelated parts of the solver! :(((

Before After
tgsbefore2 tgsafter2

In this example, the original issue was not present (no softness on translate limits or drives) and yet it still changed by a lot. To the point where I'm having trouble even producing the final look even after tweaking (increasing) values.

Here are a few possible reasons I can see.

  1. This is the way of the future, and PhysX 5 will maintain this new behavior
  2. This is a one-off for PhysX 4 only and will change once more with PhysX 5
  3. This was a mistake, a new patch is being submitted ASAP

In either case, my questions:

  1. Is there a configuration flag I can set to retain the previous behavior?
  2. Is there a new value/ratio for e.g. positionIterations I can set (tried it, no such luck..)?
  3. Is there anything at all I can do to retain the previous behavior with TGS?

I can get a semi-similar result by increasing stiffness/damping by a factor of 1000x, and doubling the positionIterations (from 32 to 64) on every rigid, however that increases the cost of each frame by 25% and isn't close enough to qualify for backwards compatibility. :S

semisimilar

I would re-open the issue since it's related, but I am unable to (permissions?). @kstorey-nvidia Help!

@kstorey-nvidia
Copy link

Hi Alan

Reopening. I think I have introduced a typo while back-porting the fix from PhysX 5. Unfortunately, the code diverged too much to be able to just merge things.

Can you try changing this code in setupSolverConstraintStep(...) in DyTGSContactPrep.cpp:

PxReal velTarget = 0.f;
	if (isKinematic0)
		velTarget += vel0 * s.velMultiplier;
	if (isKinematic1)
		velTarget -= vel1 * s.velMultiplier;

to this:

PxReal velTarget = 0.f;
if (isKinematic0)
velTarget += vel0;
if (isKinematic1)
velTarget -= vel1;

I moved the velTarget calculation above the call setSolverConstants, but that means that the velMultiplier scaling happens inside that call now so it shouldn't be scaled by velMultiplier before being passed in.

Your repro worked with this change, as did all of our unit tests, but it is definitely not the correct code.

I'll see if we can get a spot-fix submitted for that ASAP. In the meantime, could you update that and see if it resolves your issue? This should get you correct/equivalent behavior hopefully.

If this doesn't work, please upload a PVD capture and I will investigate more.

@alanjfs
Copy link
Author

alanjfs commented Apr 23, 2021

Thanks for your swift response on this! Unfortunately, it appears to have had no effect at all. This simulation have no kinematic rigids involved, other than the ones getting kicked around on the ground, so I'm not sure I would expect changes to the kinematic portion to have an effect.

4.1.2 Before Fix 4.1.2 After Fix 4.1.1 Original
bug_with_tgs_4 1 2_beforefix bug_with_tgs_4 1 2_afterfix bug_with_tgs_4 1 1

@kstorey-nvidia
Copy link

This update does contain a pretty major change to the spring model with TGS. This change is in line with what we now do in PhysX 5 and it is much more correct in terms of behavior of the spring drive compared to analytical expected results.

The TGS spring model in 4.1.1 converged on a result that was too strong for a given spring/damping coefficient. Specifically, it became stronger the more position iterations you gave it, so it would eventually converge on a hard constraint even though the underlying constraint should be soft.

The update resolves this. However, to recover back to something closer to the previous version, you would need to scale the stiffness and damping terms. Back-of-the-envelope calculations suggest the stiffness term would need to be scaled by approximately (posIterations^2) and the damping term by posIterations. As you were running with 32 iterations, the scaling of ~1000x seems about right for the stiffness term, but a scale of about 32-33 on the damping term should also be required.

You should hopefully not need to increase the number of solver iterations, as the underlying solver itself didn't change significantly - only the spring constraints did so adjusting these parameters as described should hopefully get you close to your original behavior.

mottosso pushed a commit to alanjfs/PhysX that referenced this issue Apr 23, 2021
@alanjfs
Copy link
Author

alanjfs commented Apr 23, 2021

Specifically, it became stronger the more position iterations you gave it

It did indeed, I had grown accustomed to this and increased it whenever wanting to increase the effectiveness of drives.

I will experiment with your back-of-the-napkin calculation, that helps a lot. In the ideal case, for backwards compatibility reasons, I would be able to find a formula to port existing physics contraptions to their equivalent values in the new update. And it sounds like I might be able to do just that.

I'll trust that PhysX 5 will maintain this behavior (as I wholeheartedly intend on upgrading as soon at it is made possible) and am happy to consider the issue solved. Thanks again @kstorey-nvidia!

@alanjfs
Copy link
Author

alanjfs commented Apr 24, 2021

I'm back. :)

Unfortunately I'm having severe issues with this update, it does not behave predictably.

4.1.2

  • Angular Stiffness: 1000
  • Angular Damping: 100
PGS Rotate Drive TGS Rotate Drive
bug_with_tgs_4.1.2_pgsRotate.pxd2.zip bug_with_tgs_4.1.2_tgsRotate.pxd2.zip
bug_with_tgs_4 1 2_pgsRotate bug_with_tgs_4 1 2_tgsRotate

Likewise for a linear drive.

  • Linear Stiffness: 1000
  • Linear Damping: 100
PGS Translate Drive TGS Translate Drive
bug_with_tgs_4.1.2_tgsTranslate.pxd2.zip bug_with_tgs_4.1.2_pgsTranslate.pxd2.zip
bug_with_tgs_4 1 2_pgsTranslate bug_with_tgs_4 1 2_tgsTranslate

4.1.1

Whereas in the previous release, at least rotations were on-par with PGS, only the translation were off.

TGS Translate Drive TGS Rotate Drive
bug_with_tgs_4 1 1_tgsTranslate bug_with_tgs_4 1 1_tgsRotate

These all include the typo-fix from above, I haven't tested without it. The exact build is this: https://github.com/alanjfs/PhysX

What can a commoner like myself do? :(

@kstorey-nvidia
Copy link

Hi Alan

I quickly set up some repros and they should work correctly with the very latest code on github.

We had a fix for the kinematic issue rushed through on Friday so if you were to get latest, these should all be working correctly.

The suggested fix I put in this chat was not correct. It was almost correct, but the += should have been -= and vice-versa, but it's probably best to just merge latest to your branch and resolve. The reason for the sign flip is that the multiplication by velMultiplier also flips the signs.

Hopefully this will resolve these issues.

@alanjfs
Copy link
Author

alanjfs commented Apr 26, 2021

Thanks @kstorey-nvidia! Initial tests are looking good, I'll play with this and let you know if anything comes up. Fingers crossed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants