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

Planning for separate b2 physics server thread #37

Open
briansemrau opened this issue Dec 13, 2020 · 23 comments
Open

Planning for separate b2 physics server thread #37

briansemrau opened this issue Dec 13, 2020 · 23 comments
Labels
discussion This issue needs deliberation enhancement New feature or request

Comments

@briansemrau
Copy link
Owner

The biggest change ahead for this project will be moving the b2world (and all b2 objects) onto a separate physics thread. Before work begins on this, I think major design decisions should be well thought out.

Primary goals of this enhancement:

  1. Greatly improve physics processing performance by taking it off the main thread
  2. Similar to Physics2DServer, offer a script-only option for devs by providing a physics server API so that performance may be improved by eliminating the need for the scene tree.

Initial thoughts:
I believe it would be a good idea to implement this in a similar way to the Godot physics 2d server. I.e. creating our own Box2DPhysicsServer and Box2DPhysicsServerWrapMT. In turn, all Box2D nodes will use this API for handling properties and managing the physics object lifecycle as it relates to the scene tree.
If we follow this preexisting design, it reduces the amount of architecting to do and makes for an easier transition for Godot users switching to this module.

Note:
Because creating an API for everything (I expect) involves a significant amount of repeat work, I do not plan to finish issue #2 before this is done, unless any joints are needed for project-specific use cases.

@briansemrau briansemrau added enhancement New feature or request discussion This issue needs deliberation labels Dec 13, 2020
@jordo
Copy link
Collaborator

jordo commented Dec 13, 2020

I think the greatest improvement is just going to have a multi-threaded solver. Basically what you linked here: https://github.com/jhoffman0x/Box2D-MT. I haven't looked into the source, but presumably with a multi-threaded solver, when you have physics work to do you can just step the world and it will fill all available cpu cores with work to hopefully near capacity.

Moving physics performance to another thread in and by itself doesn't make anything more efficient if you don't have any other work to do on your main thread. If say I want to take user input, apply some impulses, update the physics simulation (step the world), and then render the new positions all on the same frame. Hoping off the main thread just to simulate physics, then waiting for the physics thread to finish so we can submit our updated geometry for rendering doesn't really speed anything up.

Godot gets around this by just lagging out another frame. A large cpu bottleneck in godot is recording/encoding and submitting work to the gpu for rendering (Rendering/VisualServer draw()). So godot basically starts rendering geometry while the physics system is simulating, (waiting for physics simulation to finished is synced() at the START of the next frame tick), which means an additional frame of lag. This is generally a good trade-off for most, and this is all fine but it has really interesting design consequences with regards to interacting with the solver.

Interacting with the solver via hooks (pre-solve, post-solve. I'm not going to say callbacks, because callbacks are somewhat synonymous with asynchronous exec), just needs to be thought out well, especially in a multi-threaded environment. Godot syncs physics positions at the start of the next frame iteration. Which means that while while the physics engine is solving in a separate thread, the CPU is recording command buffers, encoding and submitting current geometry for rendering. Pre-solve in b2d needs to return immediately, because it needs to be fast due to the internal box2d solver needing an answer immediately during a solve. i.e. you can't queue these up, and just asynchronously handle them at the start of the next frame. The solver needs the info immediately to resolve the response. How to synchronize that is going to be tricky, and there's going to be some tradeoffs when a application programmer wants to push the world step into another thread and execute that step at the same time as say generating render commands.

I have to think a little more on the design of this. I think pre-solve and post-solve integration is an extremely useful feature. I'm not 100% what the design will be in the context of a separate thread in a physics server.

@jordo
Copy link
Collaborator

jordo commented Dec 13, 2020

Actually i just checked the godot source, and godot syncs at the start of the next physics tick, not necessarily the start of the next frame. Which is the correct behaviour, but somewhat makes it more difficult to sync with the solver in real-time. (Engine can go another whole frame render loop while solver is still doing it's thing).

@briansemrau
Copy link
Owner Author

You bring up a lot of good points. While just adding a separate thread may be purposeless when we have Box2D-MT (this will need good testing), I still believe there is value in adding a script API, as adding hundreds of bodies via the scene tree is far from efficient.

Though rather than me making a speculative opinion about the API, I might try to actually measure performance before I continue my argument.

@briansemrau
Copy link
Owner Author

As for pre/post solve, I believe signals may be adequate for this. Signals can be called asynchronously if you don't do a deferred emit. We can pass some object, call it ContactState, that has properties that give us access to the b2contact. Developers just have to be careful to use these functions in a thread-safe way.

@briansemrau
Copy link
Owner Author

It may be nice to have a project setting that can configure whether Box2D runs on the main thread or asynchronously so devs can choose whether they want to deal with thread safety in exchange for better performance.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

There's a per project thread setting for this, under 2d physics. I think the Box2D worlds should just by use this by default project based setting, unless overridden on the object itself.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

For pre and post solve thread safety, ya I may just punt on this, But I still want to eliminate as many footguns as possible.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

And easy start is a check if the world is locked on b2 calls that don't work if the world is locked (should be there anyways). We should catch it as a no-op, and print a warning or error msg to godot output. The alternative is to maintain a proxy state, which I'm not 100% opposed to, but it introduces some additional complexity.

@briansemrau
Copy link
Owner Author

The fail condition error printout should mention to use call_deferred for everything handled in async. This handles holding onto the state for us, as well as keeps the dev's application script clear that it isn't updating values immediately.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

call_deferred is going to get pumped at these three places in this screenshot below: (flush() on message_queue)

Screen Shot 2020-12-14 at 10 56 09 AM

We actually only ever want these to be executed in the first flush(). If the deferred execution happens on the second or third call to flush, we're going to be in sync trouble again ( In a separate thread, the world is already stepping and may be locked again )

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

for example, line 2484 will start a world tick in a physics thread. It COULD have already queued up a call_deferred from some collision that would get processed on line 2486, which will break.

Since the queue is pumped as well on line 2501, again, we may just call_deferred into another in-consistent world state

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

It's kind of like, we want to defer these things, until the next physics_process. Which is protected from being inside a simultaneous world step().

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

line 2479, will have all the process_physics() execution. Which is after the sync(), but before the step(). Which is the thread safe spot for interacting with a separate physics thread.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

Anyways, it's all good. I'll figure out something that will be elegant.

@briansemrau
Copy link
Owner Author

Yup, I see the difficulty. Best of luck.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

I think we should really considering pushing this back in priority (a separate physics thread).

  1. there probably needs to be architectural changes in godot upstream to do this properly. We need to have a callback mechanism into the multi-threaded safe 'zone'. Which is between a physics->sync(), and a physics->step(). (world has finished stepping, and hasn't started the next step yet). This is accomplished in PhysicsServer2DWrapMT using a semaphor. NOTE - there isn't even support for running any 3d physics engine multi-threaded. Both godot3d, and bullet3d only run single threaded in godot.

  2. most of the benefit is going to come from a multi-threaded solver. Which is still single threaded step(), but multi-threaded solving within the solver's implementation.

PRO's to this:

  • pre-solve and post-solve callback simplicity
  • reduces an extra physics frame of latency. (step() and updating geometry happen in the same frame )

@briansemrau
Copy link
Owner Author

Yeah, I'm not fighting to implement this anytime too soon.

Though, a possible solution crossed my mind. It's a bit of a compromise. Right now b2World.Step() happens on the main thread in internal_physics_process. What if we wait for the threads running in b2world.step() to finish before we continue processing on the main thread? This makes everything thread-safe (aside from hooks) but still benefits from having multiple threads.
This method should take almost no effort to implement. Doing this, (as opposed to having step() occur concurrently with the main thread), bottlenecks the physics step only to the most complex island. This should enable larger scenes to step much faster, with minimal return on small scenes (which should be fast anyway). Thoughts?

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

I assume that's how https://github.com/jhoffman0x/Box2D-MT works.

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

(On further inspection, that's indeed how it works :)

@briansemrau
Copy link
Owner Author

Yeah, okay, hah I feel silly. But my idea still stands! Just swap Box2D with Box2D-MT (after maybe forking and pulling some upstream changes from the original repo) and it's an immediate improvement.

@briansemrau
Copy link
Owner Author

With some of the questions I ask, sometimes I wonder how I got this far writing software lol

@jordo
Copy link
Collaborator

jordo commented Dec 14, 2020

:D It looks like we can even pass https://github.com/jhoffman0x/Box2D-MT the godot worker thread pool, which eliminates the spawning of new threads inside https://github.com/jhoffman0x/Box2D-MT, which is a great design.

@briansemrau
Copy link
Owner Author

briansemrau commented Dec 16, 2020

Switching to Box2D-MT won't be a seamless transition because it uses an older version of Box2D. We're using the latest and there are a number of breaking changes between versions. Also, the file structure and filenames are different (due to changes in 2.4.0).

I want to create a fork of Box2D-MT and merge Box2D upstream changes, which will take a fair amount of effort. However, there's also a dev branch in MT with unfinished changes (last updated May 2019) that would become hard to merge into the fork in the future after updating file structure and filenames.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue needs deliberation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants