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

[3.x] Move Bullet physics query flush from Bullet space pre-tick callback to Bullet physics flush_queries() again. #40788

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Jul 28, 2020

This reverts commit 10544f1.

As per discussion here, #40185 was reverted for 3.2.3, because of a regression issues. Once all the regression issues have been resolved this change can be reapplied.

Regression issues that need to be resolved before applying this PR:

Edit: I've added the fix for #40508 as a separate commit to this PR.

Closes #37702

@madmiraal madmiraal requested a review from AndreaCatania as a code owner July 28, 2020 12:07
@akien-mga akien-mga added this to the 3.2 milestone Jul 28, 2020
@akien-mga
Copy link
Member

I think re-cherrypicking the fix might look better than a revert of a revert (which looks like we messed up twice :P), but from a purely technical point of view this is fine :)

@madmiraal
Copy link
Contributor Author

Reverting the revert was the easiest way to ensure it was correct. Cherry-picking from master fails, because of the file renames and would require manual changes. Ultimately, it's just an automatic commit message, which can be changed. If you want me to change it, let me know.

@madmiraal
Copy link
Contributor Author

Since #40252 also has a regression (#40840), I've added the fix for #40508 as a new commit to this PR.

@akien-mga akien-mga requested a review from a team January 15, 2021 15:15
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@akien-mga akien-mga modified the milestones: 3.2, 3.3 Mar 17, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@akien-mga akien-mga changed the title [3.2] Move Bullet physics query flush from Bullet space pre-tick callback to Bullet physics flush_queries() again. [3.x] Move Bullet physics query flush from Bullet space pre-tick callback to Bullet physics flush_queries() again. Mar 26, 2021
@Chaosus Chaosus modified the milestones: 3.4, 3.5 Nov 8, 2021
@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 2, 2022
@jknightdoeswork
Copy link

jknightdoeswork commented Apr 20, 2024

Necroing this issue. Just discovered that the Bullet Physics system is 'off by 1' frame because of this. ie: body.position is off by 1 frame from what is in the physics engine. This should probably be merged. On my branch, I just tried moving the flush from the 'pre tick' to the 'post tick'. Seems to work great.

Would like @madmiraal or @AndreaCatania to comment here as it seems like there were some issues here.

Nevermind, it's still of by 1. The problem is even deeper. The "godot motion state" used by the bullet server is introducing some kind of physics interpolation at a deep level. I'm disabling that completely. For clarity: I'm working on networking the bullet physics server. I have it working but I have to use PhysicsServer::get_body_state() in order to get the real position of the object, rigid_body.translation is unusable and contains invalid values.

I am capable of networking the Godot physics server in Godot 3 as well and will be doing that soon.

I can create a PR with the changes I've made to the Bullet physics server to enable it to be networked, but it would only be worth doing if some of the frequent contributors like madmiraal or Andrea Catania want to review it. It seems like the Bullet physics server has a lot of 'edge cases' and band-aid code over various systems like the kinematic character controller, etc, that I'm not very familliar with and likely will not heavily test.

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