Auto-step climbing high tri meshes

[msinilo]

We seem to have some problems with autostep functionality sometimes climbing over obstacles that should be too tall for it to work (triangle meshes). It seems like it might be caused by precision issues. What happens is more or less:

• during the autostep phase we get a "side collision". Triangle we hit has a normal that's "almost horizontal" (normaly pointing slightly up, but barely - 1e-8 or so.. in local space). However, after world space translation (no rotation here), we get a normal that's now pointing slightly down (again, tiny number, -1e-7/-1e-8). That means testSlope(mContactNormalSidePass, upDirection, mUserParams.mSlopeLimit)) (in the "down pass" of moveCharacter will fail, so we're not setting STF_HIT_NON_WALKABLE and retrying without an autostep, despite contact point height being > stepOffset.
• the down pass hits the "top" triangle, but normal points up, so testSlope(Normal, upDirection, mUserParams.mSlopeLimit)) passes and we assume we can climb
  Our autostep is 0.45, capsule radius is 0.45 and obstacle height is around 0.8. Should we be able to climb in such case?
  One hack that seems to help (but has not been tested very extensively elsewhere) is to "clamp" mContactNormalSidePass.y if it's negative, but close to zero, ie.

if(mContactNormalSidePass.y < 0.f && mContactNormalSidePass.y > -1e-4f)
{
    mContactNormalSidePass.y = 0.0f;
}

Maybe the testSlope check for the mContactNormalSidePass is a bit too strict? The idea seems to be reject ceiling faces, but really anything that is not perfectly horizontal might be rejected her.

[PierreTerdiman]

Sorry for the delay, I missed this one. I'll investigate ASAP.

[msinilo]

vectest.zip
Thanks. Attached is a simple Python file I used to double check it (had to pack it or Github wouldn't let me attach it). Even fairly modest translation (7.5m) is enough to make the normal go from "pointing slightly upwards" to "pointing slightly downwards". Python's version uses doubles so it's actually 'fine', but if I plug in values taken straight from C++ it behaves differently (test slope return true in the first 2 cases, false in the third one)

[PierreTerdiman]

First I had to do some Perforce archeology to remember this code.

The mContactNormalSidePass stuff was added back in 2012, and then reused later (since it was already there) to deal with "ceiling faces", but that was not the original goal. Initially it was added because capsules could be stopped by the "constrained mode" after landing on tilted triangles that should be considered walk-able if you looked at their slope. But the capsules would still be blocked on them because large timesteps produced motions that would be seen as going beyond the step offset - which in these cases would actually be legal. Anyway short story is that it's not directly related to ceiling faces.

Now that I remember what this was for, I'll have a look at your case/files.

[PierreTerdiman]

I think the mistake was to reuse the "testSlope" function (which was written for ground normals initially) for normals generated in the "side pass". With a side pass getting an "horizontal" normal is quite likely, while the testSlope implementation looks like it expected these to be at the edge of legality. I see why it happened, I see why it kind of makes sense for the cases that "mContactNormalSidePass" was added for.

Now the best fix for this is not so clear. There is a lot of history in that code and it's already too complex for its own good (it could use a good rewrite at this point).

Your suggested fix looks "fine" but it introduces yet another epsilon that is just going to break for "horizontal normals" that happen to go just a tiny bit beyond that value.

Need to think about this.

[PierreTerdiman]

I think I will have to think more about this (there is something bothering me in the back of my head that I cannot immediately pinpoint). Meanwhile, I would suggest the following change:

PX_FORCE_INLINE	bool testSideNormal(const PxVec3& normal, const PxVec3& upDirection, PxF32 slopeLimit)
{
	const float dp = normal.dot(upDirection);
	return dp<slopeLimit;
}

In that line:

PhysX/physx/source/physxcharacterkinematic/src/CctCharacterController.cpp

Line 1873 in c3d5537

if((mFlags & STF_VALIDATE_TRIANGLE_SIDE) && testSlope(mContactNormalSidePass, upDirection, mUserParams.mSlopeLimit))

Use the new function above instead of the old testSlope:

if((mFlags & STF_VALIDATE_TRIANGLE_SIDE) && testSideNormal(mContactNormalSidePass, upDirection, mUserParams.mSlopeLimit))

[msinilo]

Thanks for investigating! Yeah my "fix" was more of a hack to see if it'd fix 1 case we were seeing, I'd never ship with that. Your proposed change seems to work fine in that situation. It is obviously a very fragile code, so we'll run with it for a bit and see if it's fine everywhere, but so far so good.

[msinilo]

Just a little update, we've been running with this proposed fix for a few weeks now, it seems to be working fine and didn't introduce any other problems, so closing the issue.