favreau / bullet

Automatically exported from code.google.com/p/bullet
0 stars 0 forks source link

fix btCollisionObject::getCollisionShape const-ness #554

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
It's a simple issue, I had a const btCollisionObject, and I wanted to make a 
modified instance to test for collisions in a new location.  But I get errors 
during the new.setCollisionShape(old.getCollisionShape()) because the collision 
shape returned by old is 'const', and setCollisionShape requires non-const 
argument.

I've attached the simple patch, which merges getCollisionShape into a single 
const accessor (the accessor never modifies data), but returns a non-const 
shape pointer (the pointed-to data is non-const and external).

I notice this confusion a few other places, I'll point out you only need to 
provide const/non-const versions of an accessor if it is providing a reference 
to a class member... returning an external pointer doesn't need a non-const 
version.  Unless there's a philosophical reason that the const-ness of the 
class should carry through the pointer being returned, but I don't think that 
applies here since collision shapes are not exclusively 'owned' by the 
collision object.

On the other hand, you could take this the opposite route, and store/return a 
const btCollisionShape* in btCollisionObject, since bullet probably doesn't 
intend to ever modify a collision shape, this would let the compiler watch for 
erroneous write operations within Bullet, but I looked into that a little and 
it expands into quite a bit of const-ification... not too bad perhaps, but more 
than I want to do on a whim.  So this patch is the quick fix :)

Original issue reported on code.google.com by ejtt...@gmail.com on 5 Oct 2011 at 8:06

Attachments:

GoogleCodeExporter commented 9 years ago
"Unless there's a philosophical reason that the const-ness of the class should 
carry through the pointer being returned"

Yes, in Bullet we try to avoid changing member data for const objects and I do 
not want to start providing non-const data through a const object.

Can you try to keep track of a non-const btCollisionObject and you can change 
the collision shape, or keep track of the non-const collision shape in another 
way?

Original comment by erwin.coumans on 5 Oct 2011 at 10:53

GoogleCodeExporter commented 9 years ago
Aside from this, it would be good to allow to perform a collision query given a 
btTransform and a const btCollisionShape. There is some previous work in a 
branch towards this, and I need to have a look at applying this. See for example

http://code.google.com/p/bullet/source/browse/branches/StackAllocation/src/Bulle
tCollision/BroadphaseCollision/btCollisionAlgorithm.h

So hopefully your issue can be fixed without having to provide non-const 
collision shape pointers.

Original comment by erwin.coumans on 5 Oct 2011 at 10:56

GoogleCodeExporter commented 9 years ago
> Yes, in Bullet we try to avoid changing member data for const objects
> and I do not want to start providing non-const data through a const object.

I'm not sure that makes sense as a blanket architectural principle (IMHO const 
would only propagate through tightly coupled references, e.g. one-to-one or 
'ownership', not shared references like collision shapes), but hey it's your 
show :)

Original comment by ejtt...@gmail.com on 5 Oct 2011 at 11:15

GoogleCodeExporter commented 9 years ago
I agree btw it would be great to have a simple bool collision query, with just 
the shape and transform.  Right now I'm using btCollisionWorld::contactTest, 
with a custom ContactResultCallback which returns false for needsCollision once 
the first call to addSingleResult... but it seems bullet makes a bunch of calls 
to addSingleResult to provide contact points even though I really just want to 
stop after the first... I actually looked into finding a way to short-circuit 
out, but the internals for this looked pretty complicated, so I guess I'm not 
much help on that...

Original comment by ejtt...@gmail.com on 5 Oct 2011 at 11:30

GoogleCodeExporter commented 9 years ago
I applied this StackAllocation patch, so Bullet doesn't modify the collision 
shape anymore. This mean we can consider storing a const btCollisionShape* in 
btCollisionObject.

As the focus is on Bullet 3.x I rather keep 2.x as it is. Let's close the issue.

Original comment by erwin.coumans on 12 Sep 2012 at 8:54