CollisionHandlerFloor fix

[Moguri]

CollisionHandlerFloor was not working as expected, so I have added a fix. I believe the previous code was mixing coordinate systems and using a local offset as a global position.

[codecov]

Codecov Report

Merging #957 into master will increase coverage by 0.08%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
+ Coverage   14.99%   15.08%   +0.08%     
==========================================
  Files        3701     3701              
  Lines      355662   355701      +39     
==========================================
+ Hits        53339    53647     +308     
+ Misses     302323   302054     -269     

Impacted Files	Coverage Δ
panda/src/collide/collisionHandlerFloor.cxx	75.28% <55.55%> (+75.28%)	⬆️
panda/src/device/inputDevice.I	4.13% <0.00%> (-2.48%)	⬇️
panda/src/pgraph/cullFaceAttrib.cxx	17.34% <0.00%> (-1.03%)	⬇️
panda/src/nativenet/buffered_datagramconnection.h	6.74% <0.00%> (-0.41%)	⬇️
direct/src/showbase/ProfileSession.py	19.91% <0.00%> (-0.33%)	⬇️
direct/src/dist/commands.py	7.14% <0.00%> (-0.11%)	⬇️
panda/src/pgraph/geomNode.cxx	21.17% <0.00%> (-0.04%)	⬇️
direct/src/dcparser/dcClass.h	0.00% <0.00%> (ø)
direct/src/dcparser/dcClass.cxx	0.00% <0.00%> (ø)
dtool/src/cppparser/cppExpression.h	0.00% <0.00%> (ø)
... and 24 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f1289b...40d54db. Read the comment docs.

[rdb]

For my own benefit, I tried to extract the bits from this that have actually changed from the bits that are cleaned up, and if I'm not mistaken, this change primarily replaces this code:

CPT(TransformState) trans = def._target.get_transform();
LVecBase3 pos = trans->get_pos();
pos[2] += adjust;
def._target.set_transform(trans->set_pos(pos));
def.updated_transform();

apply_linear_force(def, LVector3(0.0f, 0.0f, adjust));

With this:

LVecBase3 adjustvec = LVecBase3(0, 0, adjust);
def._target.set_pos(def._target, adjustvec);
def.updated_transform();

So two questions:

• Why have you taken out the call to apply_linear_force?
• Should the floor handler really act relative to the character's coordinate system? If I'm not mistaken, wouldn't this cause the character to fall sideways if it is leaning over?

[Moguri]

I believe I had convinced myself that apply_linear_force() was either unnecessary or redundant. However, I will need to dig through the code again to figure out why.

I will add tests to check for falling sideways.

[rdb]

It would help to understand the original bug (perhaps with a repro case) since—following the code in my mind—the original behaviour seems more correct than the fix.

[Moguri]

I'd also like to point out that the cleanup and actual changes are in two separate commits. I removed the dead code since it made things easier for me to figure out what as happening, but I could drop the cleanup commit from this PR if that would make things easier.

[rdb]

Ah, that's great—I had unfortunately simply not noticed that you had split them up.

[rdb]

Is there an issue for the bug report or a repro case so that I can validate the solution against the problem?

[rdb]

I tested this with your updated Roaming Ralph sample and it does look like it may be the right fix for how CollisionHandlerFloor was written, namely with the assumption that the character rotation determines which way it falls. It does result in Ralph sliding sideways if he's tilted.

That might be a feature, in the sense that Ralph will always stick to the terrain if you also adjust him to position himself along the surface normal, which might be good for CollisionHandlerFloor as opposed to CollisionHandlerGravity (which you want to switch to anyway as soon as you have cliffs you can fall off of). If you don't want this behaviour, you could always keep the target node upright and have the tilted character model parented to it.

Alternative implementations:

• set_highest_collision could be given the parent of the character node to take the coordinates relative to instead. This means that a tilt of the character won't be an issue, but a tilt of the character's parent will still cause "sideways falling".
• It could always use the root of the scene graph for determining the height.

I'm leaning towards accepting this as-is until someone complains.

[Moguri]

Should we merge this as-is? If not, then I will probably abandon it for the time being.

I don't know how much interest there is in using CollisionHandlerFloor in new code, especially to risk breaking old code that may be relying on the odd behavior.