ENH: ELASTIX-9 Only access connections via component indexes; BUG: ELASTIX-9...

ENH: ELASTIX-9 Only access connections via component indexes; BUG: ELASTIX-9 ComponentExist() did not work

The ENH: We deliberately avoid using connection indexes, but instead
force the user to think in terms of components which is conceptually
simpler and results in less pointers being thrown around while
constructing the blueprint.

The BUG: Temporarily disable functionality for deleting component until
we find a god way of asserting that vertex exist before it is accessed.

Modules/Core/Blueprints/include/elxBlueprint.h

@@ -33,8 +33,8 @@ public:
     ParameterMapType parameterMap;
   };
   
-  typedef boost::adjacency_list< boost::vecS,      
-                                 boost::vecS,      
+  typedef boost::adjacency_list< boost::listS,      
+                                 boost::listS,      
                                  boost::directedS,
                                  ComponentPropertyType,
                                  ConnectionPropertyType >             GraphType;
@@ -57,23 +57,45 @@ public:
   ComponentIndexType AddComponent( void );
   ComponentIndexType AddComponent( ParameterMapType parameterMap );
   ParameterMapType GetComponent( ComponentIndexType index );
-  bool SetComponent( ComponentIndexType, ParameterMapType parameterMap );
-  void DeleteComponent( ComponentIndexType );
-  bool ComponentExist( ComponentIndexType index );
+  void SetComponent( ComponentIndexType, ParameterMapType parameterMap );
+
+  // TODO: Let user delete component. To do this, we need a proper way of 
+  // checking that a vertex exist. Otherwise a call to  GetComponent() on 
+  // a deleted vertex will result in segfault. It is not really a problem 
+  // that there is no delete vertex feature at this point since typically 
+  // the blueprint interface is used procedurally to generate a specific
+  // blueprint.
+  // void DeleteComponent( ComponentIndexType );
 
   ComponentIteratorPairType GetComponentIterator( void ) {
-    return boost::vertices(this->m_Graph);
+    return boost::vertices( this->m_Graph );
   }
 
+  // Interface for managing connections between components in which we 
+  // deliberately avoid using connection indexes, but instead force
+  // the user to think in terms of components (which is conceptually simpler)
+  bool AddConnection( ComponentIndexType upstream, ComponentIndexType downstream );
+  bool AddConnection( ComponentIndexType upstream, ComponentIndexType downstream, ParameterMapType parameterMap );
+  ParameterMapType GetConnection( ComponentIndexType upstream, ComponentIndexType downstream );
+  bool SetConnection(  ComponentIndexType upstream, ComponentIndexType downstream, ParameterMapType parameterMap );
+  bool DeleteConnection( ComponentIndexType upstream, ComponentIndexType downstream );
+  bool ConnectionExists( ComponentIndexType upstream, ComponentIndexType downstream );
+
+  // Returns iterator for all connections in the graph
+  ConnectionIteratorPairType GetConnectionIterator( void ) {
+    return boost::edges(this->m_Graph);
+  }
 
   // Returns the outgoing connections from a component in the graph,
   // i.e. all components that reads data from given component
-  OutputIteratorPairType GetOuptutIterator( const ComponentIndexType index ) {
+  OutputIteratorPairType GetOutputIterator( const ComponentIndexType index ) {
     return boost::out_edges(index, this->m_Graph);
   }
 
 private:
 
+  ConnectionIndexType GetConnectionIndex( ComponentIndexType upsteam, ComponentIndexType downstream );
+
   GraphType m_Graph;
 
 };

Modules/Core/Blueprints/src/elxBlueprint.cxx

@@ -38,46 +38,113 @@ Blueprint
 {
   this->Modified();
 
-  if( this->ComponentExist( index ) ) {
-    return this->m_Graph[index].parameterMap;
-  } else {
-    itkExceptionMacro( "Blueprint does not contain component with index " << index );
+  return this->m_Graph[ index ].parameterMap;
+}
+
+void
+Blueprint
+::SetComponent( ComponentIndexType index, ParameterMapType parameterMap )
+{
+  this->Modified();
+  this->m_Graph[ index ].parameterMap = parameterMap;
+}
+
+// TODO: See explanation in elxBlueprint.h
+// void
+// Blueprint
+// ::DeleteComponent( const ComponentIndexType index )
+// {
+//   this->Modified();
+//
+//   clear_vertex( index, this->m_Graph );
+//   remove_vertex( index, this->m_Graph );
+// }
+
+bool
+Blueprint
+::AddConnection( ComponentIndexType upstream, ComponentIndexType downstream )
+{
+  this->Modified();
+
+  if( this->ConnectionExists( upstream, downstream) ) {
+    return false;
   }
+  
+  // Adds directed connection from upstream component to downstream component
+  return boost::add_edge( upstream, downstream, this->m_Graph ).second;
 }
 
 bool
 Blueprint
-::SetComponent( ComponentIndexType index, ParameterMapType parameterMap )
+::AddConnection( ComponentIndexType upstream, ComponentIndexType downstream, ParameterMapType parameterMap )
+{
+  this->Modified();
+
+  // If the connection does not exist, add the parameter map, otherwise don't do anything
+  // because previous settings will be overwritten. If the user do want to overwrite 
+  // current settings, she should use SetConnection() instead where the intent is explicit.  
+  if( this->ConnectionExists( upstream, downstream) ) {
+    return false;
+  }
+
+  this->m_Graph[ this->GetConnectionIndex( upstream, downstream ) ].parameterMap = parameterMap;
+  return true;
+}
+
+Blueprint::ParameterMapType
+Blueprint
+::GetConnection( ComponentIndexType upstream, ComponentIndexType downstream )
 {
   this->Modified();
 
-  if( this->ComponentExist( index ) )
-  {
-    this->m_Graph[index].parameterMap = parameterMap;
+  return this->m_Graph[ this->GetConnectionIndex( upstream, downstream ) ].parameterMap;
+}
+
+bool
+Blueprint
+::SetConnection( ComponentIndexType upstream, ComponentIndexType downstream, ParameterMapType parameterMap )
+{
+  this->Modified();
+
+  if( !this->ConnectionExists( upstream, downstream ) ) {
+    return this->AddConnection( upstream, downstream, parameterMap );
   } else {
-    itkExceptionMacro( "Blueprint does not contain component with index " << index )
+    this->m_Graph[ this->GetConnectionIndex( upstream, downstream ) ].parameterMap = parameterMap;
+    return true;
   }
 }
 
-void
+bool
 Blueprint
-::DeleteComponent( const ComponentIndexType index )
+::DeleteConnection( ComponentIndexType upstream, ComponentIndexType downstream )
 {
   this->Modified();
 
-  if( this->ComponentExist( index ) ) {
-    clear_vertex( index, this->m_Graph );
-    remove_vertex( index, this->m_Graph );
+  if( this->ConnectionExists( upstream, downstream ) ) {
+    this->m_Graph.remove_edge( this->GetConnectionIndex( upstream, downstream ) );
   }
+  
+  return !this->ConnectionExists( upstream, downstream );
 }
 
 bool
 Blueprint
-::ComponentExist( ComponentIndexType index )
+::ConnectionExists( ComponentIndexType upstream, ComponentIndexType downstream )
 {
-  return boost::vertex( index, this->m_Graph ) != boost::graph_traits< GraphType >::null_vertex();
+  return boost::edge( upstream, downstream, this->m_Graph).second;
 }
 
+Blueprint::ConnectionIndexType
+Blueprint
+::GetConnectionIndex( ComponentIndexType upstream, ComponentIndexType downstream )
+{
+  // This function is part of the internal API and should fail hard if we use it incorrectly
+  if( !this->ConnectionExists( upstream, downstream ) ) {
+    itkExceptionMacro( "Blueprint does not contain connection from component " << upstream << " to " << downstream );
+  }
+  
+  return boost::edge( upstream, downstream, this->m_Graph).first;
+}
 
 // void 
 // Blueprint

Testing/Unit/elxBluePrintTest.cxx

@@ -19,44 +19,52 @@ public:
   ParameterMapType parameterMap;
 };
 
-TEST_F( BlueprintTest, Add )
+TEST_F( BlueprintTest, AddComponent )
 {
   BlueprintPointerType blueprint;
   EXPECT_NO_THROW( blueprint = Blueprint::New() );
 
   ComponentIndexType index0;
   EXPECT_NO_THROW( index0 = blueprint->AddComponent() );
-  EXPECT_EQ( Blueprint::ComponentIndexType(0), index0 );
 
   ComponentIndexType index1;
   EXPECT_NO_THROW( index1 = blueprint->AddComponent( parameterMap ) );
-  EXPECT_EQ( Blueprint::ComponentIndexType(1), index1 );
-
-  Blueprint::ComponentIteratorPairType componentIterator = blueprint->GetComponentIterator();
 }
 
-TEST_F( BlueprintTest, Get ) 
+TEST_F( BlueprintTest, GetComponent ) 
 {
   BlueprintPointerType blueprint = Blueprint::New();
   ComponentIndexType index = blueprint->AddComponent( parameterMap );
-  EXPECT_EQ( Blueprint::ComponentIndexType(0), index );
 
   ParameterMapType parameterMapTest;
   EXPECT_NO_THROW( parameterMapTest = blueprint->GetComponent( index ) );
   EXPECT_EQ( parameterMap["ComponentName"], parameterMapTest["ComponentName"] );
 }
 
-TEST_F( BlueprintTest, Delete ) 
+TEST_F( BlueprintTest, SetComponent ) 
 {
   BlueprintPointerType blueprint = Blueprint::New();
   ComponentIndexType index = blueprint->AddComponent( parameterMap );
 
   ParameterMapType parameterMapTest;
+  EXPECT_NO_THROW( blueprint->SetComponent( index, parameterMap ) );
   EXPECT_NO_THROW( parameterMapTest = blueprint->GetComponent( index ) );
   EXPECT_EQ( parameterMap["ComponentName"], parameterMapTest["ComponentName"] );
-
-  EXPECT_NO_THROW( blueprint->DeleteComponent( index ) );
-  EXPECT_ANY_THROW( parameterMapTest = blueprint->GetComponent( index ) );
 }
 
+// TODO: The final line segfaults since at this point GetComponent has no way
+// of checking that the index actually exist. See explanation in elxBlueprint.h
+// TEST_F( BlueprintTest, DeleteComponent ) 
+// {
+//   BlueprintPointerType blueprint = Blueprint::New();
+//   ComponentIndexType index = blueprint->AddComponent( parameterMap );
+//
+//   ParameterMapType parameterMapTest;
+//   EXPECT_NO_THROW( parameterMapTest = blueprint->GetComponent( index ) );
+//   EXPECT_EQ( parameterMap["ComponentName"], parameterMapTest["ComponentName"] );
+//
+//   EXPECT_NO_THROW( blueprint->DeleteComponent( index ) );
+//   EXPECT_ANY_THROW( parameterMapTest = blueprint->GetComponent( index ) );
+// }
+
 } // namespace elx