char: attempt to make Character class thread-safe

panda/src/chan/partBundleNode.I

@@ -46,6 +46,7 @@ PartBundleNode(const PartBundleNode &copy) :
  */
 INLINE int PartBundleNode::
 get_num_bundles() const {
+  LightMutexHolder holder(_lock);
   return _bundles.size();
 }
 
@@ -54,6 +55,7 @@ get_num_bundles() const {
  */
 INLINE PartBundle *PartBundleNode::
 get_bundle(int n) const {
+  LightMutexHolder holder(_lock);
   nassertr(n >= 0 && n < (int)_bundles.size(), nullptr);
   return _bundles[n]->get_bundle();
 }
@@ -65,6 +67,7 @@ get_bundle(int n) const {
  */
 INLINE PartBundleHandle *PartBundleNode::
 get_bundle_handle(int n) const {
+  LightMutexHolder holder(_lock);
   nassertr(n >= 0 && n < (int)_bundles.size(), nullptr);
   return _bundles[n];
 }

panda/src/chan/partBundleNode.cxx

@@ -25,9 +25,9 @@ TypeHandle PartBundleNode::_type_handle;
  */
 PartBundleNode::
 ~PartBundleNode() {
-  Bundles::iterator bi;
+  LightMutexHolder holder(_lock);
-  for (bi = _bundles.begin(); bi != _bundles.end(); ++bi) {
+  for (PartBundleHandle *handle : _bundles) {
-    (*bi)->get_bundle()->remove_node(this);
+    handle->get_bundle()->remove_node(this);
   }
 }
 
@@ -44,9 +44,8 @@ void PartBundleNode::
 apply_attribs_to_vertices(const AccumulatedAttribs &attribs, int attrib_types,
                           GeomTransformer &transformer) {
   if ((attrib_types & SceneGraphReducer::TT_transform) != 0) {
-    Bundles::iterator bi;
+    LightMutexHolder holder(_lock);
-    for (bi = _bundles.begin(); bi != _bundles.end(); ++bi) {
+    for (PT(PartBundleHandle) handle : _bundles) {
-      PT(PartBundleHandle) handle = (*bi);
       PartBundle *bundle = handle->get_bundle();
       PT(PartBundle) new_bundle = bundle->apply_transform(attribs._transform);
       update_bundle(handle, new_bundle);
@@ -71,9 +70,8 @@ xform(const LMatrix4 &mat) {
     return;
   }
 
-  Bundles::iterator bi;
+  LightMutexHolder holder(_lock);
-  for (bi = _bundles.begin(); bi != _bundles.end(); ++bi) {
+  for (PT(PartBundleHandle) handle : _bundles) {
-    PT(PartBundleHandle) handle = (*bi);
     PartBundle *bundle = handle->get_bundle();
     if (bundle->get_num_nodes() > 1) {
       PT(PartBundle) new_bundle = DCAST(PartBundle, bundle->copy_subgraph());
@@ -90,14 +88,15 @@ xform(const LMatrix4 &mat) {
 void PartBundleNode::
 add_bundle(PartBundle *bundle) {
   PT(PartBundleHandle) handle = new PartBundleHandle(bundle);
-  add_bundle_handle(handle);
+  LightMutexHolder holder(_lock);
+  do_add_bundle_handle(handle);
 }
 
 /**
- *
+ * Assumes the lock is held.
  */
 void PartBundleNode::
-add_bundle_handle(PartBundleHandle *handle) {
+do_add_bundle_handle(PartBundleHandle *handle) {
   Bundles::iterator bi = find(_bundles.begin(), _bundles.end(), handle);
   if (bi != _bundles.end()) {
     // This handle is already within the node.
@@ -117,11 +116,15 @@ steal_bundles(PartBundleNode *other) {
     return;
   }
 
-  Bundles::iterator bi;
+  LightMutexHolder other_holder(other->_lock);
-  for (bi = other->_bundles.begin(); bi != other->_bundles.end(); ++bi) {
+  if (other->_bundles.empty()) {
-    PartBundleHandle *handle = (*bi);
+    return;
+  }
+
+  LightMutexHolder holder(_lock);
+  for (PartBundleHandle *handle : other->_bundles) {
     handle->get_bundle()->remove_node(other);
-    add_bundle_handle(handle);
+    do_add_bundle_handle(handle);
   }
   other->_bundles.clear();
 }
@@ -129,6 +132,8 @@ steal_bundles(PartBundleNode *other) {
 /**
  * Replaces the contents of the indicated PartBundleHandle (presumably stored
  * within this node) with new_bundle.
+ *
+ * Assumes the lock is held.
  */
 void PartBundleNode::
 update_bundle(PartBundleHandle *old_bundle_handle, PartBundle *new_bundle) {
@@ -146,10 +151,10 @@ void PartBundleNode::
 write_datagram(BamWriter *manager, Datagram &dg) {
   PandaNode::write_datagram(manager, dg);
 
+  LightMutexHolder holder(_lock);
   dg.add_uint16(_bundles.size());
-  Bundles::iterator bi;
+  for (PartBundleHandle *handle : _bundles) {
-  for (bi = _bundles.begin(); bi != _bundles.end(); ++bi) {
+    manager->write_pointer(dg, handle->get_bundle());
-    manager->write_pointer(dg, (*bi)->get_bundle());
   }
 }
 
@@ -161,11 +166,11 @@ int PartBundleNode::
 complete_pointers(TypedWritable **p_list, BamReader* manager) {
   int pi = PandaNode::complete_pointers(p_list, manager);
 
-  Bundles::iterator bi;
+  LightMutexHolder holder(_lock);
-  for (bi = _bundles.begin(); bi != _bundles.end(); ++bi) {
+  for (PT(PartBundleHandle) &handle : _bundles) {
     PT(PartBundle) bundle = DCAST(PartBundle, p_list[pi++]);
     bundle->add_node(this);
-    (*bi) = new PartBundleHandle(bundle);
+    handle = new PartBundleHandle(bundle);
   }
 
   return pi;

panda/src/chan/partBundleNode.h

@@ -18,7 +18,7 @@
 
 #include "partBundle.h"
 #include "partBundleHandle.h"
-
+#include "lightMutex.h"
 #include "pandaNode.h"
 #include "dcast.h"
 #include "pvector.h"
@@ -59,12 +59,13 @@ PUBLISHED:
 
 protected:
   void add_bundle(PartBundle *bundle);
-  void add_bundle_handle(PartBundleHandle *handle);
+  void do_add_bundle_handle(PartBundleHandle *handle);
   void steal_bundles(PartBundleNode *other);
   virtual void update_bundle(PartBundleHandle *old_bundle_handle,
                              PartBundle *new_bundle);
 
 protected:
+  LightMutex _lock;
   typedef pvector< PT(PartBundleHandle) > Bundles;
   Bundles _bundles;
 

panda/src/char/character.cxx

@@ -44,29 +44,29 @@ Character(const Character &copy, bool copy_bundles) :
   _lod_delay_factor(copy._lod_delay_factor),
   _do_lod_animation(copy._do_lod_animation),
   _joints_pcollector(copy._joints_pcollector),
-  _skinning_pcollector(copy._skinning_pcollector)
+  _skinning_pcollector(copy._skinning_pcollector),
+  _last_auto_update(-1.0),
+  _view_frame(-1),
+  _view_distance2(0.0f)
 {
   set_cull_callback();
 
+  LightMutexHolder holder(copy._lock);
+
   if (copy_bundles) {
     // Copy the bundle(s).
-    int num_bundles = copy.get_num_bundles();
+    for (PartBundleHandle *handle : copy._bundles) {
-    for (int i = 0; i < num_bundles; ++i) {
+      PartBundle *orig_bundle = handle->get_bundle();
-      PartBundle *orig_bundle = copy.get_bundle(i);
       PT(PartBundle) new_bundle = DCAST(PartBundle, orig_bundle->copy_subgraph());
-      add_bundle(new_bundle);
+      do_add_bundle_handle(new PartBundleHandle(new_bundle));
     }
   } else {
     // Share the bundles.
-    int num_bundles = copy.get_num_bundles();
+    for (PartBundleHandle *handle : copy._bundles) {
-    for (int i = 0; i < num_bundles; ++i) {
+      PartBundle *orig_bundle = handle->get_bundle();
-      PartBundle *orig_bundle = copy.get_bundle(i);
+      do_add_bundle_handle(new PartBundleHandle(orig_bundle));
-      add_bundle(orig_bundle);
     }
   }
-  _last_auto_update = -1.0;
-  _view_frame = -1;
-  _view_distance2 = 0.0f;
 }
 
 /**
@@ -76,13 +76,13 @@ Character::
 Character(const std::string &name) :
   PartBundleNode(name, new CharacterJointBundle(name)),
   _joints_pcollector(PStatCollector(_animation_pcollector, name), "Joints"),
-  _skinning_pcollector(PStatCollector(_animation_pcollector, name), "Vertices")
+  _skinning_pcollector(PStatCollector(_animation_pcollector, name), "Vertices"),
+  _last_auto_update(-1.0),
+  _view_frame(-1),
+  _view_distance2(0.0f)
 {
   set_cull_callback();
   clear_lod_animation();
-  _last_auto_update = -1.0;
-  _view_frame = -1;
-  _view_distance2 = 0.0f;
 }
 
 /**
@@ -90,9 +90,9 @@ Character(const std::string &name) :
  */
 Character::
 ~Character() {
-  int num_bundles = get_num_bundles();
+  LightMutexHolder holder(_lock);
-  for (int i = 0; i < num_bundles; ++i) {
+  for (PartBundleHandle *handle : _bundles) {
-    r_clear_joint_characters(get_bundle(i));
+    r_clear_joint_characters(handle->get_bundle());
   }
 }
 
@@ -263,6 +263,7 @@ merge_bundles(PartBundleHandle *old_bundle_handle,
   PartBundle *new_bundle = new_bundle_handle->get_bundle();
   new_bundle->merge_anim_preloads(old_bundle);
 
+  LightMutexHolder holder(_lock);
   update_bundle(old_bundle_handle, new_bundle);
 }
 
@@ -326,11 +327,11 @@ clear_lod_animation() {
  */
 CharacterJoint *Character::
 find_joint(const std::string &name) const {
-  int num_bundles = get_num_bundles();
+  LightMutexHolder holder(_lock);
-  for (int i = 0; i < num_bundles; ++i) {
+  for (PartBundleHandle *handle : _bundles) {
-    PartGroup *part = get_bundle(i)->find_child(name);
+    PartGroup *part = handle->get_bundle()->find_child(name);
     if (part != nullptr && part->is_character_joint()) {
-      return DCAST(CharacterJoint, part);
+      return (CharacterJoint *)part;
     }
   }
 
@@ -344,12 +345,12 @@ find_joint(const std::string &name) const {
  */
 CharacterSlider *Character::
 find_slider(const std::string &name) const {
-  int num_bundles = get_num_bundles();
+  LightMutexHolder holder(_lock);
-  for (int i = 0; i < num_bundles; ++i) {
+  for (PartBundleHandle *handle : _bundles) {
-    PartGroup *part = get_bundle(i)->find_child(name);
+    PartGroup *part = handle->get_bundle()->find_child(name);
     if (part != nullptr &&
         part->is_of_type(CharacterSlider::get_class_type())) {
-      return DCAST(CharacterSlider, part);
+      return (CharacterSlider *)part;
     }
   }
 
@@ -362,9 +363,9 @@ find_slider(const std::string &name) const {
  */
 void Character::
 write_parts(std::ostream &out) const {
-  int num_bundles = get_num_bundles();
+  LightMutexHolder holder(_lock);
-  for (int i = 0; i < num_bundles; ++i) {
+  for (PartBundleHandle *handle : _bundles) {
-    get_bundle(i)->write(out, 0);
+    handle->get_bundle()->write(out, 0);
   }
 }
 
@@ -375,9 +376,9 @@ write_parts(std::ostream &out) const {
  */
 void Character::
 write_part_values(std::ostream &out) const {
-  int num_bundles = get_num_bundles();
+  LightMutexHolder holder(_lock);
-  for (int i = 0; i < num_bundles; ++i) {
+  for (PartBundleHandle *handle : _bundles) {
-    get_bundle(i)->write_with_value(out, 0);
+    handle->get_bundle()->write_with_value(out, 0);
   }
 }
 
@@ -401,6 +402,7 @@ update_to_now() {
  */
 void Character::
 update() {
+  LightMutexHolder holder(_lock);
   double now = ClockObject::get_global_clock()->get_frame_time();
   if (now != _last_auto_update) {
     _last_auto_update = now;
@@ -421,13 +423,14 @@ update() {
  */
 void Character::
 force_update() {
+  LightMutexHolder holder(_lock);
+
   // Statistics
   PStatTimer timer(_joints_pcollector);
 
   // Update all the joints and sliders.
-  int num_bundles = get_num_bundles();
+  for (PartBundleHandle *handle : _bundles) {
-  for (int i = 0; i < num_bundles; ++i) {
+    handle->get_bundle()->force_update();
-    get_bundle(i)->force_update();
   }
 }
 
@@ -456,11 +459,13 @@ r_copy_children(const PandaNode *from, PandaNode::InstanceMap &inst_map,
   NodeMap node_map;
   JointMap joint_map;
 
-  int num_bundles = get_num_bundles();
+  LightMutexHolder from_holder(from_char->_lock);
-  nassertv(from_char->get_num_bundles() == num_bundles);
+  LightMutexHolder holder(_lock);
-  int i;
+
-  for (i = 0; i < num_bundles; ++i) {
+  size_t num_bundles = _bundles.size();
-    fill_joint_map(joint_map, get_bundle(i), from_char->get_bundle(i));
+  nassertv(from_char->_bundles.size() == num_bundles);
+  for (size_t i = 0; i < num_bundles; ++i) {
+    fill_joint_map(joint_map, _bundles[i]->get_bundle(), from_char->_bundles[i]->get_bundle());
   }
 
   GeomVertexMap gvmap;
@@ -469,14 +474,16 @@ r_copy_children(const PandaNode *from, PandaNode::InstanceMap &inst_map,
   r_copy_char(this, from_char, from_char, node_map, joint_map,
               gvmap, gjmap, gsmap);
 
-  for (i = 0; i < num_bundles; ++i) {
+  for (size_t i = 0; i < num_bundles; ++i) {
-    copy_node_pointers(node_map, get_bundle(i), from_char->get_bundle(i));
+    copy_node_pointers(node_map, _bundles[i]->get_bundle(), from_char->_bundles[i]->get_bundle());
   }
 }
 
 /**
  * Replaces the contents of the indicated PartBundleHandle (presumably stored
  * within this node) with new_bundle.
+ *
+ * Assumes the lock is held.
  */
 void Character::
 update_bundle(PartBundleHandle *old_bundle_handle, PartBundle *new_bundle) {
@@ -530,20 +537,18 @@ get_rel_transform(CullTraverser *trav, CullTraverserData &data) {
 
 /**
  * The actual implementation of update().  Assumes the appropriate
- * PStatCollector has already been started.
+ * PStatCollector has already been started, and that the lock is held.
  */
 void Character::
 do_update() {
   // Update all the joints and sliders.
   if (even_animation) {
-    int num_bundles = get_num_bundles();
+    for (PartBundleHandle *handle : _bundles) {
-    for (int i = 0; i < num_bundles; ++i) {
+      handle->get_bundle()->force_update();
-      get_bundle(i)->force_update();
     }
   } else {
-    int num_bundles = get_num_bundles();
+    for (PartBundleHandle *handle : _bundles) {
-    for (int i = 0; i < num_bundles; ++i) {
+      handle->get_bundle()->update();
-      get_bundle(i)->update();
     }
   }
 }
@@ -554,9 +559,9 @@ do_update() {
  */
 void Character::
 set_lod_current_delay(double delay) {
-  int num_bundles = get_num_bundles();
+  LightMutexHolder holder(_lock);
-  for (int i = 0; i < num_bundles; ++i) {
+  for (PartBundleHandle *handle : _bundles) {
-    get_bundle(i)->set_update_delay(delay);
+    handle->get_bundle()->set_update_delay(delay);
   }
 }
 
@@ -564,6 +569,8 @@ set_lod_current_delay(double delay) {
  * After the joint hierarchy has already been copied from the indicated
  * hierarchy, this recursively walks through the joints and builds up a
  * mapping from old to new.
+ *
+ * Assumes the lock is held.
  */
 void Character::
 fill_joint_map(Character::JointMap &joint_map,
@@ -594,6 +601,8 @@ fill_joint_map(Character::JointMap &joint_map,
  * Recursively checks the two bundles for a matching hierarchy, and adds nodes
  * as necessary to "new_group" where they are not already present.  Also fills
  * joint_map in the same manner as fill_joint_map().
+ *
+ * Assumes the lock is held.
  */
 void Character::
 r_merge_bundles(Character::JointMap &joint_map,
@@ -700,11 +709,12 @@ r_merge_bundles(Character::JointMap &joint_map,
   new_group->_children.swap(new_children);
 }
 
-
 /**
  * Recursively walks the scene graph hierarchy below the Character node,
  * duplicating it while noting the orig:copy node mappings, and also updates
  * any GeomNodes found.
+ *
+ * Assumes the lock is held.
  */
 void Character::
 r_copy_char(PandaNode *dest, const PandaNode *source,