Fix: Friction disappears when manually applying gravity #224

[chnicoloso]

Addresses:
schteppe#224

Reuses pattern from p2.js:
https://github.com/schteppe/p2.js/blob/master/src/world/World.js#L88-L92

Before:

After:

[chnicoloso]

cc @marcofugaro @codynova (you surface as recent contributors, hoping I can get a review from one of you, thank you!)

[isaac-mason]

Thanks for this! 🙂

One comment from me:

In this PR, Narrowphase has been changed to always use world.frictionGravity.

If world.gravity is changed after construction, world.frictionGravity will remain the same. As-is, users would need to update both, which is a backwards incompatible change, and maybe also slightly surprising behavior.

Maybe we could add properties like the world.useWorldGravityAsFrictionGravity and world.useFrictionGravityOnZeroGravity flags in p2 to address this?

Let me know what you think!

[chnicoloso]

@isaac-mason thanks for having a look!

Ah yes, thank you! Great catch. The flags are a good idea, though I would prefer an approach that kept the config matrix as small and simple as possible. How about this:

• By default, world.frictionGravity is undefined.
• Narrowphase applies friction using world.frictionGravity || world.gravity.

This way, current behavior is maintained and backwards compatibility is ensured while still allowing users to override friction gravity, if they need to. When the average user changed world.gravity, friction would change with it, no surprises - only users who have explicitly set world.frictionGravity would be expected to manually manage that relationship between world.gravity and world.frictionGravity.

What do you think?

[chnicoloso]

I updated with the suggestion above. Also, I noticed that dist/ is not in the project's .gitignore. Am I supposed to be building and committing the built package or is there a build/deploy pipeline that takes care of that?

[isaac-mason]

@chnicoloso that sounds reasonable to me! I'll give this another read-through shortly 🙂

[isaac-mason]

I believe right now builds are committed to dist on doing a release. There's no pipeline that handles that afaik. Would want someone else to confirm that though!

[isaac-mason]

Implementation LGTM! readme.md just needs updating to reflect world.frictionGravity defaulting to undefined.

[isaac-mason]

Could anyone else also give this a review? @marcofugaro / @codynova

[chnicoloso]

Thanks @isaac-mason! I updated the readme - will hold on building and committing dist until we get confirmation that this is indeed what we want.

[drcmda]

i can merge and publish if you guys want? i don't understand the code but from looking at the images it behaves correct.

[chnicoloso]

Thanks @drcmda! That would be great - so you don't need me to build and commit dist?

[marcofugaro]

Makes sense, thanks!

[marcofugaro]

Yeah, an example would be useful, but maybe in a future PR if someone wants