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

WIP: Make collision generation dynamic #278

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

lw64
Copy link
Contributor

@lw64 lw64 commented Dec 17, 2023

Admin edit:


Fixes #161

@TokisanGames TokisanGames mentioned this pull request Dec 19, 2023
src/terrain_3d.cpp Outdated Show resolved Hide resolved
src/terrain_3d.cpp Outdated Show resolved Hide resolved
src/terrain_3d.cpp Outdated Show resolved Hide resolved
src/terrain_3d.cpp Outdated Show resolved Hide resolved
src/terrain_3d.cpp Outdated Show resolved Hide resolved
src/terrain_3d.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@TokisanGames TokisanGames left a comment

Choose a reason for hiding this comment

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

Alright, good job. I think once the memory management is moved to build collision and destroy collision it will be on par or better than a single 65x65 collision shape. Worse because it has to manage 50-60 shapes, but better because it only needs to update a few smaller shapes instead of all 65x65.

We're pretty close. Make the modifications, rebase and it should be finished or very close.
Thanks!

src/terrain_3d.cpp Outdated Show resolved Hide resolved
src/terrain_3d.cpp Outdated Show resolved Hide resolved
@lw64
Copy link
Contributor Author

lw64 commented Jan 3, 2024

One thing I observed is that when you change the collision_mode from full to dynamic, the collision shapes aren't freed. But I guess its not a problem since this isn't changed at runtime. And in the editor you can just reopen the scene

@TokisanGames
Copy link
Owner

I couldn't move the memory allocation fully to _build_collision, as it is not defined at build time, how many collision shapes need to be allocated.

  • Array.resize() will resize arrays dynamically.
    shapes_per_region is calculated in update_collision, but doesn't need to be, it can be calculated in build_collision.

  • In game modes (physics server) are broken:

ERROR: Index p_index = 0 is out of bounds (shapes.size() = 0).
   at: set_shape (servers/physics_3d/godot_collision_object_3d.cpp:52)
ERROR: Index p_index = 0 is out of bounds (shapes.size() = 0).
   at: set_shape_transform (servers/physics_3d/godot_collision_object_3d.cpp:63)
  • Three related issues don't rebuild collision properly. Upon set_collision_mode, set shape size, & distance it should destroy and build collision to free allocations and set everything up properly, just like set_collision_enabled does.
    • Changing from Full editor to Dynamic editor all collision meshes are huge and don't get resized down.

    • Changing shape size (on dynamic in editor mode) crashes godot.

    • Changing from an editor mode to a game mode does not remove the shapes, but should.

I didn't evaluate all of the new code since there are things that aren't working. Let me know if you need help with anything or want to work on things together.

I added a PR that renames and organizes some things.

src/terrain_3d.cpp Outdated Show resolved Hide resolved
@TokisanGames
Copy link
Owner

TokisanGames commented Jan 17, 2024

In dynamic editor mode, collision shapes do not have unique positions and are doubling up

image


Though this solution is an interesting method, I feel like this is an overly complicated solution.

What I proposed was using a small 65x65 collision square around the player, then on every snap, update the vertices of the shape, so 4225 vertex updates every snap, (which is based on movement, not frames). That is how my gterrain code worked, and is very short and simple. It will work fine with the static body and physics server, and does not continually allocate and deallocate memory.

This solution has improves upon it by breaking down the collision shape into 17x17 vertex chunks, and only new chunks are updated. So when moving laterally, say 8 chunks are updated, or 2312 vertices updated.

The cost of it though is a rather complicated chunking system that needs to track 50-64 collision shapes and move them around accurately and uniquely, without making memory non-contiguous by constant allocations and deallocations. This hasn't been achieved quite yet, so additional cost is that we need to spend more time to preallocate all of the shapes in build_collision, and identify unique positions so they don't doubling up.

Then we have to maintain this code over time.

And the physics server needs to calculate based on 64 shapes instead of 1.

Though I started working on this PR to clean up and fix things, once I discovered the doubling up, I think the scales tipped for me in the cost/benefit analysis.

I'm not sure the added overhead of us and the physics server managing all of the extra shapes, and the effort we've put in and still need to put in is worth the savings of ~2000 vertex assignments per snap. Whether it's 2000 or 4000 adjustments, both are inconsequential.

I think we should revert most of these changes and go back to the simpler solution proposed in #161 and demonstrated in my gterrain code. Though the editor collision modes and settable collision shape size are good.

What do you think?

@lw64
Copy link
Contributor Author

lw64 commented Jan 17, 2024

So I addressed your issue with the doubled collision shapes and its fixed now. Also I finished the non-editor collision, which was previously not really done yet. The issues I had before with missing API was resolved by finding the correct API xD I tested this a bit and for me it doesn't leak any memory and there shouldn't be any allocations as far as I can see. So I think its good to go now.

@TokisanGames
Copy link
Owner

Alright, I tested more and now it's all working well. Good job. There are a few more improvements that I made and some more I've left for you.

Items I adjusted in my commits:

  • If Dynamic shape size is odd, the vertices don't line up with the terrain. If the size is not a power of 2, there are gaps at the region boundaries. If the size is too small relative to distance, say 2 and 64, 8 and 256 the whole engine slows down (would be fixed by redesign at bottom). The numbers can get negative or very large. I enforced sane values so they won't break

  • Renamed variables and added comments

  • Replaced append w/ push_back. They are aliases but the latter is used everywhere else

  • Replaced pop_at(i) on negative forloops with pop_back, which is faster. The former recreates the array.

  • Stripped camera detection out of update_collision

  • Enforced collision setup in build_collision and removed from update_collision

  • Changed update calculation time to usec and print it on DEBUG_CONT error level

There are two other issues I found and have not fixed.


Invalid RID

I get this error message multiple times on DYNAMIC_GAME, after running the game, moving the camera, then quitting.

ERROR: Invalid ID.
   at: (servers/physics_3d/godot_physics_server_3d.cpp:1619)

or using Jolt:

ERROR: Failed to free RID: The specified RID has no owner.
   at: (src\servers\jolt_physics_server_3d.cpp:1782)

This is caused by this code in destroy_collision,

		for (int i = PhysicsServer3D::get_singleton()->body_get_shape_count(_static_body) - 1; i >= 0; i--) {
			RID shape = PhysicsServer3D::get_singleton()->body_get_shape(_static_body, i);
			PhysicsServer3D::get_singleton()->free_rid(shape);
		}

This means we can't rely on the server to house the RIDs but must track them ourselves, as we do with _static_body and other RID variables. This is typical and expected. The solution is addressed by the redesign below.


Inefficient Loops

At even the default values for dynamic size/distance with 50-64 shapes in existing, using existing.has() to check if the shape is there within 3 for loops means checking:

  • 3 vector integers for each of 64 shapes
  • shapes per region squared - default 64
  • regions - up to 256

So up to 3 * 64 * 64 * 256 = 3.1 million integer comparisons just to tell if the collision shape already exists. Both the checking if it exists and the overall for loop need to be better designed. We don't need to check for all regions, for all shapes per region x & z, and we don't need to iterate through the array using has.

What should happen in a chunking system is:

_build_collision

  • number of shapes needed identified
  • all shapes are created, memory allocated
  • all shapes tracked in three arrays:
    • All shapes - fixed size identified from above
    • Used shapes - fixed size - identified by calculation of distance_size as a grid around camera, some indices are always empty
    • Unused shapes - dynamic size used with push_back / pop_back

_update_collision

  • We assume there is a fixed grid where shape locations can be (like an integer grid, or every 4 units, etc)
  • Rather than evaluating all possible points on the grid for all regions, we just take a small area around the camera to evaluate
  • Rather than checking the entire existing array each time, we just translate the grid location to an array index. Then we go directly to that array index and if something is already there, we skip it. This is like how the region offsets work, see storage.get_region_offset() and get_region_index(). They don't iterate over all regions, they just translate position into an array index. Our _region_map is a fixed size 16x16 array with many empty slots. This isn't the exact code we need, but we need to use the same concept.
  • Anything unused is disabled and stored in the unused array
  • The only thing that happens in this function is moving shapes between arrays, moving shape locations, and setting vertices. All in a small area around the provided update location. No allocations.

_destroy_collision

  • All shapes in our arrays are destroyed without asking the physics server or the tree (children) for them. Fixes the invalid RID problem since we track and free all of our own RIDs, which is our responsibility.

This logic is the same for physics server shapes or instanced shapes. There's very little code difference between the two. The main difference is that we get the RID of the shape and send it to the server, or how shapes are created and destroyed.

This should reduce the update time a bit, but even if not, it's a cleaner design that will help us maintain the code in the future. It won't take much modification from what's already there. But those crucial for loops need to be reworked. I can help you implement this if you like.

src/terrain_3d.cpp Outdated Show resolved Hide resolved
@TokisanGames
Copy link
Owner

I'm going to look at the design a bit more later, but here's some initial testing of functionality.

  • It was working very well before, just not the ideal design. Now as it's being redesigned, the issues are normal. Just make sure to not squash your PR yet so we can refer back to commit d720d55 for reference.

  • I recently realized the use of memfree() in _destroy_collision() is wrong. It should be memdelete() with memnew. memfree goes with memalloc. Changing these dramatically reduces the number of RID Allocations leaked at exit messages when godot closes, which it should. We weren't removing any of them.

  • In Dynamic/editor mode, it seems to be generating only 1/4th of the circle so it doesn't work in game since it's off center.
    image

  • Full / editor mode only generates for the first region.

  • On a couple of editor startups I got this error message. Other times it didn't happen. I didn't troubleshoot, so look out for it.

ERROR: Terrain3D::_update_collision: Collision not initialized, returning
   at: push_error (core/variant/variant_utility.cpp:1091)

@TokisanGames
Copy link
Owner

Note, I fixed the memory leaks for the up coming release.

c5115fe

@Saul2022
Copy link

Saul2022 commented Feb 19, 2024

An error i found checking this pr is that when trying to make play the demo, it doesn´t start and leaves the long messages, confirming cory issue.

that update coll is not initialized
It happen´s in both 4.2 and a 4.3 build.

either way , it improves loading times( like it the main reason why a game with terrain 3d takes a lot to load.

core\variant\variant_utility.cpp:1091 - Terrain3D::_update_collision: Collision not initialized, returning

editor_screenshot_2024-02-19T112621

@TokisanGames
Copy link
Owner

My current thinking for the foliage instancer is to generate MultiMeshes in chunks. I will build it off of the chunk infrastructure you're building here. I'll be ready to work on foliage in a week or two, so if this isn't done by then I'll get involved to move it along.

@TokisanGames
Copy link
Owner

This PR needs address #341 (comment), and make sure it works without error when running and quitting without regions.

@TokisanGames TokisanGames marked this pull request as draft April 20, 2024 07:23
@TokisanGames TokisanGames changed the title Make collision generation dynamic WIP: Make collision generation dynamic Jun 1, 2024
@TokisanGames
Copy link
Owner

Rebased and consolidated down to three commits. 1st is the previously identified working commit. It works fine but has some errors on cleanup. 2nd is a first pass rewrite that also seems to work, with errors out of bounds. 3rd is a separation into separate classes, which is currently broken due to changes in the main tree not considered in this branch. Unknown if it works.

Next step is to audit the new design, consolidate the new classes down to 1 or 2, and review and adjust the code.

@lw64
Copy link
Contributor Author

lw64 commented Dec 31, 2024

@TokisanGames yes, thanks for the rebase! Please give some feedback on the system design and APIs

@TokisanGames
Copy link
Owner

@lw64 I'll take over this PR and work on it for my next priority. As you requested, I'll give you feedback along the way.

Some notes I have already:

  • The number of classes and level of abstraction is good for a project that's going to scale very big, or an academic project. For our needs, pairing that down to 1 or 2 classes will do. The instancer handles its own chunk system all in one function.

  • Like Godot code, all the headers will have the same copyright. You are included in Contributors and noted on the front page of the repo and in Authors.

Next I need to understand what changes are made in commits 2 and 3, and what is the best way to structure it.

src/terrain_3d.h Outdated Show resolved Hide resolved
p_size++;
}
p_size = CLAMP(p_size, 8, 256);
_chunk_size = p_size;
Copy link
Owner

Choose a reason for hiding this comment

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

This is duplicate code and if it's not already available in the engine, should be put in Terrain3DUtil

@TokisanGames
Copy link
Owner

We have 5 classes: 3 manager types, 2 chunk types.

Each derivative class should represent a unique object we will create. So if we were going have chunks for say instances, instance collision, ground collision that might warrant 3 child classes and 1 base class for chunks.

However, one manager could process all of those classes. It would be designed to work with the base chunk so it could handle all child types. I don't see the reasoning for having 3 manager types, and certainly not for having more manager types than managee types (chunks).

The design pattern that would be a better fit for this might be a mixture of the composite and template patterns. Something like this, which is exactly how our ProximityManager is constructed. In this case the manager might move the baseobject, then pass the transform or other data to the object run process which could handle generating ground collision, or generating an MMI, or generating collision for objects in the area.

class Manager {
    std::vector<BaseObject*> objects;

public:
    void registerObject(BaseObject* object) {
        objects.push_back(object);
    }

    void processAll() {
        for (auto o : objects) {
            o->process();
        }
    }
};

class BaseObject {
public:
    virtual void process() = 0;
    virtual ~BaseObject() = default;  // Virtual destructor for proper cleanup
};

class Box : public BaseObject {
public:
    void process() override {
        std::cout << "I'm a box." << std::endl;
    }
};

class Sphere : public BaseObject {
public:
    void process() override {
        std::cout << "I'm a sphere." << std::endl;
    }
};

@lw64
Copy link
Contributor Author

lw64 commented Jan 9, 2025

However, one manager could process all of those classes. It would be designed to work with the base chunk so it could handle all child types. I don't see the reasoning for having 3 manager types, and certainly not for having more manager types than managee types (chunks).

I went for the most simple approach for the start, which is each type of chunk has its own very simple manager, so simple even that they could be in the same file, and the main logic is in the base manager class.

But I can see a lot of potential for optimization there, and having only one manager instance handling everything can enable quite a lot. Also something to keep in mind is doing more complex chunking for example with quadtrees later.

But I think the most complicated part about having one single manager instance is that each chunk type might have different properties, gras chunks are smaller and have shorter visibility, while tree chunks should be bigger and have very far visibility. I think this is possible though, and could avoid a lot of overhead, since not every manager instance does the same work again and again.

@lw64
Copy link
Contributor Author

lw64 commented Jan 9, 2025

@TokisanGames and thanks for taking over! :D

@TokisanGames
Copy link
Owner

The overall hierarchical structure of the later commits isn't bad. I just think it is not a good match for our use case.

base_chunk would be more useful if we were to have multiple class types derive from it. mesh_chunk, collision_shape_chunk, instancer_chunk, etc, each with their own unique functionality, but also enough shared code like positioning, signal responding, creation, deletion (a lot that is already handled by Node3D).

chunk_manager would be useful if we were to have multiple types of managers with their own unique code, but also a lot of shared code.

I think these many classes might be used more if we were making a chunked terrain. In our case a lot of work is already done by engine classes or the clipmap nature.

In a chunked terrain, a chunk might contain a group of nodes with several mesh instance LODs, a physics body, and a collision shape. It may also have a subscene with the assets at that location. It's a lot of unique functionality not already present in the engine.

What functionality do we need in our chunks?

In the instancer, we store the transforms in a data structure, then we create MMIs and place them on the terrain in a grid. The MMIs are the objects we instance that perform a special function (rendering), hence they are the chunk. We don't need another any other functionality in the chunk beyond what the MMI and it's inherited Node3D and Object classes do, so we don't need another class to contain it.

For collision, the object we want to instance and move around is a collision shape. The only functionality we need is to store the data and move it. You have the chunk molding itself to the heightmap. That functionality can easily be done by the manager. I think either is a fine design choice, but in this case, since forming the shape is the only thing it might do, we might as well have the manager do it so we can make the chunk slimmer and remove the containing class entirely.

So we can get away with an instancer management class that manages its MMIs, and a single collision manager class that manages its collision shapes. Both managers creates, moves, configures, and destroys their chunks, but the amount of code to handle those isn't similar enough that it's worth abstracting into a super class. And the chunks already share abstract code in Node3D and Object, so I don't think we need to wrap them any further.

This is the direction I'm going to move in. Consolidating the classes into a single manager with the collision shape as a chunk and using the first commit as a guide, since it works very well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate Collision Dynamically
3 participants