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

Fix MotionState #22

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
3 participants
@Squareys
Contributor

Squareys commented Oct 11, 2016

Hi @mosra !

Here is the mentioned pullrequest to finally fix the behaviour of BulletIntegration::MotionState with parents in the SceneGraph.

While I was at it, I added more conversions, a test and did the @todoc in MotionState.h

Regards, Squareys.

@coveralls

This comment has been minimized.

coveralls commented Oct 12, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 1f8c521 on Squareys:fix-motion-state into 2d154fa on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 12, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 089a767 on Squareys:fix-motion-state into 2d154fa on mosra:master.

Squareys added some commits Oct 11, 2016

BulletIntegration: Add conversions between btTransform and Matrix4
Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Add conversion between btQuaternion and Quaternion
Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Add test for MotionState
Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Add usage documentation for MotionState
Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Include Corrade/Utility/Macros in DebugDraw.
For `CORRADE_UNUSED`.

Signed-off-by: Squareys <Squareys@googlemail.com>
BulletIntegration: Clean up includes in MotionState.h
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:fix-motion-state branch from 089a767 to b34e45d Oct 13, 2016

BulletIntegration: Fix local transforms from bullet world transforms
fixup! start fixing MotionState

Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:fix-motion-state branch from b34e45d to ad27c9d Oct 13, 2016

BulletIntegration: Organize includes in IntegrationTest
Signed-off-by: Squareys <Squareys@googlemail.com>
@coveralls

This comment has been minimized.

coveralls commented Oct 13, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 58b5a94 on Squareys:fix-motion-state into 2d154fa on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 13, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 58b5a94 on Squareys:fix-motion-state into 2d154fa on mosra:master.

@Squareys Squareys changed the title from [WIP] Fix MotionState to Fix MotionState Oct 13, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 13, 2016

Coverage Status

Coverage remained the same at 0.0% when pulling 58b5a94 on Squareys:fix-motion-state into 2d154fa on mosra:master.

@@ -24,5 +25,5 @@
# DEALINGS IN THE SOFTWARE.
#
corrade_add_test(BulletIntegrationTest IntegrationTest.cpp LIBRARIES MagnumBulletIntegration)
corrade_add_test(BulletIntegrationTest IntegrationTest.cpp LIBRARIES MagnumBulletIntegration ${BULLET_LIBRARIES})

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

This shouldn't be needed -- the bullet libraries are already a dependency of MagnumBulletIntegration.

/* Setup rigid body which will receive the motion state */
MotionState motionState{child};
btSphereShape collisionShape{0.0};

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

This should be 0.0f -- some compilers are taking type narrowing very seriously. I think you should get at least a warning from the compiler.

Btw., how is it with btScalar? Is it always float or can it be switched to double at compile time?

#include "Magnum/BulletIntegration/Integration.h"
#include "Magnum/BulletIntegration/DebugDraw.h"
#include "Magnum/BulletIntegration/MotionState.h"
#include <BulletDynamics/btBulletDynamicsCommon.h>

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

(Okay, I'm just being annoying here. Sorry.)

I would not separate the includes with empty lines. Just two groups -- one with <> and the second with "".

EDIT: I saw you fixed that in a later commit -- could you squash those two together?

@@ -43,7 +43,31 @@ namespace Magnum { namespace BulletIntegration {
Encapsulates `btMotionState` as @ref SceneGraph feature.
@todoc Usage...
# Usage

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

This should be ## Usage, otherwise the header gets way too big in Doxygen output.

// rigidBody will be affected when changing the transform of object and
// object will be affected when transformation of rigidBody is changed.
@endcode

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

Please don't indent stuff inside @code -- it will get indented also in the documentation, wasting precious horizontal space.

void MAGNUM_BULLETINTEGRATION_LOCAL getWorldTransform(btTransform& worldTrans) const override;
void MAGNUM_BULLETINTEGRATION_LOCAL setWorldTransform(const btTransform& worldTrans) override;
void MAGNUM_BULLETINTEGRATION_EXPORT getWorldTransform(btTransform& worldTrans) const override;
void MAGNUM_BULLETINTEGRATION_EXPORT setWorldTransform(const btTransform& worldTrans) override;

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

I saw the OSX CI fail on some linker error. I guess this is probably related to the above changes of the EXPORT/LOCAL macros. I think reverting those and defaulting the destructor inside the *.cpp file (instead of having it inline in the header) might help.

Overriden virtuals don't need to be exported (unless you are calling them directly, which you can't, because they are private).

.rotate(bRotation, Vector3(bAxis).normalized())
.translate(Vector3(bPosition));
auto parent = object().parent();
if(parent) {

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

This will be almost always true, no? Because even the scene is a parent.

@@ -26,27 +26,50 @@

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

There's some noise in the commit message ;)

const Rad angle = rotation.angle();
if(angle != 0.0_radf) {
/* If angle is 0, the axis would be a NaN vector */

This comment has been minimized.

@mosra
const Quaternion rotation = (parentRotation.inverted()*Quaternion{worldTrans.getRotation()})
.normalized(); /* Avoid floating point drift */
const Vector3 translation = Vector3{worldTrans.getOrigin()} - parentTransform.translation();

This comment has been minimized.

@mosra

mosra Oct 14, 2016

Owner

Hmm. All the inversions, sqrt() when converting the matrix to quaternion and renormalizations are quite costly operations and I'm not sure how this would perform with 100s of objects.

But on the other hand I also don't see any better way how to handle this if you really want to cover the general case where every object can have a different parent but you still want them all in a single physics world.

Things to consider:

  • there is transformation caching that could be maybe used for this purpose (docs) assuming that the parent transformation doesn't change that much (otherwise it would be just more overhead)
  • there's also a way to get absolute transformations of a bunch of objects at once (not just one) using SceneGraph::Object and that would also save some calculation time when more objects share the same parent. Does Bullet have some way to batch update transformations of more objects where this could be used? Unfortunately there's no way to get inverse absolute transformations (yet), I would need to add that.
@mosra

This comment has been minimized.

Owner

mosra commented May 25, 2017

I completely forgot that this has been a thing :) Thanks a lot, it now helped me when fixing the DLL linker errors on Windows. I cherry-picked most of the things in 3a44040, bff3f7c, ba5b1a7, d6941bf, c45d5a2 and a971a88 and thanks to the new test the code coverage is now doubled :)

Just for the record, the "Fix local transforms from bullet world transforms" commit is not merged as we finally agreed that this was too much performance impact for too little benefit. Just aside: isn't this behavior practically the same in UE4? IIRC they also just disconnect a physics object from all the parents when it starts to be simulated.

@mosra mosra closed this May 25, 2017

@mosra mosra referenced this pull request May 25, 2017

Closed

BulletIntegration: Usage #19

0 of 2 tasks complete
@Squareys

This comment has been minimized.

Contributor

Squareys commented May 25, 2017

behavior practically the same in UE4?

While, yes, it is disconnected, you can reconnect it to have it act in weird unexpected ways (simulated, but also getting relative transforms of the parent applied to it, but I think also depends on update order, but not sure...).

As long as the documentation is clear about it, I'm fine with the parent of the object being ignored :)

@mosra mosra moved this from TODO to Done in Physics Jan 23, 2018

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment