- Added all_modules_ to hold pointers to ALL allocated modules.

- Add a private set_uninitialized() method for resetting the
  initialized_ flag and I also clear the contents of bmodules_ and
  cmodule_. initialize() will repopulate them.
- Fixing a memory leak in remove() - previously, when cmodule_ was
  being removed, it was not deallocated.
- Attempted to simplify the DecimaterT<Mesh>::initialize method.
- Added a "FIXME" comment critical of special treatment of quadric module.
- Replaced -1 with proper ILLEGAL_COLLAPSE enum constant in
  collapse_priority.

Patch submitted by Ilya A. Kriveshko

git-svn-id: http://www.openmesh.org/svnrepo/OpenMesh/trunk@129 fdac6126-5c0c-442c-9429-916003d36597

src/OpenMesh/Tools/Decimater/DecimaterT.cc

@@ -103,14 +103,13 @@ DecimaterT<Mesh>::
   mesh_.remove_property(priority_);
   mesh_.remove_property(heap_position_);
 
-  // dispose modules
+  // dispose of modules
   {
-    typename ModuleList::iterator m_it, m_end = bmodules_.end();
+    set_uninitialized();
-    for( m_it=bmodules_.begin(); m_it!=m_end; ++m_it)
+    typename ModuleList::iterator m_it, m_end = all_modules_.end();
+    for( m_it=all_modules_.begin(); m_it!=m_end; ++m_it)
       delete *m_it;
-    bmodules_.clear();
+    all_modules_.clear();
-    if (cmodule_)
-      delete cmodule_;
   }
 }
 
@@ -123,15 +122,33 @@ void
 DecimaterT<Mesh>::
 info( std::ostream& _os )
 {
-  typename ModuleList::iterator m_it, m_end = bmodules_.end();
+  if(initialized_)
-
+  {
-  _os << "binary modules: " << bmodules_.size() << std::endl;
+    _os << "initialized : yes" << std::endl;
-  for( m_it=bmodules_.begin(); m_it!=m_end; ++m_it)
+    _os << "binary modules: " << bmodules_.size() << std::endl;
-    _os << "  " << (*m_it)->name() << std::endl;
+    for( ModuleListIterator m_it=bmodules_.begin(); m_it!=bmodules_.end(); ++m_it)
-
+    {
-  _os << "priority module: "
+      _os << "  " << (*m_it)->name() << std::endl;
-      << (cmodule_ ? cmodule_->name().c_str() : "<None>") << std::endl;
+    }
-  _os << "is initialized : " << (initialized_ ? "yes" : "no") << std::endl;
+    _os << "priority module: " << cmodule_->name().c_str() << std::endl;
+  }
+  else {
+    _os << "initialized : no" << std::endl;
+    _os << "available modules: " << all_modules_.size() << std::endl;
+    for( ModuleListIterator m_it=all_modules_.begin(); m_it!=all_modules_.end(); ++m_it)
+    {
+      _os << "  " << (*m_it)->name() << " : ";
+      if((*m_it)->is_binary()) {
+        _os << "binary";
+        if((*m_it)->name() == "Quadric") {
+          _os << " and priority (special treatment)";
+        }
+      } else {
+        _os << "priority";
+      }
+      _os << std::endl;
+    }
+  }
 }
 
 
@@ -143,56 +160,64 @@ bool
 DecimaterT<Mesh>::
 initialize()
 {
-  typename ModuleList::iterator m_it, m_end = bmodules_.end();
+  if(initialized_)
+  {
+    return true;
+  }
 
+  // FIXME: quadric module shouldn't be treated specially.
+  // Q: Why?
+  // A: It isn't generic and breaks encapsulation. Also, using string
+  // name comparison is not reliable, since you can't guarantee that
+  // no one else will name their custom module "Quadric".
+  // Q: What should be done instead?
+  // A: ModBaseT API should support modules that can be both binary
+  // and priority, or BETTER YET, let the DecimaterT API specify the
+  // priority module explicitly.
+
+  // find the priority module: either the only non-binary module in the list, or "Quadric"
   Module *quadric=NULL;
-
+  Module *pmodule=NULL;
-  Module* origC = NULL;
+  for (ModuleListIterator m_it=all_modules_.begin(), m_end=all_modules_.end(); m_it != m_end; ++m_it)
-  
-  // If already initialized, remember original cModule (priority module)
-  // if no new cmodule is provided use old one
-  if (initialized_)
-    origC = cmodule_;
-  
-  cmodule_ = NULL;
-  
-  for (m_it=bmodules_.begin(); m_it != m_end; ++m_it)
   {
     if ( (*m_it)->name() == "Quadric")
       quadric = *m_it;
 
-    if ( ! (*m_it)->is_binary() )
+    if ( !(*m_it)->is_binary() )
     {
-      if ( !cmodule_ ) // only one non-binary module allowed!
+      if(pmodule)
-        cmodule_ = *m_it;
+      {
-      else
+        // only one priority module allowed!
+        set_uninitialized();
         return false;
+      }
+      pmodule = *m_it;
     }
-    (*m_it)->initialize();
+  }
+
+  // Quadric is used as default priority module (even if it is set to be binary)
+  if(!pmodule && quadric) {
+    pmodule = quadric;
   }
   
-  // If the decimater has already been initialized and we have no new cmodule, 
+  if(!pmodule) {
-  // use the old cmodule
+    // At least one priority module required
-  if ( initialized_ && !cmodule_ ) {
+    set_uninitialized();
-    cmodule_ = origC;
+    return false;
-    cmodule_->initialize(); 
   }
 
-  if (!cmodule_) // one non-binary module is mandatory!
+  // set pmodule as the current priority module
+  cmodule_ = pmodule;
+
+  for(ModuleListIterator m_it=all_modules_.begin(), m_end=all_modules_.end(); m_it != m_end; ++m_it)
   {
-    if (!quadric)
+    // every module gets initialized
-      return false;
+    (*m_it)->initialize();
-    else
-    {
-      cmodule_ = quadric; // let the quadric become the priority module
-    }
-  }
 
-  // If we do not reuse the original cmodule delete the new cmodule from the 
+    if(*m_it != pmodule) {
-  // binary module list
+      // all other modules are binary, and go into bmodules_ list
-  if ( !initialized_ || (cmodule_ != origC) ) {
+      bmodules_.push_back(*m_it);
-    m_it = std::find(bmodules_.begin(), bmodules_.end(), cmodule_ );
+    }
-    bmodules_.erase( m_it );
   }
 
   return initialized_ = true;
@@ -303,7 +328,7 @@ DecimaterT<Mesh>::collapse_priority(const CollapseInfo& _ci)
   for (m_it = bmodules_.begin(); m_it != m_end; ++m_it)
   {
     if ( (*m_it)->collapse_priority(_ci) < 0.0)
-      return -1.0; // ILLEGAL_COLLAPSE
+      return ModBaseT<DecimaterT<Mesh> >::ILLEGAL_COLLAPSE;
   }
   return cmodule_->collapse_priority(_ci);
 }

src/OpenMesh/Tools/Decimater/DecimaterT.hh

@@ -73,6 +73,7 @@ public: //-------------------------------------------------------- public types
   typedef CollapseInfoT<MeshT>       CollapseInfo;
   typedef ModBaseT<Self>             Module;
   typedef std::vector< Module* >     ModuleList;
+  typedef typename ModuleList::iterator ModuleListIterator;
 
 public: //------------------------------------------------------ public methods
 
@@ -113,9 +114,10 @@ public: //--------------------------------------------------- module management
       return false;
 
     _mh.init( new _Module(*this) );
-    bmodules_.push_back( _mh.module() );
+    all_modules_.push_back( _mh.module() );
 
-    initialized_ = false;
+    set_uninitialized();
+    
     return true;
   }
 
@@ -127,25 +129,18 @@ public: //--------------------------------------------------- module management
     if (!_mh.is_valid())
       return false;
 
-    if ( cmodule_ == _mh.module() ) {
+    typename ModuleList::iterator it = std::find(all_modules_.begin(),
-      cmodule_     = 0;
+                                                 all_modules_.end(),
-      initialized_ = false; // reset initialized state
+                                                 _mh.module() );
-      _mh.clear();
-      return true;
-    }
 
-    typename ModuleList::iterator it = std::find(bmodules_.begin(),
+    if ( it == all_modules_.end() ) // module not found
-             bmodules_.end(),
-             _mh.module() );
-
-    if ( it == bmodules_.end() ) // module not found
       return false;
 
     delete *it;
-    bmodules_.erase( it ); // finally remove from list
+    all_modules_.erase( it ); // finally remove from list
     _mh.clear();
 
-    initialized_ = false; // reset initialized state
+    set_uninitialized();
     return true;
   }
 
@@ -242,7 +237,12 @@ private: //---------------------------------------------------- private methods
   /// Post-process a collapse
   void postprocess_collapse(CollapseInfo& _ci);
 
-
+  // Reset the initialized flag, and clear the bmodules_ and cmodule_
+  void set_uninitialized() {
+    initialized_ = false;
+    cmodule_ = 0;
+    bmodules_.clear();
+  }
 
 
 private: //------------------------------------------------------- private data
@@ -254,10 +254,15 @@ private: //------------------------------------------------------- private data
   // heap
   std::auto_ptr<DeciHeap> heap_;
 
-  // list of modules
+  // list of binary modules
   ModuleList bmodules_;
+
+  // the current priority module
   Module*    cmodule_;
 
+  // list of all allocated modules (including cmodule_ and all of bmodules_)
+  ModuleList all_modules_;
+    
   bool       initialized_;