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.

Sign up for GitHub

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

Jump to bottom

Optimized physics object spawn time #39726

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Optimized physics object spawn time #39726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions modules/bullet/area_bullet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ void AreaBullet::main_shape_changed() {
btGhost->setCollisionShape(get_main_shape());
}

void AreaBullet::reload_body() {
void AreaBullet::do_reload_body() {
if (space) {
space->remove_area(this);
space->add_area(this);
Expand All @@ -177,13 +177,15 @@ void AreaBullet::set_space(SpaceBullet *p_space) {
isScratched = false;

// Remove this object form the physics world
space->unregister_collision_object(this);
space->remove_area(this);
}

space = p_space;

if (space) {
space->add_area(this);
space->register_collision_object(this);
reload_body();
}
}

Expand Down
2 changes: 1 addition & 1 deletion modules/bullet/area_bullet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ class AreaBullet : public RigidCollisionObjectBullet {
_FORCE_INLINE_ int get_spOv_priority() { return spOv_priority; }

virtual void main_shape_changed();
virtual void reload_body();
virtual void do_reload_body();
virtual void set_space(SpaceBullet *p_space);

virtual void dispatch_callbacks();
Expand Down
58 changes: 41 additions & 17 deletions modules/bullet/collision_object_bullet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ btTransform CollisionObjectBullet::ShapeWrapper::get_adjusted_transform() const
}

void CollisionObjectBullet::ShapeWrapper::claim_bt_shape(const btVector3 &body_scale) {
if (!bt_shape) {
if (bt_shape == nullptr) {
if (active) {
bt_shape = shape->create_bt_shape(scale * body_scale);
Copy link
Contributor

Choose a reason for hiding this comment

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

A matter of taste, but I prefer the original if (!bt_shape), or better yet, the opposite: if (bt_shape) return; to avoid needing to indent the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result is the same, but in this way it's much more clear that it's a pointer. The indentation just make the code more clear, in this case, for this reason I did it so.

} else {
Expand All @@ -88,6 +88,13 @@ void CollisionObjectBullet::ShapeWrapper::claim_bt_shape(const btVector3 &body_s
}
}

void CollisionObjectBullet::ShapeWrapper::release_bt_shape() {
if (bt_shape != nullptr) {
shape->destroy_bt_shape(bt_shape);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I prefer if (!bt_shape) return;

bt_shape = nullptr;
}
}

CollisionObjectBullet::CollisionObjectBullet(Type p_type) :
RIDBullet(),
type(p_type) {}
Expand Down Expand Up @@ -158,6 +165,13 @@ bool CollisionObjectBullet::has_collision_exception(const CollisionObjectBullet
return !bt_collision_object->checkCollideWith(p_otherCollisionObject->bt_collision_object);
}

void CollisionObjectBullet::prepare_object_for_dispatch() {
if (need_body_reload) {
do_reload_body();
need_body_reload = false;
}
}

void CollisionObjectBullet::set_collision_enabled(bool p_enabled) {
collisionsEnabled = p_enabled;
if (collisionsEnabled) {
Expand Down Expand Up @@ -319,58 +333,68 @@ bool RigidCollisionObjectBullet::is_shape_disabled(int p_index) {
return !shapes[p_index].active;
}

void RigidCollisionObjectBullet::prepare_object_for_dispatch() {
if (need_shape_reload) {
do_reload_shapes();
need_shape_reload = false;
}
CollisionObjectBullet::prepare_object_for_dispatch();
}

void RigidCollisionObjectBullet::shape_changed(int p_shape_index) {
ShapeWrapper &shp = shapes.write[p_shape_index];
if (shp.bt_shape == mainShape) {
mainShape = nullptr;
}
bulletdelete(shp.bt_shape);
shp.release_bt_shape();
reload_shapes();
}

void RigidCollisionObjectBullet::reload_shapes() {
need_shape_reload = true;
}

void RigidCollisionObjectBullet::do_reload_shapes() {
if (mainShape && mainShape->isCompound()) {
// Destroy compound
Copy link
Contributor

Choose a reason for hiding this comment

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

Postponing the reloading of shapes until later causes crashes if those shapes are needed before do_reload_shapes() is run. #40311 and #40840 are just two simple examples. Even with additional triggers to call do_reload_shapes() (e.g. #40886 (or the suggested alternative) and #40252) the crashes still happen.

reload_shapes() is called for numerous reasons:

  • Creating a RigidBody3D
  • Creating an Area3D
  • Adding a Shape
  • Setting a Shape
  • Removing a Shape
  • Changing a Shape
  • Changing a RigidBody's size

Since each of these changes could be made before needing the shapes in a single user _physics_process() loop, there is no way to add sufficient calls to do_reload_shape() without effectively calling do_reload_shape() when calling reload_shapes(), which is exactly what this PR was attempting to avoid.

Furthermore, the test used to demonstrate the effectiveness of this PR is misleading, because it doesn't include the time for subsequently calling do_reload_shapes() on all the RigidBodies added. When including this time, the performance gains are eliminated.

Test project: 39726.zip

Using this test project with 1,000 RigidBodies and Shapes on my laptop I get these total times (i.e. time since _ready() was called):

Godot Physics:
Frame 1: 3.470007
Frame 2: 36.684109
Frame 3: 36.751202
Frame 4: 36.815403
Frame 5: 36.876218
Frame 6: 36.935695
Frame 7: 36.99797
Frame 8: 37.061041
Frame 9: 37.289435
Frame 10: 37.353653

Bullet Physics:
Frame 1: 0.096981
Frame 2: 17.908696
Frame 3: 30.998907
Frame 4: 35.428883
Frame 5: 38.835653
Frame 6: 40.655049
Frame 7: 41.806311
Frame 8: 42.736726
Frame 9: 43.267167
Frame 10: 43.71381

In other words, not only does this PR cause numerous crashes that may never be resolvable, it doesn't achieve the performance gain it set out to achieve; the slow Bullet Frame Rate is still there.

Therefore, I think this PR should be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the code used for the test:

extends Node

var count = 0

var shapes_per_frame := 1000
var shape := BoxShape3D.new()
var t1

func _ready():
	t1 = OS.get_unix_time()
	_do_test()

func _physics_process(delta):
	if count < 10:
		var t2 = OS.get_unix_time()
		count += 1
		print("Frame ", count, ": ", t2 - t1)

func _do_test():
	print("Test start")
	for i in range(0, shapes_per_frame):
		var rb = RigidBody3D.new()
		var c = CollisionShape3D.new()
		c.set_shape(shape)
		rb.add_child(c)
		add_child(rb)
		if i % 50 == 0:
			print("Added: ", i)

To be fair, I think that your test doesn't prove what you claim.

It adds 1000 RigibBody into the scene, into the _ready function. So to know how much time it taken to spawn those RigidBodies - you just need to check the first ever frame, that according to your measurements is slower in Godot Physics: Frame 1: 3.470007 vs Frame 1: 0.096981. All other frame measurements are useless to prove spawn time and so the effectiveness of this PR is proven.

However, is important to say that this PR want to optimize the performance relative to Bullet itself and not relative to GodotPhysics which was running already well.

I don't think that the introduced bugs are a good reason to revert the optimization done instead of fixing the issues. It's like say that the Vulkan implementation is bad because is half broken, or the GDScript v2 is bad because it's not yet fully stable: and so both should be reverted.

The mentioned bug, especially #40840, have a solution: #40886 (comment) please let me know if you want to do that change, if no I can provide a new PR to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not claiming anything. I'm refuting the claims made to support this PR; specifically that it solves #30027. I'm also highlighting why it has introduced numerous crashing issues, and suggesting that a PR that causes more problems than it solves should be reverted.

My test, which is a modified version of the original test provided to support this PR, shows that only looking at the time passed at the start of the first call to _physics_process() is misleading. It is misleading, because, at the time of the first call to _physics_process(), the calls to do_reload_shapes() have not occurred. They are called when the physics server's step() function is called, which happens after _physics_process().

However, more importantly, this PR does not solve issue #30027. The PR reduces the number of times that the function that reloads the shapes is called, but these calls are not the source of the performance problem. The primary source of the performance problem is in the step() function; more specifically, calls to btDiscreteDynamicsWorld::stepSimulation() i.e. within Bullet physics.

While the actual gains of this PR are minimal, the issues caused by this change are significant. The more I investigate the crashing issues caused, the more I'm convinced they will not be solved with multiple band-aid fixes like #40886 (comment) without ultimately reverting this change, or worse: actually increasing the number of calls to do_reload_shapes().

In summary, I believe this PR should be reverted, because it causes multiple crashing issues while doing little to improve actual performance.

Copy link
Contributor Author

@AndreaCatania AndreaCatania Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Check this: #41082 (comment)
This PR fixes the remaining issues: #41096

bulletdelete(mainShape);
}

mainShape = nullptr;

ShapeWrapper *shpWrapper;
const int shape_count = shapes.size();
ShapeWrapper *shapes_ptr = shapes.ptrw();

// Reset shape if required
// Reset all shapes if required
if (force_shape_reset) {
for (int i(0); i < shape_count; ++i) {
shpWrapper = &shapes.write[i];
bulletdelete(shpWrapper->bt_shape);
shapes_ptr[i].release_bt_shape();
}
force_shape_reset = false;
}

const btVector3 body_scale(get_bt_body_scale());

// Try to optimize by not using compound
if (1 == shape_count) {
shpWrapper = &shapes.write[0];
btTransform transform = shpWrapper->get_adjusted_transform();
// Is it possible to optimize by not using compound?
btTransform transform = shapes_ptr[0].get_adjusted_transform();
if (transform.getOrigin().isZero() && transform.getBasis() == transform.getBasis().getIdentity()) {
shpWrapper->claim_bt_shape(body_scale);
mainShape = shpWrapper->bt_shape;
shapes_ptr[0].claim_bt_shape(body_scale);
mainShape = shapes_ptr[0].bt_shape;
main_shape_changed();
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of shapes_ptr[0]., why not use shapes_ptr->?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the pointer of an array, and just above is used as so. I'm not using the dereferencing syntax to keep the code coherent and more clear.

// Nothing more to do
return;
}
}

// Optimization not possible use a compound shape
// Optimization not possible use a compound shape.
btCompoundShape *compoundShape = bulletnew(btCompoundShape(enableDynamicAabbTree, shape_count));

for (int i(0); i < shape_count; ++i) {
shpWrapper = &shapes.write[i];
shpWrapper->claim_bt_shape(body_scale);
btTransform scaled_shape_transform(shpWrapper->get_adjusted_transform());
shapes_ptr[i].claim_bt_shape(body_scale);
btTransform scaled_shape_transform(shapes_ptr[i].get_adjusted_transform());
scaled_shape_transform.getOrigin() *= body_scale;
compoundShape->addChildShape(scaled_shape_transform, shpWrapper->bt_shape);
compoundShape->addChildShape(scaled_shape_transform, shapes_ptr[i].bt_shape);
}

compoundShape->recalculateLocalAabb();
Expand All @@ -389,5 +413,5 @@ void RigidCollisionObjectBullet::internal_shape_destroy(int p_index, bool p_perm
if (shp.bt_shape == mainShape) {
mainShape = nullptr;
}
bulletdelete(shp.bt_shape);
shp.release_bt_shape();
}
26 changes: 23 additions & 3 deletions modules/bullet/collision_object_bullet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,12 @@ class CollisionObjectBullet : public RIDBullet {

struct ShapeWrapper {
ShapeBullet *shape = nullptr;
btCollisionShape *bt_shape = nullptr;
btTransform transform;
btVector3 scale;
bool active = true;
btCollisionShape *bt_shape = nullptr;

public:
ShapeWrapper() {}

ShapeWrapper(ShapeBullet *p_shape, const btTransform &p_transform, bool p_active) :
Expand Down Expand Up @@ -107,6 +108,7 @@ class CollisionObjectBullet : public RIDBullet {
btTransform get_adjusted_transform() const;

void claim_bt_shape(const btVector3 &body_scale);
void release_bt_shape();
};

protected:
Expand All @@ -124,12 +126,17 @@ class CollisionObjectBullet : public RIDBullet {

VSet<RID> exceptions;

bool need_body_reload = true;

/// This array is used to know all areas where this Object is overlapped in
/// New area is added when overlap with new area (AreaBullet::addOverlap), then is removed when it exit (CollisionObjectBullet::onExitArea)
/// This array is used mainly to know which area hold the pointer of this object
Vector<AreaBullet *> areasOverlapped;
bool isTransformChanged = false;

public:
bool is_in_world = false;

public:
CollisionObjectBullet(Type p_type);
virtual ~CollisionObjectBullet();
Expand Down Expand Up @@ -183,13 +190,21 @@ class CollisionObjectBullet : public RIDBullet {
return collisionLayer & p_other->collisionMask || p_other->collisionLayer & collisionMask;
}

virtual void reload_body() = 0;
bool need_reload_body() const {
return need_body_reload;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, need_reload_body() doesn't appear to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it's the getter of a variable, it's there for code completeness.

void reload_body() {
need_body_reload = true;
}
virtual void do_reload_body() = 0;
virtual void set_space(SpaceBullet *p_space) = 0;
_FORCE_INLINE_ SpaceBullet *get_space() const { return space; }

virtual void on_collision_checker_start() = 0;
virtual void on_collision_checker_end() = 0;

virtual void prepare_object_for_dispatch();
virtual void dispatch_callbacks() = 0;

void set_collision_enabled(bool p_enabled);
Expand All @@ -215,6 +230,7 @@ class RigidCollisionObjectBullet : public CollisionObjectBullet, public ShapeOwn
protected:
btCollisionShape *mainShape = nullptr;
Vector<ShapeWrapper> shapes;
bool need_shape_reload = true;

public:
RigidCollisionObjectBullet(Type p_type) :
Expand Down Expand Up @@ -246,8 +262,12 @@ class RigidCollisionObjectBullet : public CollisionObjectBullet, public ShapeOwn
void set_shape_disabled(int p_index, bool p_disabled);
bool is_shape_disabled(int p_index);

virtual void prepare_object_for_dispatch();

virtual void shape_changed(int p_shape_index);
virtual void reload_shapes();
void reload_shapes();
bool need_reload_shapes() const { return need_shape_reload; }
virtual void do_reload_shapes();
Copy link
Contributor

Choose a reason for hiding this comment

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

As far a I can tell, the need_reload_shapes() function is never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it's the getter of a variable, it's there for code completeness.


virtual void main_shape_changed() = 0;
virtual void body_scale_changed();
Expand Down
12 changes: 7 additions & 5 deletions modules/bullet/rigid_body_bullet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ void RigidBodyBullet::KinematicUtilities::copyAllOwnerShapes() {
case PhysicsServer3D::SHAPE_CYLINDER:
case PhysicsServer3D::SHAPE_CONVEX_POLYGON:
case PhysicsServer3D::SHAPE_RAY: {
shapes.write[i].shape = static_cast<btConvexShape *>(shape_wrapper->shape->create_bt_shape(owner_scale * shape_wrapper->scale, safe_margin));
shapes.write[i].shape = static_cast<btConvexShape *>(shape_wrapper->shape->internal_create_bt_shape(owner_scale * shape_wrapper->scale, safe_margin));
} break;
default:
WARN_PRINT("This shape is not supported for kinematic collision.");
Expand Down Expand Up @@ -307,7 +307,7 @@ void RigidBodyBullet::main_shape_changed() {
set_continuous_collision_detection(is_continuous_collision_detection_enabled()); // Reset
}

void RigidBodyBullet::reload_body() {
void RigidBodyBullet::do_reload_body() {
if (space) {
space->remove_rigid_body(this);
if (get_main_shape()) {
Expand All @@ -325,13 +325,15 @@ void RigidBodyBullet::set_space(SpaceBullet *p_space) {
assert_no_constraints();

// Remove this object form the physics world
space->unregister_collision_object(this);
space->remove_rigid_body(this);
}

space = p_space;

if (space) {
space->add_rigid_body(this);
space->register_collision_object(this);
reload_body();
}
}

Expand Down Expand Up @@ -803,8 +805,8 @@ const btTransform &RigidBodyBullet::get_transform__bullet() const {
}
}

void RigidBodyBullet::reload_shapes() {
RigidCollisionObjectBullet::reload_shapes();
void RigidBodyBullet::do_reload_shapes() {
RigidCollisionObjectBullet::do_reload_shapes();

const btScalar invMass = btBody->getInvMass();
const btScalar mass = invMass == 0 ? 0 : 1 / invMass;
Expand Down
4 changes: 2 additions & 2 deletions modules/bullet/rigid_body_bullet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ class RigidBodyBullet : public RigidCollisionObjectBullet {
_FORCE_INLINE_ btRigidBody *get_bt_rigid_body() { return btBody; }

virtual void main_shape_changed();
virtual void reload_body();
virtual void do_reload_body();
virtual void set_space(SpaceBullet *p_space);

virtual void dispatch_callbacks();
Expand Down Expand Up @@ -315,7 +315,7 @@ class RigidBodyBullet : public RigidCollisionObjectBullet {
virtual void set_transform__bullet(const btTransform &p_global_transform);
virtual const btTransform &get_transform__bullet() const;

virtual void reload_shapes();
virtual void do_reload_shapes();

virtual void on_enter_area(AreaBullet *p_area);
virtual void on_exit_area(AreaBullet *p_area);
Expand Down
Loading