Using NGD multithreading support

I am trying to get Newton multi-threading work to with OgreNewt and as much as I have learned about the subject, the problem is the default transform callback, because multiple threads will try to set transformation of the same object, causing a crash (same problem in newton forum).

Default transformcallback implementation:
void Body::standardTransformCallback( OgreNewt::Body* me, const Ogre::Quaternion& orient, const Ogre::Vector3& pos, int threadIndex )
{
me->m_node->setOrientation( orient );
me->m_node->setPosition( pos );
}

So I tried making a transformation request in the callback and applying the transformations in my main loop, but the crash persists when using world->setThreadCount(2);

My extensions/changes to OgreNewt are following:

OgreNewt_TransformRequest.h
/*
OgreNewt Library

Ogre implementation of Newton Game Dynamics SDK

OgreNewt basically has no license, you may use any or all of the library however you desire... I hope it can help you in any way.


please note that the "boost" library files included here are not of my creation, refer to those files and boost.org for information.


by Walaber
some changes by melven
this class by kallaspriit

*/

#ifndef _INCLUDE_OGRENEWT_TRANSFORMREQUEST
#define _INCLUDE_OGRENEWT_TRANSFORMREQUEST

#include "OgreNewt_Body.h"
#include <OgreVector3.h>
#include <OgreQuaternion.h>

// OgreNewt namespace. all functions and classes use this namespace.
namespace OgreNewt
{

/*
CLASS DEFINITION:

TransformRequest

USE:
this class represents a transformation of a ogrenewt body. This
is neccesary for using Newton in multiple threads. The requests should be
accumulated and applied in the main loop.
*/
//! OgreNewt body transformation request
class TransformRequest
{
public:
//! constructor.
/*!
creates the request object for given body with given transformations
\param body pointer to transformed body
\param position new position
\param orientation new orientation
*/
TransformRequest(OgreNewt::Body* body, const Ogre::Vector3& position, const Ogre::Quaternion& orientation);

//! Applies the transformations
void apply();

OgreNewt::Body* getBody() { return m_body; }
Ogre::Vector3 getPosition() { return m_position; }
Ogre::Quaternion getOrientation() { return m_orientation; }

protected:
//! The body
OgreNewt::Body* m_body;

//! New position
Ogre::Vector3 m_position;

//! New orientation
Ogre::Quaternion m_orientation;

};

}

#endif
// _INCLUDE_OGRENEWT_TRANSFORMREQUEST


OgreNewt_TransformRequest.cpp
#include "OgreNewt_TransformRequest.h"

namespace OgreNewt
{

TransformRequest::TransformRequest(OgreNewt::Body* body, const Ogre::Vector3& position, const Ogre::Quaternion& orientation)
{
m_body = body;
m_position = position;
m_orientation = orientation;
}

void TransformRequest::apply()
{
if(m_body != NULL)
{
m_body->getOgreNode()->setOrientation(m_orientation);
m_body->getOgreNode()->setPosition(m_position);
}
}

}


OgreNewt_TransformAccumulator.h
/*
OgreNewt Library

Ogre implementation of Newton Game Dynamics SDK

OgreNewt basically has no license, you may use any or all of the library however you desire... I hope it can help you in any way.


please note that the "boost" library files included here are not of my creation, refer to those files and boost.org for information.


by Walaber
some changes by melven
this class by kallaspriit

*/

#ifndef _INCLUDE_OGRENEWT_TRANSFORMACCUMULATOR
#define _INCLUDE_OGRENEWT_TRANSFORMACCUMULATOR

#include "OgreNewt_TransformRequest.h"
#include "OgreNewt_Body.h"
#include <map>

// OgreNewt namespace. all functions and classes use this namespace.
namespace OgreNewt
{

/*
CLASS DEFINITION:

TransformAccumulator

USE:
this class accumulates OgreNewt::TransformRequest objects and enables
to apply them in main loop
*/
//! OgreNewt transform accumulator
class TransformAccumulator
{
public:
//! Registers new request
/*!
Registers new transform request. These will be accumulated and
applied by calling applyAll().
\param request The request
*/
void add(TransformRequest request);

//! Applies all accumulated transformations
void applyAll();

protected:
//! Map of transform requests
/*!
A map is used so there wouldnt be several requests for a
single body.
*/
std::map<Body*, TransformRequest> requests;

};

}

#endif
// _INCLUDE_TRANSFORMACCUMULATOR


OgreNewt_TransformAccumulator.cpp
#include "OgreNewt_TransformAccumulator.h"
#include "OgreNewt_World.h"

namespace OgreNewt
{

void TransformAccumulator::add(TransformRequest request)
{
std::map<Body*, TransformRequest>::iterator search = requests.find(request.getBody());

if(search != requests.end())
{
requests.erase(search);
}

requests.insert(std::make_pair(request.getBody(), request));
}

void TransformAccumulator::applyAll()
{
std::map<Body*, TransformRequest>::iterator iter;

for(iter = requests.begin(); iter != requests.end(); iter++)
{
iter->second.apply();
}
}

}


The TransformAccumulator belongs to the OgreNewt World, the new transformcallback is as follows:
void Body::standardTransformCallback( OgreNewt::Body* me, const Ogre::Quaternion& orient, const Ogre::Vector3& pos, int threadIndex )
{
TransformRequest request(me, pos, orient);
me->getWorld()->getTransformAccumulator().add(request);
}

And finally, in the main loop or actually in the OgreNewt::World::update(), I call applyAll() on the accumulator:
void World::update( Ogre::Real t_step )
{
NewtonUpdate( m_world, (float)t_step );

m_transformAccumulator->applyAll();
}

Hoped this would work but what am I missing or doing wrong?

Concerning your code:
Looks nice, but what about thread safety of std::map (I'm not shure if this could even depend on the implementation of the stl library...) ?
I don't think that writing to a container without locking it is thread safe: See http://www.sgi.com/tech/stl/thread_safety.html (refers also to the gnu stl).

To solve this problem I see several possibilities (I'm not very familiar with multi-threading programming, so this are only suggestions!)
- using the current implementation (applies the transformations directly without accumulating them) with a locking mechanism (so only one thread at a time performs a transformation), but there are probably performance issues: if one thread currently applies a transformation all other threads have to wait...
- using your code and locking the write access to the std::map : the result shouldn't really differ from the solution above, but writing to a map could be faster, so the other threads don't need to wait too long (I would suggest to use a simple list if there are not multiple transformations for one body (we could ask this in the newton forum))
- using several "accumuluators", one for each thread (using the threadIndex parameter of the transformCallback function). You would still need to check (or ask
in the newton forum) if you can get more than one transformation per body... I think this would be the nicest solution...



Additionally you need to be careful with multithreading in OgreNewt, the whole code is not tested in multithreading environments, so there are probably several other problems in OgreNewt... And you need to check, when you should use the criticalSectionLock-functions...

You are of course correct about std::map not being thread safe and your third proposal is very good. I implemented the multiple accumulators approach, there were some problems like sharing the transformation between the accumulators which meant that if the accumulator with the older transformation got applied later (which was often the case), the transformation lagged behind until the wrong accumulator got updated again. Got pass this by removing all the applied transformations, this also means that it was no longer necessary to check for existing transformations in the add method.

After these changes everything seemed to work for some time, but I still kept getting rare crashes and was almost giving up when I remembered that this was not the only callback I was using.. disabled my material callback for sound and haven't seen a crash for some time so I think it actually works

TransformRequest, I believe, did not change, this is the new implementation of the accumulator:

OgreNewt_TransformAccumulator.cpp
#include "OgreNewt_TransformAccumulator.h"
#include "OgreNewt_World.h"

namespace OgreNewt
{

void TransformAccumulator::add(TransformRequest request)
{
requests.insert(std::make_pair(request.getBody(), request));
}

void TransformAccumulator::applyAll()
{
while(!requests.empty())
{
requests.begin()->second.apply();
requests.erase(requests.begin());
}
}

}


New OgreNewt::Body::standardTransformCallback()
void Body::standardTransformCallback( OgreNewt::Body* me, const Ogre::Quaternion& orient, const Ogre::Vector3& pos, int threadIndex )
{
TransformRequest request(me, pos, orient);
me->getWorld()->getTransformAccumulator(threadIndex).add(request);
}

In World header, there is a new member:
std::map<int, TransformAccumulator*> m_transformAccumulators;

I moved the OgreNewt::World::setThreadCount() implementation from the header to the source file with new code:
void World::setThreadCount(int threads)
{
NewtonSetThreadsCount( m_world, threads );

while(!m_transformAccumulators.empty())
{
delete m_transformAccumulators.begin()->second;
m_transformAccumulators.erase(m_transformAccumulators.begin());
}

for(int threadIndex = 0; threadIndex < threads; threadIndex++)
{
TransformAccumulator* accumulator = new TransformAccumulator();
m_transformAccumulators.insert(std::make_pair(threadIndex, accumulator));
}
}

This pre-creates the accumulators for as many threads as requested.

OgreNewt::World has a new method:
//! get the TransformAccumulator for this world
TransformAccumulator& getTransformAccumulator(int threadIndex) const;
with implementation:
TransformAccumulator& World::getTransformAccumulator(int threadIndex) const
{
std::map<int, TransformAccumulator*>::const_iterator search = m_transformAccumulators.find(threadIndex);

TransformAccumulator* accumulator = NULL;

if(search != m_transformAccumulators.end())
{
accumulator = search->second;
}

assert(accumulator != NULL);

return *accumulator;
}

The OgreNewt::World::~World() was modified to destroy the accumulators:
World::~World()
{
while(!m_transformAccumulators.empty())
{
delete m_transformAccumulators.begin()->second;
m_transformAccumulators.erase(m_transformAccumulators.begin());
}

if (m_debugger)
{
delete m_debugger;
m_debugger = NULL;
}

if (m_defaultMatID)
{
delete m_defaultMatID;
m_defaultMatID = NULL;
}

if (m_world)
{
NewtonDestroy( m_world );
m_world = NULL;
}
}

Added applying the transformations directly to OgreNewt::World::update() method:
void World::update( Ogre::Real t_step )
{
NewtonUpdate( m_world, (float)t_step );

// Apply the transformations
std::map<int, TransformAccumulator*>::iterator it;

for(it = m_transformAccumulators.begin(); it != m_transformAccumulators.end(); it++)
{
it->second->applyAll();
}
}

Is this the best place for it or should it be moved to a standalone method and perhaps expose the accumulators map so the user could implement their own transforming, perhaps selectively ignoring some and so on?

Anyway, this should be about it, could anyone try this to confirm it works (be careful with other callbacks modifying common resources)

What I do is simply store the latest body state in the body itself. Then, my world simply iterates over all the movable bodies, reads this state, and applies it to the graphical node. Seems to work for me (no Newton related crashes at all), and this way you don't need external transformation tracking.

Very good and simple idea, implemented this too and works like a charm without too much extra complexity. Very simple to do, but if someone cares for it, I can post my code

What I do is simply store the latest body state in the body itself. Then, my world simply iterates over all the movable bodies, reads this state, and applies it to the graphical node. Seems to work for me (no Newton related crashes at all), and this way you don't need external transformation tracking.

What about thread safety of the position/orientation stored in the OgreNewt::Body?

Can we hope writing a vector and quaternion being atomic or actually you might be right it not being so.. did not seem to crash for some time while testing, but I dont know..