Float/double serialization issues

[GoogleCodeExporter]

Hello Erwin,

Unfortunately we did not have time until now to check if the changes in bullet 
solve all of our serialization problems. Now we managed to create a list of our 
remarks about already closed "Issue 734: Complete float/double serialization".

We identified some minor issues (we included fixes/modifications for almost all 
of them in a patch file):
- btBulletXmlWorldImporter::auto_serialize_root_level_children is not finished 
for constraints (btAssert(0);//todo). It is not a problem for us as we do not 
use this class.
- btHingeConstraint::serialize still casts some btScalar value to float.
- struct btConeTwistConstraintData should not be guarded by 
BT_BACKWARDS_COMPATIBLE_SERIALIZATION as there is no dedicated float 
serialization structure.
- btWorldImporter::convertRigidBodyFloat/Double does not use some serialized 
members for deserialization. Maybe not all is needed, but we can not filter out 
which are the important ones and which can be neglected. Inertia 
(m_invInertiaLocal and m_invInertiaTensorWorld members of 
btRigidBodyFloat/DoubleData) is not yet used, because it is recalculated from 
shape and mass data, and we did not yet check if it is OK for us this way, but 
we will check it.
- there is something wrong with either the deserialization of the margin/size 
of capsule shape in btWorldImporter::convertCollisionShape or the handling of 
the margin and scale is not correct in btCapsuleShape constructors as 
m_implicitShapeDimensions is set directly without considering margin and scale. 
For now we have changed the deserialization of capsule shapes (as we do not use 
scaling and margin). We have also changed the size calculation for the box and 
cylinder a little bit, please take a look at it.
- btGeneric6DofConstraint::serialize should fill not used vector components 
with 0-s just to be nice. (It was useful for us when comparing different 
.bullet files.)

We also collected some issues about serialization where we need some advice and 
have some questions:
- unfortunately there is another problem with constraint float serialization. 
It seems that in the current version usage of btTypedConstraintData and 
btTypedConstraintFloatData is messed up a little bit: e.g. 
btPoint2PointConstraintFloatData has a btTypedConstraintData member (to be 
compatible with older versions) but that is guarded by the 
BT_BACKWARDS_COMPATIBLE_SERIALIZATION flag, which is not correct this way. 
Furthermore serialization of btPoint2PointConstraint calls back 
btTypedConstraint::serialize giving a pointer to the btTypedConstraintData 
member as the first parameter while btTypedConstraint::serialize uses this 
pointer as a btTypedConstraintFloatData (when BT_USE_DOUBLE_PRECISION is not 
defined), which is not correct. btTypedConstraintFloatData is only used in 
btGearConstraint as a member. We suggest removing btTypedConstraintFloatData 
and using btTypedConstraintData as the float version without the 
BT_BACKWARDS_COMPATIBLE_SERIALIZATION guard. The only problem with 
btTypedConstraintData in this case is that m_rbA and m_rbB members are 
btRigidBodyData* instead of btRigidBodyFloatData* which is not nice at all, but 
we think it does not cause any problems. Is this solution acceptable for you? 
Should I create a patch with this modification?
- using Windows/Visual C++ 2010 the default alignment is 8 bytes for doubles 
even in x86 (32 bit) mode. This causes a problem when the serialization 
structure is processed using the DNA, e.g. in 
bFile::resolvePointersStructRecursive as the extra padding for doubles are not 
considered there. We have 2 ideas, but do not know which one would fit you 
better: either the default packaging should be changed for the Visual C++ 
project files of bullet (but we do not know if it will cause any side effects) 
or the makesdna should be improved to give a warning/error about this case and 
a special padding member should be used (we already implemented such for the 
DFF serialization (CONDITIONAL_ALIGN)). Which one is better? Or do you have any 
other ideas?

What version of the product are you using? On what operating system?
2.82.2712, Win32 build on Windows7 with BT_USE_DOUBLE_PRECISION defined.

Original issue reported on code.google.com by nsark...@gmail.com on 11 Nov 2013 at 3:40

Attachments:

• serialization_fix.patch

[GoogleCodeExporter]

Since then we managed to check the inertia problem in 
btWorldImporter::convertRigidBodyFloat/Double. We suggest to use the saved 
inertia members (colObjData->m_invInertiaLocal and 
colObjData->m_invInertiaTensorWorld) instead of recalculating those because of 
the following reasons:
- btWorldImporter::createRigidBody uses the 
btCollisionObject::setWorldTransform function of the created rigid body to set 
its world/center of mass transformation, but this function does not update the 
inertia tensor (contrary to btRigidBody::setCenterOfMassTransform). This way 
the inertia tensor will not be restored correctly.
Though it is true that calling the btRigidBody::updateInertiaTensor would be 
enough after setting the world transform of the newly created rigid body, but 
it would not solve the second problem that follows.
- there are cases when the original mass can not be restored from the inverse 
mass because of finite floating point precision. In this case considering the 
usual setup of inertia and mass (see below), the mass can not be restored 
exactly from saved data and this way the localInertia can not be restored 
exactly:
// Creating rigid body having a btScalar mass, a btCollisionShape* shape
    btVector3 localInertia;
    localInertia.setZero();

    if (mass)
        shape->calculateLocalInertia(mass,localInertia); // Local inertia is calculated from mass

    btRigidBody* body = new btRigidBody(mass,0,shape,localInertia); // btRigidBody only stores inverse mass
...

// This is how btWorldImporter tries to restore it:
    btScalar mass = btScalar(colObjData->m_inverseMass? 1.f/colObjData->m_inverseMass : 0.f);
...
    btVector3 localInertia;
    localInertia.setZero();

    if (mass)
        shape->calculateLocalInertia(mass,localInertia); // Problem here, mass is not the same as when rigid body was created

    btRigidBody* body = new btRigidBody(mass,0,shape,localInertia); // No real problem here, 1 / mass will evaluate to the same as when created

We attached a patch where the inertia is deserialized to the created rigid body 
based on the original revision 2712 but we can also provide a patch where all 
the serialization modifications are included if needed.

Original comment by nsark...@gmail.com on 13 Nov 2013 at 12:49

Attachments:

• deserialize_inertia.patch

[GoogleCodeExporter]

That is a long wall of text, I hope to find some time to digest and test all.

>> or the makesdna should be improved to give a warning/error 
>>about this case and a special padding member should be used

Yes, we need new padding rules/warnings for double-precision to avoid issues 
indeed.

Original comment by erwin.coumans on 17 Nov 2013 at 8:16

[GoogleCodeExporter]

moved to github https://github.com/bulletphysics/bullet3/issues/74

Original comment by erwin.coumans on 30 Mar 2014 at 5:36

• Changed state: Done