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

Improved Bullet Physics flush algorithm, Lazy collision filter reload, Shape reload regression fix. #40252

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

AndreaCatania
Copy link
Contributor

@AndreaCatania AndreaCatania commented Jul 10, 2020

  • Improved flush algorithm, so now when a RigidBody goes into sleeping mode, it's flush is not executed (as should have be).
  • Added pre flush list, used by areas, so their notifications arrives to RigidBody before those get flushed.
  • Make sure to correctly fetch gravity when the integrate_forces function is used
  • Lazy reload body when layer and mask changes
  • Shapes are reloaded just before the physics step starts.
  • Improved some other parts of the code.
  • Removed std::vector and Vector in favour of the recently added LocalVector.

Check this PR: #40186
Improves this: #40127

Fixes #40508
Fixes #40311
Fixes #40506

Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

I don't think trying to control the order that the flush happens is the right approach; especially not by creating multiple queues, which significantly increases the complexity of the flush system without the flexibility of varying the number of priorities.

Also, as highlighted below, your solution appears to require the Standard Template Library's std::vector, which I thought we shouldn't be using.

modules/bullet/rigid_body_bullet.h Outdated Show resolved Hide resolved
modules/bullet/space_bullet.cpp Outdated Show resolved Hide resolved
modules/bullet/space_bullet.h Outdated Show resolved Hide resolved
@AndreaCatania
Copy link
Contributor Author

The old solution was really bad because all the rigid bodies are processed, even the one that are not needed. Instead, the queue allows to only process the bodys that need to. Exactly like the Godot nodes are processed.

The solution of single queue, has the big disadvantage that before its use it must be sorted. This is acceptable when each element can have N priorities. For this use case, we have just 2 priorities (and this is unlikely to change), so use two separate queues is much more convenient.

Regarding the std::vector: Back when I've implemented it, the Vector was not using COW and it was perfect for this use case. However, later COW got implemented and that added a significant overhead for this use case.
Since avoid the Vector copy on write makes the code more complex, and since as far as I know is fine use std::vector into modules, I preferred use the std version.

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Jul 19, 2020

Update: Rebased and fixed some bugs listed in the main post.

@AndreaCatania AndreaCatania changed the title Improved Bullet Physics flush algorithm Improved Bullet Physics flush algorithm, Lazy collision filter reload, Shape reload regression fix. Jul 19, 2020
@akien-mga akien-mga requested a review from reduz July 20, 2020 12:00
@AndreaCatania
Copy link
Contributor Author

Removed std::vector and Vector in favour of the recently added LocalVector.

@AndreaCatania
Copy link
Contributor Author

Force push: Removed invalid comment, It was no more valid since std::vector is gone.

Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

I haven't tested whether or not this PR solves the issues indicated, but aside from the few minor points below I think the changes look good. 👍

modules/bullet/area_bullet.cpp Show resolved Hide resolved
modules/bullet/area_bullet.h Show resolved Hide resolved
modules/bullet/collision_object_bullet.cpp Show resolved Hide resolved
modules/bullet/collision_object_bullet.h Show resolved Hide resolved
modules/bullet/collision_object_bullet.h Show resolved Hide resolved
modules/bullet/collision_object_bullet.h Show resolved Hide resolved
modules/bullet/rigid_body_bullet.cpp Show resolved Hide resolved
modules/bullet/rigid_body_bullet.h Show resolved Hide resolved
modules/bullet/space_bullet.cpp Show resolved Hide resolved
- Flushing Areas before anything else.
- Make sure to correctly fetch gravity when the integrate_forces function is used
- Lazy reload body when layer and mask changes
- Shapes are reloaded just before the physics step starts.
- Improved some other parts of the code.
- Added override keyword
- Using LocalVector
@AndreaCatania
Copy link
Contributor Author

All fixed

@akien-mga akien-mga merged commit 3e87022 into godotengine:master Jul 27, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Two apparent regressions from this PR: #40840 #40848.

Vector<Vector3> contactDebug;
int contactDebugCount = 0;
LocalVector<Vector3> contactDebug;
uint32_t contactDebugCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be snake_case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it should be. Also I would check if make sense remove it (now that we have LocalVector)

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