From 07ad64dae5074f26dc30e1022664cbf3a2de4071 Mon Sep 17 00:00:00 2001
From: John Derek Chapman <chapman@hep.phy.cam.ac.uk>
Date: Fri, 23 Sep 2022 17:49:34 +0200
Subject: [PATCH] Workaround feature in HepMC3::GenVertex::position()

Workaround feature in HepMC3::GenVertex::position()

In HepMC3, if a GenVertex is positioned at the origin, then it is treated as not having its
position set. In such a case, the method will loop over the production vertices of particles
incoming to the current vertex and return their positions if possible.

In the case of events generated at the origin, then many of the GenVertex objects end up being
flagged as not having their positions set.

Once the position of a GenVertex is reset by the vertex positioning code, calling `position()`
on any "downstream" GenVertex objects which were positioned at the origin, then returns the "upstream"
GenVertex (already-shifted) position, the shift is then applied to this causing the positions of these
"downstream" GenVertex objects to effectively be shifted twice, and so on.

The workaround is to avoid calling `GenVertex::position()` in the case that `GenVertex::has_set_position()`
returns `false` and directly use `HepMC::FourVector::ZERO_VECTOR()` as the GenVertex position.
---
 .../BeamEffects/src/GenEventVertexPositioner.cxx    |  9 ++++++++-
 .../BeamEffects/src/ZeroLifetimePositioner.cxx      | 10 ++++++++--
 .../ISF/ISF_Core/ISF_Services/src/TruthSvc.cxx      | 13 ++++++++++++-
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/Simulation/BeamEffects/src/GenEventVertexPositioner.cxx b/Simulation/BeamEffects/src/GenEventVertexPositioner.cxx
index 7aa9d9821592..3604a1a0e123 100644
--- a/Simulation/BeamEffects/src/GenEventVertexPositioner.cxx
+++ b/Simulation/BeamEffects/src/GenEventVertexPositioner.cxx
@@ -65,7 +65,7 @@ namespace Simulation
         continue;
       }
 
-      ATH_MSG_VERBOSE("Retrieved Vertex shift of: " << *curShift);
+      ATH_MSG_VERBOSE("Retrieved Vertex shift of: " << *curShift << " from " << vertexShifter->name());
 
       // As signal process vertex is a pointer, there is some risk
       // that the pointer points to a vertex somewhere else in the
@@ -86,7 +86,14 @@ namespace Simulation
       for( ; vtxIt != vtxItEnd; ++vtxIt) {
         // quick access:
         auto curVtx = (*vtxIt);
+#ifdef HEPMC3
+        // NB Doing this check to explicitly avoid the fallback mechanism in
+        // HepMC3::GenVertex::position() to return the position of
+        // another GenVertex in the event if the position isn't set (or is set to zero)!
+        const HepMC::FourVector &curPos = (curVtx->has_set_position()) ? curVtx->position() : HepMC::FourVector::ZERO_VECTOR();
+#else
         const HepMC::FourVector &curPos = curVtx->position();
+#endif
 
         // get a copy of the current vertex position
         CLHEP::HepLorentzVector newPos( curPos.x(), curPos.y(), curPos.z(), curPos.t() );
diff --git a/Simulation/BeamEffects/src/ZeroLifetimePositioner.cxx b/Simulation/BeamEffects/src/ZeroLifetimePositioner.cxx
index 475f1cb70e83..5e5c50e98891 100644
--- a/Simulation/BeamEffects/src/ZeroLifetimePositioner.cxx
+++ b/Simulation/BeamEffects/src/ZeroLifetimePositioner.cxx
@@ -81,7 +81,10 @@ StatusCode Simulation::ZeroLifetimePositioner::manipulate(HepMC::GenEvent& ge, b
     if (ATH_UNLIKELY(this->msgLvl (MSG::VERBOSE))) {
       HepMC::Print::line(nextVtx);
     }
-    const HepMC::FourVector &nextVec = nextVtx->position();
+    // NB Doing this check to explicitly avoid the fallback mechanism in
+    // HepMC3::GenVertex::position() to return the position of
+    // another GenVertex in the event if the position isn't set (or is set to zero)!
+    const HepMC::FourVector &nextVec = (nextVtx->has_set_position()) ? nextVtx->position() : HepMC::FourVector::ZERO_VECTOR();
     const CLHEP::HepLorentzVector nextPos( nextVec.x(), nextVec.y(), nextVec.z(), nextVec.t() );
     ATH_MSG_VERBOSE("Current Vertex:");
     if (ATH_UNLIKELY(this->msgLvl (MSG::VERBOSE))) {
@@ -93,7 +96,10 @@ StatusCode Simulation::ZeroLifetimePositioner::manipulate(HepMC::GenEvent& ge, b
       if (ATH_UNLIKELY(this->msgLvl (MSG::VERBOSE))) {
         HepMC::Print::line(prevVtx);
       }
-      const HepMC::FourVector &prevVec = prevVtx->position();
+      // NB Doing this check to explicitly avoid the fallback mechanism in
+      // HepMC3::GenVertex::position() to return the position of
+      // another GenVertex in the event if the position isn't set (or is set to zero)!
+      const HepMC::FourVector &prevVec = (prevVtx->has_set_position()) ? prevVtx->position() : HepMC::FourVector::ZERO_VECTOR();
       const CLHEP::HepLorentzVector prevPos( prevVec.x(), prevVec.y(), prevVec.z(), prevVec.t() );
       CLHEP::HepLorentzVector newPos = 0.5*(prevPos+nextPos);
       curVtx->set_position(HepMC::FourVector(newPos.x(),newPos.y(),newPos.z(),newPos.t()));
diff --git a/Simulation/ISF/ISF_Core/ISF_Services/src/TruthSvc.cxx b/Simulation/ISF/ISF_Core/ISF_Services/src/TruthSvc.cxx
index 0e46a58985c0..30b80472263e 100644
--- a/Simulation/ISF/ISF_Core/ISF_Services/src/TruthSvc.cxx
+++ b/Simulation/ISF/ISF_Core/ISF_Services/src/TruthSvc.cxx
@@ -267,7 +267,11 @@ void ISF::TruthSvc::recordIncidentToMCTruth( ISF::ITruthIncident& ti, bool passW
       // 2) A new GenVertex for the intermediate interaction should be
       // added.
 #ifdef HEPMC3
-      auto newVtx = HepMC::newGenVertexPtr( vtx->position(), vtx->status());
+      // NB Doing this check to explicitly avoid the fallback mechanism in
+      // HepMC3::GenVertex::position() to return the position of
+      // another GenVertex in the event if the position isn't set (or is set to zero)!
+      const HepMC::FourVector &posVec = (vtx->has_set_position()) ? vtx->position() : HepMC::FourVector::ZERO_VECTOR();
+      auto newVtx = HepMC::newGenVertexPtr( posVec, vtx->status());
       HepMC::GenEvent *mcEvent = parentBeforeIncident->parent_event();
       auto tmpVtx = newVtx;
       mcEvent->add_vertex( newVtx);
@@ -448,7 +452,14 @@ HepMC::GenVertexPtr  ISF::TruthSvc::createGenVertexFromTruthIncident( ISF::ITrut
       this->deleteChildVertex(oldVertex);
     }
     else {
+#ifdef HEPMC3
+      // NB Doing this check to explicitly avoid the fallback mechanism in
+      // HepMC3::GenVertex::position() to return the position of
+      // another GenVertex in the event if the position isn't set (or is set to zero)!
+      const HepMC::FourVector &old_pos = (oldVertex->has_set_position()) ? oldVertex->position() : HepMC::FourVector::ZERO_VECTOR();
+#else
       const auto& old_pos=oldVertex->position();
+#endif
       const auto& new_pos=ti.position();
       double diffr=std::sqrt(std::pow(new_pos.x()-old_pos.x(),2)+std::pow(new_pos.y()-old_pos.y(),2)+std::pow(new_pos.z()-old_pos.z(),2));
       //AV The comparison below is not portable.
-- 
GitLab