From 6fcf40d9f27fd5959ed3db77b44369cc5fc9b1b2 Mon Sep 17 00:00:00 2001
From: Ansgar Philippsen <ansgar.philippsen@gmail.com>
Date: Fri, 27 Jul 2012 13:03:27 +0200
Subject: [PATCH] EntityHandle interface change! Also refactored EntityImpl to
 use Transform handle transformations

the internal representation of a transformation for EntityImpl was changed from Mat4 to Transform
in preparation for adding transformations to entity views; this impacts quite a few higher-level routines
that used the transformation matrix, which now can use the more elegant Transform::Apply method instead;

GetTransformationMatrix and IsTransformationIdentity methods in the EntityHandle interface are marked
as deprecated; use GetTransform and HasTransform instead. In addition, there is a ClearTransform method
which ensures that the internal has_transform flag gets set to false

geom::Transform was extended to offer ApplyInverse, and matrix inversion exceptions are handled internally

constructing a Vec3 from a Vec4 no longer throws a DivideByZero error when w is close to zero, instead it
is silently assumed that this is not a true homogeneous coordinate conversion

as an additional comment (and note to self), atom->GetPos always returns transformed positions; however,
GetAltPos returns original positions, and original positions are also stored in the AtomGroup on the
Impl level. Both the mol unit test as well as the PDB writer expect this behavior; the PDB writer applies
the transform after calling GetAltPos. Commented code is in place now to change this behavior, i.e. have
GetAltPos return transformed positions, making it consistent with GetPos, should this be desired.
---
 modules/geom/src/transform.cc             | 31 +++++++++--
 modules/geom/src/transform.hh             | 14 ++++-
 modules/geom/src/vec3.hh                  | 12 ++--
 modules/geom/tests/test_vec3.cc           |  2 +-
 modules/io/src/mol/pdb_writer.cc          |  6 +-
 modules/mol/base/pymod/export_editors.cc  | 12 +++-
 modules/mol/base/pymod/export_entity.cc   | 22 +++++---
 modules/mol/base/src/entity_handle.cc     | 52 +++++++++++++++--
 modules/mol/base/src/entity_handle.hh     | 19 +++++--
 modules/mol/base/src/impl/atom_impl.cc    |  6 +-
 modules/mol/base/src/impl/chain_impl.cc   |  7 +++
 modules/mol/base/src/impl/chain_impl.hh   |  2 +
 modules/mol/base/src/impl/entity_impl.cc  | 63 +++++++++------------
 modules/mol/base/src/impl/entity_impl.hh  | 21 +++----
 modules/mol/base/src/impl/residue_impl.cc | 35 ++++++++++--
 modules/mol/base/src/impl/residue_impl.hh |  3 +
 modules/mol/base/src/xcs_editor.cc        | 68 +++++++++++------------
 modules/mol/base/src/xcs_editor.hh        |  3 +-
 modules/mol/base/tests/test_entity.cc     | 22 +++-----
 19 files changed, 256 insertions(+), 144 deletions(-)

diff --git a/modules/geom/src/transform.cc b/modules/geom/src/transform.cc
index b40a3229d..c620410fb 100644
--- a/modules/geom/src/transform.cc
+++ b/modules/geom/src/transform.cc
@@ -40,6 +40,13 @@ void Transform::SetMatrix(const Mat4& m)
 {
   tm_=m;
   ttm_ = Transpose(tm_);
+  try {
+    itm_ = Invert(tm_);
+  } catch (GeomException& e) {
+    std::cerr << "caught GeomException in Transform::SetMatrix: " << e.what() << std::endl;
+    std::cerr << m << std::endl;
+    itm_=geom::Mat4();
+  }
   update_components();
 }
 
@@ -151,6 +158,18 @@ Vec4 Transform::Apply(const Vec4& v) const
   return nrvo;
 }
 
+Vec3 Transform::ApplyInverse(const Vec3& v) const
+{
+  Vec3 nrvo(itm_*Vec4(v));
+  return nrvo;
+}
+
+Vec4 Transform::ApplyInverse(const Vec4& v) const
+{
+  Vec4 nrvo=itm_*v;
+  return nrvo;
+}
+
 geom::AlignedCuboid Transform::Apply(const geom::AlignedCuboid& c) const
 {
   geom::Vec3 cmin=c.GetMin();
@@ -196,14 +215,14 @@ void Transform::update_tm()
 {
   tm_ =
     Mat4(1.0,0.0,0.0,trans_[0],
-               0.0,1.0,0.0,trans_[1],
-               0.0,0.0,1.0,trans_[2],
-               0.0,0.0,0.0,1.0) *
+         0.0,1.0,0.0,trans_[1],
+         0.0,0.0,1.0,trans_[2],
+         0.0,0.0,0.0,1.0) *
     Mat4(rot_) *
     Mat4(1.0,0.0,0.0,-cen_[0],
-               0.0,1.0,0.0,-cen_[1],
-               0.0,0.0,1.0,-cen_[2],
-               0.0,0.0,0.0,1.0);
+         0.0,1.0,0.0,-cen_[1],
+         0.0,0.0,1.0,-cen_[2],
+         0.0,0.0,0.0,1.0);
   ttm_ = Transpose(tm_);
   // TODO: calculate from rot, cen and trans
   try {
diff --git a/modules/geom/src/transform.hh b/modules/geom/src/transform.hh
index ac6a87d0b..6965c4843 100644
--- a/modules/geom/src/transform.hh
+++ b/modules/geom/src/transform.hh
@@ -39,7 +39,10 @@ namespace geom {
 class DLLEXPORT_OST_GEOM Transform {
 public:
   Transform();
-
+  
+  /// \brief reset to identity
+  void Reset() {*this=Transform();}
+  
   /// \brief retrieve transformation matrix
   Mat4 GetMatrix() const {return tm_;}
   /// \brief retrieve transposed transformation matrix
@@ -78,9 +81,16 @@ public:
   void SetTrans(const Vec3& t);
   Vec3 GetTrans() const;  
   //@}
-  
+
+  // apply to a vec3 and return result
   Vec3 Apply(const Vec3& v) const;
+  // apply to a vec4 and return result
   Vec4 Apply(const Vec4& v) const;
+  // apply inverse to a vec3 and return result
+  Vec3 ApplyInverse(const Vec3& v) const;
+  // apply inverse to a vec4 and return result
+  Vec4 ApplyInverse(const Vec4& v) const;
+  // apply to an aligned cuboid and return result
   AlignedCuboid Apply(const AlignedCuboid& c) const;
   Transform Apply(const Transform& tf) const;
 
diff --git a/modules/geom/src/vec3.hh b/modules/geom/src/vec3.hh
index 7b4bb5b33..c1cde046c 100644
--- a/modules/geom/src/vec3.hh
+++ b/modules/geom/src/vec3.hh
@@ -239,11 +239,15 @@ namespace geom {
   inline Vec3::Vec3(const Vec4& v): x(v.x), y(v.y), z(v.z) 
   { 
     if (std::fabs(v.w)<1e-10) {
-      throw DivideByZeroException();
+      // it is better to ignore very small w and to simply assume
+      // that this is not a homogeneous coordinate rather than
+      // throwing an exception
+      //throw DivideByZeroException();
+    } else {
+      x/=v.w;
+      y/=v.w;
+      z/=v.w;
     }
-    x/=v.w;
-    y/=v.w;
-    z/=v.w;
   }
 } // namespace geom
 
diff --git a/modules/geom/tests/test_vec3.cc b/modules/geom/tests/test_vec3.cc
index 9e3e4d948..3964daf00 100644
--- a/modules/geom/tests/test_vec3.cc
+++ b/modules/geom/tests/test_vec3.cc
@@ -58,8 +58,8 @@ BOOST_AUTO_TEST_CASE(init_vec3)
   // conversion from vec4
   Vec3 v6(Vec4(0.4,1.2,4.0,2.0));
   BOOST_CHECK(match(v6,0.2,0.6,2.0));
-  BOOST_CHECK_THROW( Vec3(Vec4(1.0,1.0,1.0,0.0)), DivideByZeroException);
 
+  BOOST_CHECK(match(Vec3(Vec4(2.0,1.0,3.0,0.0)),2.0,1.0,3.0));
 }
 
 BOOST_AUTO_TEST_CASE(access_vec3)
diff --git a/modules/io/src/mol/pdb_writer.cc b/modules/io/src/mol/pdb_writer.cc
index d06a5228c..28c18cd36 100644
--- a/modules/io/src/mol/pdb_writer.cc
+++ b/modules/io/src/mol/pdb_writer.cc
@@ -154,8 +154,10 @@ void write_atom(std::ostream& ostr, FormattedLine& line,
   } else {
     for (std::vector<String>::const_iterator
          i=names.begin(), e=names.end(); i!=e; ++i) {
-      geom::Mat4 tf=atom.GetEntity().GetTransformationMatrix();
-      p=geom::Vec3(tf*geom::Vec4(atom.GetAltPos(*i)));
+      // GetAltPos always return orig pos, i.e. does not honor
+      // transformations like GetPos does - so apply it here
+      // seems like a FIXME...
+      p=atom.GetEntity().GetTransform().Apply(atom.GetAltPos(*i));
       line(30, 50).Clear();
 
       if (i->size()>1) {
diff --git a/modules/mol/base/pymod/export_editors.cc b/modules/mol/base/pymod/export_editors.cc
index c9d787398..aae903f7d 100644
--- a/modules/mol/base/pymod/export_editors.cc
+++ b/modules/mol/base/pymod/export_editors.cc
@@ -216,6 +216,7 @@ void export_Editors()
   import_array();
 #endif
 
+
   class_<EditorBase>("EditorBase", no_init)
     .def("InsertChain", insert_chain_a)
     .def("InsertChain", insert_chain_b,(arg("chain_name"),arg("chain"), arg("deep")=false))
@@ -250,12 +251,19 @@ void export_Editors()
     .def("RenumberAllResidues",&EditorBase::RenumberAllResidues)
   ;
   
+  void (XCSEditor::*apply_transform1)(const geom::Mat4&) = &XCSEditor::ApplyTransform;
+  void (XCSEditor::*apply_transform2)(const geom::Transform&) = &XCSEditor::ApplyTransform;
+  void (XCSEditor::*set_transform1)(const geom::Mat4&) = &XCSEditor::SetTransform;
+  void (XCSEditor::*set_transform2)(const geom::Transform&) = &XCSEditor::SetTransform;
+
   class_<XCSEditor, bases<EditorBase> >("XCSEditor", no_init)
     .def("SetAtomPos", set_t_pos)
     .def("SetAtomTransformedPos", set_t_pos)
     .def("SetAtomOriginalPos", set_o_pos)
-    .def("ApplyTransform", &XCSEditor::ApplyTransform)
-    .def("SetTransform", &XCSEditor::SetTransform)
+    .def("ApplyTransform", apply_transform1)
+    .def("ApplyTransform", apply_transform2)
+    .def("SetTransform", set_transform1)
+    .def("SetTransform", set_transform2)
     .def("UpdateICS", &XCSEditor::UpdateICS)
     .def("__exit__", &XCSEditor::UpdateICS)    
   ;
diff --git a/modules/mol/base/pymod/export_entity.cc b/modules/mol/base/pymod/export_entity.cc
index adf2c83b7..d01cd0472 100644
--- a/modules/mol/base/pymod/export_entity.cc
+++ b/modules/mol/base/pymod/export_entity.cc
@@ -100,7 +100,15 @@ PyObject* get_pos1(EntityHandle& entity)
   return get_pos2(entity,true);
 }
 
+geom::Mat4 depr_get_transformation_matrix(const EntityHandle& eh)
+{
+  return eh.GetTransformationMatrix();
+}
 
+bool depr_is_transformation_identity(const EntityHandle& eh)
+{
+  return eh.IsTransformationIdentity();
+}
 
 #endif
 } // ns
@@ -181,17 +189,17 @@ void export_Entity()
     .add_property("bonds", &EntityHandle::GetBondList)
     .def("GetBounds", &EntityHandle::GetBounds)
     .add_property("bounds", &EntityHandle::GetBounds)
-    .def("GetTransformationMatrix", &EntityHandle::GetTransformationMatrix,
-         return_value_policy<copy_const_reference>())
-    .add_property("transform", 
-                   make_function(&EntityHandle::GetTransformationMatrix, 
-                                 return_value_policy<copy_const_reference>()))    
-
+    .def("GetTransformationMatrix", depr_get_transformation_matrix)
+    .def("IsTransformationIdentity",depr_is_transformation_identity)
+    .def("GetTransform",&EntityHandle::GetTransform)
+    .def("SetTransform",&EntityHandle::SetTransform)
+    .add_property("transform",&EntityHandle::GetTransform,&EntityHandle::SetTransform)
+    .def("HasTransform",&EntityHandle::HasTransform)
+    .def("ClearTransform",&EntityHandle::ClearTransform)
     .def("EditXCS", &EntityHandle::EditXCS, arg("mode")=UNBUFFERED_EDIT)
     .def("EditICS", &EntityHandle::EditICS, arg("mode")=UNBUFFERED_EDIT)
     .def("RequestXCSEditor", &depr_request_xcs_editor, arg("mode")=UNBUFFERED_EDIT)
     .def("RequestICSEditor", &depr_request_ics_editor, arg("mode")=UNBUFFERED_EDIT)  
-    .def("IsTransformationIdentity",&EntityHandle::IsTransformationIdentity)
     .def(self==self)
     .def(self!=self)
 #if OST_NUMPY_SUPPORT_ENABLED
diff --git a/modules/mol/base/src/entity_handle.cc b/modules/mol/base/src/entity_handle.cc
index 0abad5ede..e34e11d2e 100644
--- a/modules/mol/base/src/entity_handle.cc
+++ b/modules/mol/base/src/entity_handle.cc
@@ -17,6 +17,8 @@
 // 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
 //------------------------------------------------------------------------------
 
+#include <ost/log.hh>
+
 #include "impl/entity_impl.hh"
 #include "bond_handle.hh"
 #include "torsion_handle.hh"
@@ -235,24 +237,62 @@ Real EntityHandle::GetAngle(const AtomView& a1, const AtomView& a2,
   return GetAngle(a1.GetHandle(), a2.GetHandle(), a3.GetHandle());
 }
 
-const geom::Mat4& EntityHandle::GetTransformationMatrix() const
-
+geom::Mat4 EntityHandle::GetTransformationMatrix() const
 {
+  static bool warn=true;
+  if(warn) {
+    LOG_WARNING("Entity::GetTransformationMatrix is deprecated, use GetTransform instead");
+    warn=false;
+  }
   this->CheckValidity();
-  return Impl()->GetTransfMatrix();
+  return Impl()->GetTransform().GetMatrix();
 }
 
 
-const geom::Mat4& EntityHandle::GetInvTransformationMatrix() const
+geom::Mat4 EntityHandle::GetInvTransformationMatrix() const
 {
+  static bool warn=true;
+  if(warn) {
+    LOG_WARNING("Entity::GetInvTransformationMatrix is deprecated, use GetTransform instead");
+    warn=false;
+  }
   this->CheckValidity();
-  return Impl()->GetInvTransfMatrix();
+  return Impl()->GetTransform().GetInvertedMatrix();
 }
 
 bool EntityHandle::IsTransformationIdentity() const
+{
+  static bool warn=true;
+  if(warn) {
+    LOG_WARNING("Entity::IsTransformationIdentity is deprecated, use HasTransform instead");
+    warn=false;
+  }
+  this->CheckValidity();
+  return !Impl()->HasTransform();
+}
+
+geom::Transform EntityHandle::GetTransform() const
+{
+  this->CheckValidity();
+  return Impl()->GetTransform();  
+}
+
+void EntityHandle::SetTransform(const geom::Transform& tf)
+{
+  this->CheckValidity();
+  Impl()->SetTransform(tf);  
+}
+
+bool EntityHandle::HasTransform() const
+{
+  this->CheckValidity();
+  return Impl()->HasTransform();  
+}
+
+void EntityHandle::ClearTransform()
 {
   this->CheckValidity();
-  return Impl()->IsTransfIdentity();
+  Impl()->ClearTransform();  
 }
 
 ResidueHandle EntityHandle::FindResidue(const String& chain_name,
diff --git a/modules/mol/base/src/entity_handle.hh b/modules/mol/base/src/entity_handle.hh
index 2783b404b..fe2a43913 100644
--- a/modules/mol/base/src/entity_handle.hh
+++ b/modules/mol/base/src/entity_handle.hh
@@ -264,13 +264,22 @@ public:
   Real GetAngle(const AtomView& a1, const AtomView& a2,
                 const AtomView& a3) const;
 
-  const geom::Mat4& GetTransformationMatrix() const;
-
-
-  const geom::Mat4& GetInvTransformationMatrix() const;
-
+  /// \brief DEPRECATED
+  geom::Mat4 GetTransformationMatrix() const;
+  /// \brief DEPRECATED
+  geom::Mat4 GetInvTransformationMatrix() const;
+  /// \brief DEPRECATED
   bool IsTransformationIdentity() const;
 
+  /// \brief retrieve transformation of this entity
+  geom::Transform GetTransform() const;
+  /// \brief set transformation that will affect this entity
+  void SetTransform(const geom::Transform& t);
+  /// \brief checks whether a transform has been set
+  bool HasTransform() const;
+  /// \brief remove transform
+  void ClearTransform();
+
   /// \brief get complete list of residues
   /// \sa #ResiduesBegin, #ResiduesEnd
   ResidueHandleList GetResidueList() const;
diff --git a/modules/mol/base/src/impl/atom_impl.cc b/modules/mol/base/src/impl/atom_impl.cc
index 4f7f059b8..3a2b4d013 100644
--- a/modules/mol/base/src/impl/atom_impl.cc
+++ b/modules/mol/base/src/impl/atom_impl.cc
@@ -56,10 +56,8 @@ AtomImpl::AtomImpl(const EntityImplPtr& e,
   state_(0),
   index_(index)
 {
-  EntityHandle ent = this->GetEntity();
-  geom::Mat4 transf_matrix = ent.GetTransformationMatrix();
-  geom::Vec3 transf_pos = geom::Vec3(transf_matrix*geom::Vec4(p));
-  tf_pos_ = transf_pos;
+  EntityImplPtr eip = GetEntity();
+  tf_pos_ = eip->HasTransform() ? eip->GetTransform().Apply(p) : p;
   prop_=AtomProp::GetDefaultProps(element_);
 }
 
diff --git a/modules/mol/base/src/impl/chain_impl.cc b/modules/mol/base/src/impl/chain_impl.cc
index 91261db8e..8b3cbe271 100644
--- a/modules/mol/base/src/impl/chain_impl.cc
+++ b/modules/mol/base/src/impl/chain_impl.cc
@@ -530,5 +530,12 @@ void ChainImpl::SetInSequence(const int index)
     this->UpdateShifts();
   }
 }
+
+void ChainImpl::UpdateTransformedPos()
+{
+  for (ResidueImplList::iterator rit=residue_list_.begin(); rit!=residue_list_.end(); ++rit) {
+    (*rit)->UpdateTransformedPos();
+  }
+}
   
 }}} // ns
diff --git a/modules/mol/base/src/impl/chain_impl.hh b/modules/mol/base/src/impl/chain_impl.hh
index 446c74052..018c3b9e1 100644
--- a/modules/mol/base/src/impl/chain_impl.hh
+++ b/modules/mol/base/src/impl/chain_impl.hh
@@ -182,6 +182,8 @@ public:
   ///       property and updates it accordingly      
   void SetInSequence(int index);
 
+  void UpdateTransformedPos();
+
 private:
   int GetIndexForResNumInSequence(const ResNum& number) const;
   void UpdateShifts();
diff --git a/modules/mol/base/src/impl/entity_impl.cc b/modules/mol/base/src/impl/entity_impl.cc
index 6bdbd835c..8b81293fb 100644
--- a/modules/mol/base/src/impl/entity_impl.cc
+++ b/modules/mol/base/src/impl/entity_impl.cc
@@ -67,9 +67,8 @@ EntityImpl::EntityImpl():
   chain_list_(),
   connector_map_(),
   torsion_map_(),
-  transformation_matrix_(),
-  inverse_transformation_matrix_(),
-  identity_transf_(true),
+  transform_(),
+  has_transform_(false),
   atom_organizer_(5.0),
   fragment_list_(),
   observer_map_(),
@@ -251,21 +250,6 @@ int EntityImpl::GetChainCount() const
   return static_cast<int>(chain_list_.size());
 }
 
-const geom::Mat4& EntityImpl::GetTransfMatrix() const
-{
-  return transformation_matrix_;
-}
-
-const geom::Mat4& EntityImpl::GetInvTransfMatrix() const
-{
-  return inverse_transformation_matrix_;
-}
-
-bool EntityImpl::IsTransfIdentity() const
-{
-  return identity_transf_;
-}
-
 EntityImpl::~EntityImpl()
 {
   // notify all observers of pending destruct
@@ -329,23 +313,18 @@ Real EntityImpl::GetMass() const {
 
 AtomImplPtr EntityImpl::CreateAtom(const ResidueImplPtr& rp,
                                    const String& name,
-                                   const geom::Vec3& pos,
+                                   const geom::Vec3& pos2,
                                    const String& ele)
 {
 #if MAKE_SHARED_AVAILABLE
   AtomImplPtr ap=boost::make_shared<AtomImpl>(shared_from_this(), rp, name, 
-                                              pos, ele,next_index_++);
+                                              pos2, ele,next_index_++);
 #else
-  AtomImplPtr ap(new AtomImpl(shared_from_this(), rp, name, pos, ele, next_index_++));
+  AtomImplPtr ap(new AtomImpl(shared_from_this(), rp, name, pos2, ele, next_index_++));
 #endif
-  if (!identity_transf_) {
-    geom::Vec3 transformed_pos = geom::Vec3(transformation_matrix_*geom::Vec4(pos));
-    ap->TransformedPos()=transformed_pos;
-    atom_organizer_.Add(ap,transformed_pos);
-  } else {
-    ap->TransformedPos()=pos;
-    atom_organizer_.Add(ap,pos);
-  }
+  geom::Vec3 pos = has_transform_ ? transform_.Apply(pos2) : pos2;
+  ap->TransformedPos()=pos;
+  atom_organizer_.Add(ap,pos);
   atom_map_.insert(AtomImplMap::value_type(ap.get(),ap));
   return ap;
 }
@@ -767,21 +746,25 @@ void EntityImpl::Apply(EntityVisitor& v)
   v.OnExit();
 }
 
-void EntityImpl::ApplyTransform(const geom::Mat4 transfmat)
+void EntityImpl::ApplyTransform(const geom::Transform& tf)
 {
-  geom::Mat4 updated_transformation_matrix=transfmat*transformation_matrix_;
-  this->SetTransform(updated_transformation_matrix);
+  SetTransform(transform_.Apply(tf));
 }
 
-void EntityImpl::SetTransform(const geom::Mat4 transfmat)
+void EntityImpl::SetTransform(const geom::Transform& tf)
 {
-  transformation_matrix_=transfmat;
-  inverse_transformation_matrix_=Invert(transformation_matrix_);
-  identity_transf_ = (transformation_matrix_==geom::Mat4());
+  transform_=tf;
+  has_transform_=true;
   this->UpdateTransformedPos();
   this->MarkOrganizerDirty();
 }
 
+void EntityImpl::ClearTransform()
+{
+  has_transform_=false;
+  SetTransform(geom::Transform());
+}
+
 void EntityImpl::AttachObserver(const EntityObserverPtr& o)
 {
   observer_map_.insert(EntityObserverMap::value_type(o.get(),o));
@@ -805,7 +788,8 @@ void EntityImpl::Swap(EntityImpl& impl)
   chain_list_.swap(impl.chain_list_);
   connector_map_.swap(impl.connector_map_);
   torsion_map_.swap(impl.torsion_map_);
-  std::swap(transformation_matrix_,impl.transformation_matrix_);
+  std::swap(transform_,impl.transform_);
+  std::swap(has_transform_,impl.has_transform_);
   atom_organizer_.Swap(impl.atom_organizer_);
   fragment_list_.swap(impl.fragment_list_);
   observer_map_.swap(impl.observer_map_);
@@ -1201,7 +1185,10 @@ void EntityImpl::RenameChain(ChainImplPtr chain, const String& new_name)
 
 void EntityImpl::UpdateTransformedPos(){
   for(AtomImplMap::iterator it = atom_map_.begin();it!=atom_map_.end();++it) {
-    it->second->TransformedPos()=geom::Vec3(transformation_matrix_*geom::Vec4(it->second->OriginalPos()));
+    it->second->TransformedPos()=has_transform_ ? transform_.Apply(it->second->OriginalPos()) : it->second->OriginalPos();
+  }
+  for(ChainImplList::iterator cit=chain_list_.begin();cit!=chain_list_.end();++cit) {
+    (*cit)->UpdateTransformedPos();
   }
 }
 
diff --git a/modules/mol/base/src/impl/entity_impl.hh b/modules/mol/base/src/impl/entity_impl.hh
index 209858644..a5f5cd805 100644
--- a/modules/mol/base/src/impl/entity_impl.hh
+++ b/modules/mol/base/src/impl/entity_impl.hh
@@ -143,9 +143,12 @@ public:
   void UpdateFromXCS();
 
   void Apply(EntityVisitor& v);
-  void ApplyTransform(const geom::Mat4 transfmat);
+  void ApplyTransform(const geom::Transform& t);
 
-  void SetTransform(const geom::Mat4 transfmat);
+  void SetTransform(const geom::Transform& t);
+  const geom::Transform& GetTransform() const {return transform_;}
+  bool HasTransform() const {return has_transform_;}
+  void ClearTransform();
 
   void AttachObserver(const EntityObserverPtr& o);
   void DetachObserver(const EntityObserverPtr& o);
@@ -198,14 +201,6 @@ public:
   //! Get number of chains
   int GetChainCount() const;
 
-  //! Get transformation matrix
-  const geom::Mat4& GetTransfMatrix() const;
-
-  //! Get inverse transformation matrix
-  const geom::Mat4& GetInvTransfMatrix() const;
-
-  bool IsTransfIdentity() const;
-
   const ChainImplList& GetChainList() const { return chain_list_; }
 
   ChainImplList& GetChainList() { return chain_list_; }
@@ -266,10 +261,8 @@ private:
   ConnectorImplMap connector_map_;
   TorsionImplMap torsion_map_;
 
-  geom::Mat4 transformation_matrix_;
-  geom::Mat4 inverse_transformation_matrix_;
-  bool identity_transf_;
-
+  geom::Transform transform_;
+  bool has_transform_;
 
   SpatialAtomOrganizer atom_organizer_;
   FragmentImplList fragment_list_;
diff --git a/modules/mol/base/src/impl/residue_impl.cc b/modules/mol/base/src/impl/residue_impl.cc
index 1e0fa1d46..fd2c7762b 100644
--- a/modules/mol/base/src/impl/residue_impl.cc
+++ b/modules/mol/base/src/impl/residue_impl.cc
@@ -118,9 +118,16 @@ geom::Vec3 ResidueImpl::GetAltAtomPos(const AtomImplPtr& atom,
     throw Error("No alt atom group '"+group+"'");
   }
   const AtomGroup& g=i->second;
+  EntityImplPtr eip=GetEntity();
   for (AtomGroupEntryList::const_iterator j=g.atoms.begin(), 
        e=g.atoms.end(); j!=e; ++j) {
     if (atom==j->atom.lock()) {
+      // the alternate entry positions are stored as original, and
+      // are returned as original as well ?! at least that is what
+      // the unit test seems to require
+      // also, the PDB writer currently expects this behavior, so
+      // if it is changed here, it must be changed there as well
+      //return eip->HasTransform() ? eip->GetTransform().Apply(j->pos) : j->pos;
       return j->pos;
     }
   }
@@ -626,14 +633,11 @@ bool ResidueImpl::SwitchAtomPos(const String& group) {
   AtomGroup& agr=i->second;
   AtomGroupEntryList::iterator j=agr.atoms.begin();
   for (; j!=agr.atoms.end(); ++j) {
-
     AtomGroupEntry& entry=*j;
     assert(!entry.atom.expired());
     entry.atom.lock()->OriginalPos()=entry.pos;
-    EntityHandle ent = entry.atom.lock()->GetEntity();
-    geom::Mat4 transf_matrix = ent.GetTransformationMatrix();
-    geom::Vec3 transf_pos = geom::Vec3(transf_matrix*geom::Vec4(entry.pos));
-    entry.atom.lock()->TransformedPos()=transf_pos;
+    EntityImplPtr eip = entry.atom.lock()->GetEntity();
+    entry.atom.lock()->TransformedPos() = eip->GetTransform().Apply(entry.pos);
     entry.atom.lock()->SetBFactor(j->b_factor);
     entry.atom.lock()->SetOccupancy(j->occ);
   }
@@ -671,4 +675,25 @@ String ResidueImpl::GetStringProperty(Prop::ID prop_id) const
   }
 }
 
+void ResidueImpl::UpdateTransformedPos()
+{
+  /*
+    the alt atom positions always store the original pos; hence the below code
+    is not necessary; however, it isn't clear (at least to me (AP)) if the
+    AtomImplPtr in the alt group is supposed to be modified here, or if this is
+    already taken care of in EntityImpl::UpdateTransformedPos()
+   */
+#if 0
+  geom::Transform tf = GetEntity()->GetTransform();
+  for(AtomEntryGroups::iterator git=alt_groups_.begin(); git!=alt_groups_.end();++git) {
+    for(AtomGroupEntryList::iterator lit=git->second.atoms.begin(); lit!=git->second.atoms.end();++lit) {
+      AtomImplPtr atom=lit->atom.lock().get();
+      geom::Vec3 tpos=tf.Apply(atom->OriginalPos());
+      atom->TransformedPos()=tpos;
+      lit->pos=atom->tpos;
+    }
+  }
+#endif
+}
+
 }}} // ns
diff --git a/modules/mol/base/src/impl/residue_impl.hh b/modules/mol/base/src/impl/residue_impl.hh
index 24f829c69..91b6843e7 100644
--- a/modules/mol/base/src/impl/residue_impl.hh
+++ b/modules/mol/base/src/impl/residue_impl.hh
@@ -223,6 +223,9 @@ public:
   
   bool IsLigand() const { return ligand_; }
   void SetIsLigand(bool flag) { ligand_=flag; }
+
+  void UpdateTransformedPos();
+
 private:
   void AddAltAtom(const String& group, const AtomImplPtr& atom,
                   const geom::Vec3& position, Real occ, Real b_factor);
diff --git a/modules/mol/base/src/xcs_editor.cc b/modules/mol/base/src/xcs_editor.cc
index 0c2454b18..5596e3962 100644
--- a/modules/mol/base/src/xcs_editor.cc
+++ b/modules/mol/base/src/xcs_editor.cc
@@ -74,14 +74,11 @@ void XCSEditor::SetAtomTransformedPos(const AtomHandle& atom,
                                       const geom::Vec3& position)
 {
   CheckHandleValidity(atom);
+  impl::EntityImplPtr eip=ent_.Impl();
   atom.Impl()->TransformedPos()=position;
-  if(ent_.Impl()->IsTransfIdentity()) {
-    atom.Impl()->OriginalPos()=position;
-  } else {
-    atom.Impl()->OriginalPos() = geom::Vec3(ent_.Impl()->GetInvTransfMatrix()*geom::Vec4(position));
-  }
-  ent_.Impl()->MarkICSDirty();
-  ent_.Impl()->MarkOrganizerDirty();
+  atom.Impl()->OriginalPos() = eip->HasTransform() ? eip->GetTransform().ApplyInverse(position) : position;
+  eip->MarkICSDirty();
+  eip->MarkOrganizerDirty();
   this->Update();
 }
 
@@ -89,17 +86,14 @@ namespace {
   template<typename T>
   void set_transformed_pos(impl::EntityImpl* ent, const AtomHandleList& alist, T *positions)
   {
-    bool has_tf=ent->IsTransfIdentity();
+    bool has_tf=ent->HasTransform();
     for(AtomHandleList::const_iterator ait=alist.begin();ait!=alist.end();++ait) {
       if(ait->IsValid()) {
-        ait->Impl()->TransformedPos()[0]=static_cast<Real>(positions[0]);
-        ait->Impl()->TransformedPos()[1]=static_cast<Real>(positions[1]);
-        ait->Impl()->TransformedPos()[2]=static_cast<Real>(positions[2]);
-        if(has_tf) {
-          ait->Impl()->OriginalPos()=ait->Impl()->TransformedPos();
-        } else {
-          ait->Impl()->OriginalPos() = geom::Vec3(ent->GetInvTransfMatrix()*geom::Vec4(ait->Impl()->TransformedPos()));
-        }
+        geom::Vec3& tpos=ait->Impl()->TransformedPos();
+        tpos[0]=static_cast<Real>(positions[0]);
+        tpos[1]=static_cast<Real>(positions[1]);
+        tpos[2]=static_cast<Real>(positions[2]);
+        ait->Impl()->OriginalPos()=has_tf ? ent->GetTransform().ApplyInverse(tpos) : tpos;
       }
       positions+=3;
     }
@@ -124,14 +118,11 @@ void XCSEditor::SetAtomOriginalPos(const AtomHandle& atom,
                                    const geom::Vec3& position)
 {
   CheckHandleValidity(atom);
+  impl::EntityImplPtr eip=ent_.Impl();
   atom.Impl()->OriginalPos()=position;
-  if(ent_.Impl()->IsTransfIdentity()) {
-    atom.Impl()->TransformedPos()=position;
-  } else {
-    atom.Impl()->TransformedPos() = geom::Vec3(ent_.Impl()->GetTransfMatrix()*geom::Vec4(position));
-  }
-  ent_.Impl()->MarkICSDirty();
-  ent_.Impl()->MarkOrganizerDirty();
+  atom.Impl()->TransformedPos() = eip->HasTransform() ? eip->GetTransform().Apply(position) : position;
+  eip->MarkICSDirty();
+  eip->MarkOrganizerDirty();
   this->Update();
 }
 
@@ -139,17 +130,14 @@ namespace {
   template<typename T>
   void set_original_pos(impl::EntityImpl* ent, const AtomHandleList& alist, T *positions)
   {
-    bool has_tf=ent->IsTransfIdentity();
+    bool has_tf=ent->HasTransform();
     for(AtomHandleList::const_iterator ait=alist.begin();ait!=alist.end();++ait) {
       if(ait->IsValid()) {
-        ait->Impl()->OriginalPos()[0]=static_cast<Real>(positions[0]);
-        ait->Impl()->OriginalPos()[1]=static_cast<Real>(positions[1]);
-        ait->Impl()->OriginalPos()[2]=static_cast<Real>(positions[2]);
-        if(has_tf) {
-          ait->Impl()->TransformedPos()=ait->Impl()->OriginalPos();
-        } else {
-          ait->Impl()->TransformedPos() = geom::Vec3(ent->GetTransfMatrix()*geom::Vec4(ait->Impl()->OriginalPos()));
-        }
+        geom::Vec3& opos=ait->Impl()->OriginalPos();
+        opos[0]=static_cast<Real>(positions[0]);
+        opos[1]=static_cast<Real>(positions[1]);
+        opos[2]=static_cast<Real>(positions[2]);
+        ait->Impl()->TransformedPos()= has_tf ? ent->GetTransform().Apply(opos) : opos;
       }
       positions+=3;
     }
@@ -186,6 +174,13 @@ void XCSEditor::SetAtomPos(const AtomHandleList& alist, double *positions)
 }
 
 void XCSEditor::ApplyTransform(const geom::Mat4& transform)
+{
+  geom::Transform tf;
+  tf.SetMatrix(transform);
+  this->ApplyTransform(tf);
+}
+
+void XCSEditor::ApplyTransform(const geom::Transform& transform)
 {
   ent_.Impl()->ApplyTransform(transform);
   ent_.Impl()->UpdateTransformedPos();
@@ -194,8 +189,14 @@ void XCSEditor::ApplyTransform(const geom::Mat4& transform)
   this->Update();
 }
 
-
 void XCSEditor::SetTransform(const geom::Mat4& transform)
+{
+  geom::Transform tf;
+  tf.SetMatrix(transform);
+  this->SetTransform(tf);
+}
+
+void XCSEditor::SetTransform(const geom::Transform& transform)
 {
   ent_.Impl()->SetTransform(transform);
   ent_.Impl()->UpdateTransformedPos();
@@ -204,7 +205,6 @@ void XCSEditor::SetTransform(const geom::Mat4& transform)
   this->Update();
 }
 
-
 void XCSEditor::Update()
 {
   if (GetMode()==UNBUFFERED_EDIT) {
diff --git a/modules/mol/base/src/xcs_editor.hh b/modules/mol/base/src/xcs_editor.hh
index 73d96a652..a7cc317b2 100644
--- a/modules/mol/base/src/xcs_editor.hh
+++ b/modules/mol/base/src/xcs_editor.hh
@@ -98,10 +98,11 @@ public:
   /// This transformation is applied \em after the transformation
   /// already stored in the entity
   void ApplyTransform(const geom::Mat4& transform); 
+  void ApplyTransform(const geom::Transform& transform); 
 
   /// \brief apply a new transformation to all atoms
-
   void SetTransform(const geom::Mat4& transform);
+  void SetTransform(const geom::Transform& transform);
 
   /// \brief immediately update internal coordinate system
   void UpdateICS();  
diff --git a/modules/mol/base/tests/test_entity.cc b/modules/mol/base/tests/test_entity.cc
index d8ecd8f3b..47ffcbd73 100644
--- a/modules/mol/base/tests/test_entity.cc
+++ b/modules/mol/base/tests/test_entity.cc
@@ -179,14 +179,12 @@ BOOST_AUTO_TEST_CASE(transformation)
   BOOST_CHECK(within_list1[0]==atom1);
   BOOST_CHECK(within_list1[1]==atom2);
 
-  BOOST_CHECK(eh.IsTransformationIdentity()==true);
+  BOOST_CHECK(eh.HasTransform()==false);
 
   geom::Transform trans;
   trans.ApplyZAxisRotation(90.0);
-  geom::Mat4 mat = trans.GetMatrix();
-
-  e.ApplyTransform(mat);
-  BOOST_CHECK(eh.IsTransformationIdentity()==false);
+  e.ApplyTransform(trans);
+  BOOST_CHECK(eh.HasTransform()==true);
 
   geom::Vec3 orig_atom1=geom::Vec3(1.0,0.0,0.0);
   geom::Vec3 orig_atom2=geom::Vec3(0.0,2.0,0.0);
@@ -205,11 +203,9 @@ BOOST_AUTO_TEST_CASE(transformation)
 
   geom::Transform trans2;
   trans2.ApplyXAxisTranslation(3.5);
-  geom::Mat4 mat2 = trans2.GetMatrix();
-
-  e.ApplyTransform(mat2);
+  e.ApplyTransform(trans2);
 
-  BOOST_CHECK(eh.IsTransformationIdentity()==false);
+  BOOST_CHECK(eh.HasTransform()==true);
 
   tr_atom1=geom::Vec3(3.5,-1.0,0.0);
   tr_atom2=geom::Vec3(5.5,0.0,0.0);
@@ -253,7 +249,7 @@ BOOST_AUTO_TEST_CASE(transformation)
 
   geom::Mat4 identity;
   e.SetTransform(identity);
-  BOOST_CHECK(eh.IsTransformationIdentity()==true);
+  //BOOST_CHECK(eh.HasTransform()==false);
 
   BondHandle bond1 = e.Connect(atom1,atom2);
   BondHandle bond2 = e.Connect(atom1,atom3);
@@ -261,9 +257,9 @@ BOOST_AUTO_TEST_CASE(transformation)
   BOOST_CHECK(bond1.GetLength()==1.5);
   BOOST_CHECK(bond2.GetLength()==2.0);
 
-  e.SetTransform(mat);
-
-  BOOST_CHECK(eh.IsTransformationIdentity()==false);
+  e.SetTransform(trans);
+  
+  //BOOST_CHECK(eh.HasTransform()==true);
 
   BOOST_CHECK(bond1.GetLength()==1.5);
   BOOST_CHECK(bond2.GetLength()==2.0);
-- 
GitLab