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

Collision: more accurate computation with acceleration and long dtime #15408

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kno10
Copy link
Contributor

@kno10 kno10 commented Nov 9, 2024

Refreshed version of #15029 (reverted for 5.10.0 by #15400, includes #15378)

This fixes inaccuracies in the old collision code which in essence assumed a linear movement, and in case of a collision still used the final speed.

This version:

  • uses a parabolic computation with acceleration (postponed for now, did not get it to work perfectly)
  • more accurately computes the time of collision
  • sorts node boxes to (hopefully) find collisions faster (dropped, did not improve enough)
  • adds several additional unit tests (not all are perfectly passed by the previous nor the current code yet)

It is still an approximation to not need to iterate too much - consider a ball bouncing on a surface, and we have a long time step: the ball might do multiple bounces; this code will still only allow one per timestep: to avoid issues when standing on a floor, any collision will zero the axis velocity (unless bouncy) and zero the acceleration (always) for now.
The ultimate solution would be to make everything so fast that we can do much smaller dtime steps and all of this does not matter anymore.

The following cases should be tested:

@kno10 kno10 changed the title Collision fixes [no squash] Collision fixes Nov 9, 2024
@kno10 kno10 force-pushed the collision-fixes branch 4 times, most recently from 3ea6af2 to 217fd70 Compare November 9, 2024 22:19
@kno10 kno10 changed the title [no squash] Collision fixes Fix collisions with long dtime, in particular with bouncing [nosq] Nov 10, 2024
@sfan5 sfan5 added @ Server / Client / Env. Bugfix 🐛 PRs that fix a bug Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Nov 10, 2024
@sfan5 sfan5 changed the title Fix collisions with long dtime, in particular with bouncing [nosq] [no squash] Fix collisions with long dtime, in particular with bouncing Nov 12, 2024
@sfan5 sfan5 added Rebase needed The PR needs to be rebased by its author and removed Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Nov 12, 2024
@sfan5
Copy link
Collaborator

sfan5 commented Nov 12, 2024

You can rebase now.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author label Nov 12, 2024
@kno10 kno10 changed the title [no squash] Fix collisions with long dtime, in particular with bouncing WIP: Fix collisions with long dtime, in particular with bouncing Nov 13, 2024
@kno10
Copy link
Contributor Author

kno10 commented Nov 13, 2024

@appgurueu: working on taking acceleration into account directly.

@kno10 kno10 force-pushed the collision-fixes branch 2 times, most recently from 00989ce to 890e993 Compare November 14, 2024 21:30
@kno10 kno10 changed the title WIP: Fix collisions with long dtime, in particular with bouncing Collision rewrite: more accurate with acceleration and long dtime Nov 14, 2024
@kno10
Copy link
Contributor Author

kno10 commented Nov 14, 2024

@sfan5 @appgurueu this version now is probably good for reviewing and testing.

Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not doing a thorough review, but I looked into the MinGW CI failure. Please include the <algorithm> header since you used std::sort; it's transitively depending on an include all the way down in an irrlicht/ header, and that also makes the compiler errors much more confusing.

src/collision.cpp Outdated Show resolved Hide resolved
@SmallJoker
Copy link
Member

Did a few rudimentary performance comparisons. In a new devtest world.

Walking on floor, in circles. Full range view enabled.

plateau

  • master: 10.5 us
  • PR: 13.0 us

Holding the forward button in this scene:

benchy

  • master: 18 us
  • PR: 24 us

This is actually pretty good considering the introduced sqrt calculations.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review iteration. Works well from what I can see so far.

Tested:

  • singleplayer: walking
  • local server with dedicated_server_step = 0.6: falling nodes

src/collision.cpp Outdated Show resolved Hide resolved
src/collision.cpp Outdated Show resolved Hide resolved
src/collision.cpp Outdated Show resolved Hide resolved
src/collision.cpp Outdated Show resolved Hide resolved
@kno10 kno10 force-pushed the collision-fixes branch 2 times, most recently from 66c8332 to f4ace0f Compare November 17, 2024 23:22
@SmallJoker SmallJoker self-requested a review November 21, 2024 18:38
src/collision.cpp Show resolved Hide resolved
src/collision.cpp Outdated Show resolved Hide resolved
src/collision.cpp Outdated Show resolved Hide resolved
src/collision.cpp Show resolved Hide resolved
src/collision.cpp Outdated Show resolved Hide resolved
@kno10 kno10 force-pushed the collision-fixes branch 2 times, most recently from e0c226b to eb6b3ec Compare December 1, 2024 02:00
@SmallJoker SmallJoker self-requested a review December 6, 2024 17:53
@kno10 kno10 force-pushed the collision-fixes branch 2 times, most recently from aeb560d to 1442a18 Compare December 8, 2024 14:51
@kno10
Copy link
Contributor Author

kno10 commented Dec 8, 2024

Giving up on the parabolic movement for now. I could not get rid of all the glitches.
If you walk diagonally against a wall, you would frequently get a collision with the next node, too.
Hence I reset this pull to the basic version for now. For this one I could not notice obvious glitches; it is closer to the current version, but improves the accuracy of the collision time computation.
We can then try to tackle the collision detection with acceleration (parabolic, not linear movement) in a subsequent step/pull potentially in a future version.

@kno10 kno10 changed the title Collision rewrite: more accurate with acceleration and long dtime Collision: more accurate computation with acceleration and long dtime Dec 9, 2024
@kno10
Copy link
Contributor Author

kno10 commented Dec 27, 2024

rebased, no changes. Ready for review.

@SmallJoker
Copy link
Member

SmallJoker commented Dec 30, 2024

Slippery test

Tested with this script: #15375 (comment) (N° 173 for my reference)

  • PR sand: 11.7 tiles
  • PR grass: 15 tiles
  • master sand: 11.6 tiles
  • master grass: 14.9 tiles

☑️ This is good enough.

Bouncy test

Physical entitiy code:

diff --git a/games/devtest/mods/testentities/visuals.lua b/games/devtest/mods/testentities/visuals.lua
index 6bb1b8282..7517f7a3e 100644
--- a/games/devtest/mods/testentities/visuals.lua
+++ b/games/devtest/mods/testentities/visuals.lua
@@ -52,7 +52,11 @@ core.register_entity("testentities:mesh", {
                textures = {
                        "testnodes_mesh_stripes2.png"
                },
+               physical = true
        },
+       on_activate = function(self)
+               self.object:set_acceleration({ x = 0, y = -9.81, z = 0 })
+       end,
 })
 
 core.register_entity("testentities:mesh_unshaded", {

Setup: sand pillar + testnodes:bouncy100
setup

  1. Local server
  2. Spawned with the Entity Spawner tool -> testentities:mesh.
  3. Executed the command while in downwards movement: //lua local t0 = os.time() while os.time() < t0+2 do end (from Bounciness issues with large dtime steps #15027 (comment))
  4. The entity shoots off into the sky, hence does not fix Bounciness issues with large dtime steps #15027.

@kno10
Copy link
Contributor Author

kno10 commented Dec 31, 2024

That last test goes beyond the scope of this pull request, as it requires computing the parabolic movement.
As this did not yet work sufficiently well, and there was little interest, I have currently given up on this.
The case it tries to fix is catapulting entities even with next to no fall velocity into the sky nevertheless.
Think of the villagers in Mineclonia etc. standing on beds and suddenly bouncing through (!) the roof with some dtime lag (e.g., because the village emerge code is still working, or because all entities want to pathfind now).
Try the same without the sand pillar on master and on this branch.

@Zughy Zughy added this to the 5.12.0 milestone Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants