Heightmap collision shape support in Godot Physics

[pouleyKetchoupp]

Functional HeightMapShape3DSW in Godot Physics 3D, based on btHeightfieldTerrainShape from Bullet Physics with a simplified implementation.

Test project:
heightmap-4.0.zip

[pouleyKetchoupp]

CC @Zylann
In case you're interested in giving it a try.
Raycasting uses the Bresenham algorithm from Bullet. For now, I haven't implemented the chunk structure to optimize it on large grids, but it can be added later.

[Zylann]

I am not setup for Godot 4 at all yet so I probably wont test this right now, but I can give it a read.

Note, if you want to check the validity and performance of raycasts, you can try [upgrading to Godot 4 and] using this plugin I made which scans the whole screen with raycasts and overlays the resulting normals: https://github.com/Zylann/godot_collision_scanner

About the acceleration grid structure, I hardcoded the Bullet implementation to use a grid of 16 cells. If the Godot version doesn't do that, it will be slower, but from what I remember it is still somewhat acceptable. However if you do very long raycasts that can traverse the whole terrain, that's where performance will be an issue. And this happens very quickly if the shape is pickable by default, which is why I turn that off in my plugin.

Most of my comments are about optimization. I assume it's good otherwise, the algorithm looks the same. I noticed that another C++ physics engine implemented that approach too with different code, if you want to check it out: DanielChappuis/reactphysics3d@d0fa4c2

[Zylann]

Optimization comment:
Probably worth a different PR/issue, but careful about using the CLAMP macro. It expands the arguments multiple times, which can sometimes be expensive if the compiler cannot reduce the expression. In this present case I assume any decent compiler will figure it out.
I wonder why this is even a macro tbh

[pouleyKetchoupp]

I didn't realize that, it can be dangerous in some cases indeed. For this case it seems like it wouldn't make a huge difference anyway so I can probably leave it this way for readability.

[Zylann]

What if the ray begins outside of the heightmap? If it starts outside but intersects the AABB it should ideally start on the box hit

[pouleyKetchoupp]

That's what this code does, just before the loop:

// Start inside the grid.
int x_start = CLAMP(x, 0, width - 2);
int z_start = CLAMP(z, 0, depth - 2);

// Adjust initial cross values.
cross_x += delta_x * x_step * (x_start - x);
cross_z += delta_z * z_step * (z_start - z);

x = x_start;
z = z_start;

But I agree it might be better to just reduce the ray itself according to the AABB, to replace this and the check to stop when outside the grid within the loop.

[pouleyKetchoupp]

Given that it's already skipping the beginning and end of the ray and the current version seems to work fine, I actually haven't added the extra AABB check to change the ray.

[Zylann]

Optimization comment:
If we clamp the maximum distance before entering the big loop it could allow to remove this check?

[pouleyKetchoupp]

Actually for this one, the check is needed in any case because it could fall outside the grid boundaries and crash otherwise (it's also checked for each quad in Bullet).

[Zylann]

The idea was to clamp the max distance to the exit border so that case would never actually happen. However float imprecision might make this not doable anyways.

[pouleyKetchoupp]

Yeah it would seem a little bit risky not to have a test at all given that it's based on float calculation and might cause crashes in rare cases.

[pouleyKetchoupp]

@Zylann Thanks for your review! I'm going to check your comments in details, but just so you know there's also a 3.x backport of this PR with the exact same implementation: #47349.

[Zylann]

I tested your PR with my heightmap plugin.

Upon opening my project (which has no default scene so it opens on a blank scene), I get this error:

ERROR: PhysicsServerSW::shape_get_data: Condition "!shape->is_configured()" is true. Returned: Variant()
   At: servers\physics\physics_server_sw.cpp:127

Followed later by this error:

ERROR: RID_Owner<class BodySW>::get: Condition "!p_rid.is_valid()" is true. Returned: 0
   At: D:\PROJETS\INFO\GODOT\Engine\nekomatata_fork\core/rid.h:149
ERROR: PhysicsServerSW::body_attach_object_instance_id: Condition "!body" is true.
   At: servers\physics\physics_server_sw.cpp:673
ERROR: Invalid ID.
   At: servers\physics\physics_server_sw.cpp:1402

I don't know what these are or if they are even related to your PR, but they happen before opening any scene.

I then tested a few things:

• Throwing rigidbody cubes around

  • Debug build:
    Worked. Can't tell about performance, I don't have a benchmark scene setup for this

  • ReleaseDebug build:
    Also works

• Raycast scanning:

  • Debug build:
    It works, however it is A LOT slower than Bullet, maybe 4 times slower. I'll assume the debug build makes things a bit meaningless, but as stated in the review the GodotPhysics shape does not use any accelerator.
    It appears to match the visuals but it's too slow to wait for the full detailed pass.

  • ReleaseDebug build:
    Also works, faster as well.
    The x4 performance ratio can still be observed though. This is how long it takes to fill the viewport:

    • GodotPhysics: 4min53s
    • Bullet: 58s

  Comparing two screenshots, the result is identical:

GodotPhysics:

Bullet:

[pouleyKetchoupp]

@Zylann I've just pushed changes based on your comments:

• _face_cull_segment, _cell_cull_segment, _get_point and _get_height are now inlined
• Early exit on hit
• Forcing Vector<float> instead of using real_t. I've also changed the scene node code and the Bullet server code the same way, but I haven't ported this change to the 3.x version. There's no way to distinguish an array of float from double in a variant on 3.x, so it's safer to stick to real_t.

Thanks for running the performance tests! Given the x5 performance difference in release_debug, it's definitely worth adding the raycast acceleration based on chunks. I'll add it in a different PR later.

[kalysti]

Btw its working well. Can u bring it into master. Because i need a heightmap collider for my terrain tool ;-)

[pouleyKetchoupp]

@sboron Thanks for testing! This PR still needs a review and approval from a maintainer to be merged. In the meantime you can cherry-pick the commit to your custom branch.

[akien-mga]

Thanks!

[Zylann]

I just remembered, we may consider supporting holes using nans/infs at some point, which would make it way easier to have openings in the map Zylann/godot_heightmap_plugin#125

[pouleyKetchoupp]

@Zylann That sounds useful! Feel free to open a proposal if you want, otherwise I'll just add it to my todo list and write one when I find some time.