API refactoring (was ugliness)

[Erwin Coumans]

Update Bullet 2.11:
* more demos have been converted to use btDynamicsWorld
* more consistent memory strategy (who creates things needs to delete them)
* removed Extras/PhysicsInterface folder

Code: Select all

btDiscreteDynamicsWorld::~btDiscreteDynamicsWorld()
{
	//only delete it when we created it
	if (m_ownsIslandManager)
		delete m_islandManager;
	if (m_ownsConstraintSolver)
		 delete m_constraintSolver;
}

Thanks for the help, pointing out memory issues, more feedback is welcome!
Erwin

[DevO]

What is now with MyMotionState (PHY_IMotionState) is there equivalent for it?
It was really handily to use.

Do Bullet 2.12 support kinematic objects?

[Erwin Coumans]

What is now with MyMotionState (PHY_IMotionState) is there equivalent for it?
It was really handily to use.

Do Bullet 2.12 support kinematic objects?

There is no replacement at the moment for *MotionState. I consider adding it in Bullet 2.13, together with some logic so it only updates 'active' objects (and not the sleeping/deactivated ones).

There is no replacement for kinematic support yet either (it was in CcdPhysicsEnvironment). You can track the request here:
http://code.google.com/p/bullet/issues/list

Thanks for the feedback,
Erwin

[SteveBaker]

There are a bunch of global variables exported by src/BulletDynamics/Dynamics/btRigidBody.h :

Code: Select all

  extern float gLinearAirDamping;
  extern bool gUseEpa;

  extern float gDeactivationTime;
  extern bool gDisableDeactivation;
  extern float gLinearSleepingTreshold;
  extern float gAngularSleepingTreshold;

It's not good to export globals from the API, especially since they don't conform to the 'btXXXX' naming scheme. At least one of the demo's uses them (CcdPhysicsDemo sets gDisableDeactivation) - so if they are reallyimportant, they should probably be rolled into one or other of the main Bullet classes with appropriate get/set functions.

[Erwin Coumans]

I agree, and I am against using global variables and singletons. Those were some internal testing globals, and if they prove to be useful they will get a proper API. I will put it on the TODO.

Thanks Steve,
Erwin

There are a bunch of global variables exported by src/BulletDynamics/Dynamics/btRigidBody.h :

Code: Select all

  extern float gLinearAirDamping;
  extern bool gUseEpa;

  extern float gDeactivationTime;
  extern bool gDisableDeactivation;
  extern float gLinearSleepingTreshold;
  extern float gAngularSleepingTreshold;

It's not good to export globals from the API, especially since they don't conform to the 'btXXXX' naming scheme. At least one of the demo's uses them (CcdPhysicsDemo sets gDisableDeactivation) - so if they are reallyimportant, they should probably be rolled into one or other of the main Bullet classes with appropriate get/set functions.

[SteveBaker]

In src/BulletCollision/CollisionDispatch/btConvexConvexAlgorithm.cpp and in the CcdPhysicsDemo, there are references to:

m_ccdSweptShereRadius

Is that a typo? I presume it should be 'm_ccdSweptSphereRadius'.

[Erwin Coumans]

In src/BulletCollision/CollisionDispatch/btConvexConvexAlgorithm.cpp and in the CcdPhysicsDemo, there are references to:

m_ccdSweptShereRadius

Is that a typo? I presume it should be 'm_ccdSweptSphereRadius'.

Thanks for pointing it out Steve, it has been fixed in version 2.17

Erwin

[SteveBaker]

In the sample programs, you have a lot of application program functions that start with the string 'glut' - that's not a wise move. The entire point of having all actual GLUT functions beginning with 'glut' is precisely to avoid the risk of some GLUT function coincidentally havintg the same name as some application function. If you go around calling application functions 'glutXXXXX' then you are asking for problems in the future.

(eg 'glutMotionFuncCallback')

[SteveBaker]

From reading the demo programs, it seems that the only way to perform some operations on btRigidBody objects is to access the raw member variables of the base class (eg, the Demos/OpenGL/DemoApplication.cpp reads and writes 'm_worldTransform'). This is kinda nasty because it means you can't tell when one of these fields has been touched if you ever need to in the future.

There should be inlined access functions for these things with all of the member variables of public classes made 'private'.

(Also, there is a typo: m_ccdSweptMotionTreshold ...should be 'Threshold')

[Erwin Coumans]

From reading the demo programs, it seems that the only way to perform some operations on btRigidBody objects is to access the raw member variables of the base class (eg, the Demos/OpenGL/DemoApplication.cpp reads and writes 'm_worldTransform'). This is kinda nasty because it means you can't tell when one of these fields has been touched if you ever need to in the future.

There should be inlined access functions for these things with all of the member variables of public classes made 'private'.

(Also, there is a typo: m_ccdSweptMotionTreshold ...should be 'Threshold')

Thanks for pointing out the typo, I'll rename it.

Bullet software makes distinguishment between classes and structs. The btCollisionObject is a struct at the moment, and for structs I allow direct data access. However, I agree it is not a good habbit. We can clean this up, but in general some structs are really mostly public data, which is not intended to be private.

More important, the world transform should be read/written by btMotionState. I'll probably remove the btRigidBody constructor that passes in a worldtransform, and you just pass the btMotionState (that contains the graphics worldtransform, including potential center-of-mass shifts etc).

Thanks for the useful feedback,
Erwin

[SteveBaker]

Bullet software makes distinguishment between classes and structs. The btCollisionObject is a struct at the moment, and for structs I allow direct data access.

Yeah - for simple structs with no member functions, I would also leave the member variables exposed - but btRigidBody is a CLASS (admittedly derived from a STRUCT). To all intents and purposes, it's a class with exposed internals.

More important, the world transform should be read/written by btMotionState. I'll probably remove the btRigidBody constructor that passes in a worldtransform, and you just pass the btMotionState (that contains the graphics worldtransform, including potential center-of-mass shifts etc).

OK - but the problem for application writers is that the only documentation of any use is the demo programs - and they don't seem to be using btMotionState. If the demo's don't use "best practices" then we can't expect application authors to guess what we expect them to do.

[Erwin Coumans]

Bullet software makes distinguishment between classes and structs. The btCollisionObject is a struct at the moment, and for structs I allow direct data access.

Yeah - for simple structs with no member functions, I would also leave the member variables exposed - but btRigidBody is a CLASS (admittedly derived from a STRUCT). To all intents and purposes, it's a class with exposed internals.

More important, the world transform should be read/written by btMotionState. I'll probably remove the btRigidBody constructor that passes in a worldtransform, and you just pass the btMotionState (that contains the graphics worldtransform, including potential center-of-mass shifts etc).

OK - but the problem for application writers is that the only documentation of any use is the demo programs - and they don't seem to be using btMotionState. If the demo's don't use "best practices" then we can't expect application authors to guess what we expect them to do.

OK. Let's put it on the todo, to modify all demos so they consistently use getCenterOfMassTransform and btMotionState. The demos are using btMotionState to synchronize the graphics for rendering, see DemoApplication::localCreateRigidBody.

Also, I will add this to the Bullet_User_Manual.pdf. Other feedback requests for reorganizing the Wiki so it becomes more useful and adding a User Gallery so users can share their work using Bullet better.

Thanks,
Erwin

[Erwin Coumans]

Hi Steve,

This has been fixed in SVN: btCollisionObject is a class now. Its members like m_worldTransform are not public anymore, and required set/getWorldTransform(). Also, btMotionState is required now, to synchronize world transform properly.

Thanks for the feedback. Hopefully no big refactoring changes anymore, so we can focus on improving quality and performance
Erwin

Bullet software makes distinguishment between classes and structs. The btCollisionObject is a struct at the moment, and for structs I allow direct data access.

Yeah - for simple structs with no member functions, I would also leave the member variables exposed - but btRigidBody is a CLASS (admittedly derived from a STRUCT). To all intents and purposes, it's a class with exposed internals.

More important, the world transform should be read/written by btMotionState. I'll probably remove the btRigidBody constructor that passes in a worldtransform, and you just pass the btMotionState (that contains the graphics worldtransform, including potential center-of-mass shifts etc).

OK - but the problem for application writers is that the only documentation of any use is the demo programs - and they don't seem to be using btMotionState. If the demo's don't use "best practices" then we can't expect application authors to guess what we expect them to do.

[SteveBaker]

Many thanks!

I'll have a bit of time over the weekend - I'm getting my game back up and working with all of this refactored goodness.

[Noehrgel]

while generating the bt stuff a minor error happened. Just do a

Code: Select all

grep bteral `find <yourbulletdir> *`

(while doing some substitution every "Gen" got replaced by a "bt" what produced a "GNU bteral Public License")

Noehrgel