turanszkij / WickedEngine

3D engine with modern graphics
https://wickedengine.net
MIT License
5.45k stars 568 forks source link

Improve entity remove #828

Closed rjallanreal closed 3 weeks ago

rjallanreal commented 1 month ago

I was seeing massive drop in framerate when removing entities during gameplay. Obviously, it's possible to simply transform entities instead of removing them, but this is more difficult and potentially a memory bloat when spawning a lot of projectiles.

I have gotten rid of the recursive call from Entity_Remove, instead traversing through the hierarchy and populating a vector of entities to remove. I also put the removal on a separate thread. Potentially a spin lock might be needed, but in my testing I did not see any issues with the current implementation.

In addition, I have included the check on the objects index in the Intersects function. This time I put it in the condition of the loop.

turanszkij commented 1 month ago

I don't like this, you didn't even wait for the job to finish, this might work in some cases, but it will fail if you access anything while removal job is still going. Why did you implement Remove functions again instead of calling the existing ones for each element? I think if you're not satisfied with the existing entity remove function, this threading can be implemented in your project instead.

What about this for example:

wi::jobsystem::context ctx;
wi::jobsystem::Execute(ctx, [](wi::jobsystem::JobArgs args){
  scene.Entity_Remove(entity);
  scene.Entity_Remove(entity2);
  ...
});

// do other stuff in your game that is not using scene while removals are happening...

wi::jobsystem::Wait(ctx); // wait for removals
rjallanreal commented 1 month ago

To address your points:

  1. I don’t wait for the job because the point is to improve performance by allowing it to happen on a different thread. If I wait on it immediately, I might as well not have dispatched a job.

  2. I created a new remove overload because it is actually faster to nest the loops in the order that I have them. As an entity is discovered and deleted, the time spent iterating through subsequent component types is decreased.

  3. Using the job system at a higher level is a good idea, but is currently not possible through the lua binding. I am calling the Entity_Remove from a lua script.

turanszkij commented 1 month ago

It would be better to add a lua binding for async removal that can also be waited.

turanszkij commented 3 weeks ago

Async removal added for lua in this PR: https://github.com/turanszkij/WickedEngine/pull/842