Skip to content

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

Merged
sfan5 merged 6 commits into
luanti-org:masterfrom
kno10:collision-fixes
Feb 15, 2025
Merged

Collision: more accurate computation with acceleration and long dtime#15408
sfan5 merged 6 commits into
luanti-org:masterfrom
kno10:collision-fixes

Conversation

@kno10
Copy link
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Contributor Author

kno10 commented Nov 14, 2024

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

Copy link
Copy Markdown
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.

Comment thread src/collision.cpp Outdated
@SmallJoker
Copy link
Copy Markdown
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.

@SmallJoker SmallJoker self-requested a review November 21, 2024 18:38
Comment thread src/collision.cpp
Comment thread src/collision.cpp Outdated
Comment thread src/collision.cpp Outdated
Comment thread src/collision.cpp
Comment thread src/collision.cpp Outdated
@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
Copy Markdown
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
Copy Markdown
Contributor Author

kno10 commented Dec 27, 2024

rebased, no changes. Ready for review.

@SmallJoker
Copy link
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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.

Whereas it does not fix the bouncy issue (see comment above), it is a good cleanup and likely reduces speed calculation errors (aspeed). There's no noticeable differences between master and PR, which is nice to see.

I also noticed that the bouncy nodes might deal damage too quickly (already present in master), due to abrupt speed changes. In the meantime, fall_damage_add_percent = -50 or so might be needed to achieve an enjoyable trampoline.

Comment thread src/unittest/test_collision.cpp Outdated
Copy link
Copy Markdown
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

looked at the test code mainly, should be fine.

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