From 1803b25a5c1891e3c035e7c2c940492664cfd41c Mon Sep 17 00:00:00 2001
From: Andreas Salzburger <Andreas.Salzburger@cern.ch>
Date: Thu, 17 Nov 2016 16:06:18 +0100
Subject: [PATCH] New GeoID frame debugged

---
 Core/include/ACTS/Utilities/GeometryID.hpp |  34 +++---
 Core/include/ACTS/Utilities/Helpers.hpp    |   7 +-
 Core/src/Detector/TrackingVolume.cpp       |   1 +
 Tests/Utilities/GeometryID_tests.cpp       | 120 ++++++++++++++-------
 4 files changed, 108 insertions(+), 54 deletions(-)

diff --git a/Core/include/ACTS/Utilities/GeometryID.hpp b/Core/include/ACTS/Utilities/GeometryID.hpp
index f70d9ae4e..cd7af5c20 100644
--- a/Core/include/ACTS/Utilities/GeometryID.hpp
+++ b/Core/include/ACTS/Utilities/GeometryID.hpp
@@ -14,7 +14,7 @@
 #define ACTS_GEOMETRYUTILS_GEOMETRYID_H 1
 
 #include <iostream>
-#include "ACTS/Utilities/Helpers.h"
+#include "ACTS/Utilities/Helpers.hpp"
 
 namespace Acts {
 
@@ -31,27 +31,30 @@ typedef uint64_t geo_id_value;
 
 class GeometryID
 {
+    
 public:
-  const static geo_id_value volume_mask     = 0xff00000000000000;
-  const static geo_id_value boundary_mask   = 0x00ff000000000000;
-  const static geo_id_value layer_mask      = 0x0000ff0000000000;
-  const static geo_id_value approach_mask   = 0x000000ff00000000;
-  const static geo_id_value sensitive_mask  = 0x00000000ffff0000;
-  const static geo_id_value channel_mask    = 0x000000000000ffff;
+  const static geo_id_value volume_mask    = 0xff00000000000000;
+  const static geo_id_value boundary_mask  = 0x00ff000000000000;
+  const static geo_id_value layer_mask     = 0x0000ff0000000000;
+  const static geo_id_value approach_mask  = 0x000000ff00000000;
+  const static geo_id_value sensitive_mask = 0x00000000ffff0000;
+  const static geo_id_value channel_mask   = 0x000000000000ffff;
 
   /// default constructor
   ///
   GeometryID() : m_value(0) {}
+
   /// constructor from a ready-made value
   ///
   /// @param id_value is the full decoded value of the identifier
   GeometryID(geo_id_value id_value) : m_value(id_value) {}
+
   // constructor from a shift and a value
   ///
   /// @param id is numbered object
   /// @param type_mask is necessary for the decoding
-  GeometryID(geo_id_value type_id, geo_id_mask type_mask)
-    : m_value(ACTS_BIT_DECODE(type_id,type_mask))
+  GeometryID(geo_id_value type_id, geo_id_value type_mask)
+    : m_value(ACTS_BIT_ENCODE(type_id, type_mask))
   {
   }
 
@@ -59,6 +62,7 @@ public:
   ///
   /// @param tddID is the geometry ID that will be copied
   GeometryID(const GeometryID& tddID) : m_value(tddID.m_value) {}
+
   /// Assignement operator
   ///
   /// @param tddID is the geometry ID that will be assigned
@@ -88,11 +92,12 @@ public:
     m_value += add_value;
     return (*this);
   }
-  
-  /// Add sine stuff - a new 
-  void add(geo_id_value type_id, geo_id_mask type_mask)
+
+  /// Add some stuff - a new
+  void
+  add(geo_id_value type_id, geo_id_value type_mask)
   {
-    m_value += ACTS_BIT_DECODE(type_id,type_mask);
+    m_value += ACTS_BIT_ENCODE(type_id, type_mask);
   }
 
   /// return the value
@@ -109,7 +114,8 @@ private:
 inline geo_id_value
 GeometryID::value(geo_id_value mask) const
 {
-  if (mask) return ACTS_ENCODE(m_value, mask);
+  if (mask)
+      return ACTS_BIT_DECODE(m_value, mask);
   return m_value;
 }
 
diff --git a/Core/include/ACTS/Utilities/Helpers.hpp b/Core/include/ACTS/Utilities/Helpers.hpp
index f282b3157..2b79a988d 100644
--- a/Core/include/ACTS/Utilities/Helpers.hpp
+++ b/Core/include/ACTS/Utilities/Helpers.hpp
@@ -11,7 +11,7 @@
 ///////////////////////////////////////////////////////////////////
 
 #ifndef ACTS_HELPERS_H
-#define ACTS_HELPERS_H
+#define ACTS_HELPERS_H 1
 
 // STL include(s)
 #include <cmath>
@@ -25,14 +25,15 @@
 #ifndef ACTS_BIT_CODING
 #define ACTS_BIT_CODING 1
 #define ACTS_BIT_SHIFT(mask) (__builtin_ffsl(mask)-1)
-#define ACTS_BIT_DECODE(id, mask) id << ACTS_BIT_SHIFT(mask)
-#define ACTS_BIT_ENCODE(id, mask) (id && mask) >> ACTS_BIT_SHIFT(mask)
+#define ACTS_BIT_ENCODE(value, mask) (value << ACTS_BIT_SHIFT(mask))
+#define ACTS_BIT_DECODE(code, mask) ((code & mask) >> ACTS_BIT_SHIFT(mask))
 #endif 
 
 /** Geometry primitives helper functions
  */
 
 namespace Acts {
+        
 /** EventPrimitvesToStringConverter
 
     inline methods for conversion of EventPrimitives (Matrix)
diff --git a/Core/src/Detector/TrackingVolume.cpp b/Core/src/Detector/TrackingVolume.cpp
index b7c166770..28ef166d9 100644
--- a/Core/src/Detector/TrackingVolume.cpp
+++ b/Core/src/Detector/TrackingVolume.cpp
@@ -414,6 +414,7 @@ Acts::TrackingVolume::interlinkLayers()
 {
   if (m_confinedLayers) {
     auto& layers = m_confinedLayers->arrayObjects();
+      
     // forward register the last one as the previous one
     //  first <- | -> second, first <- | -> second, first <- | -> second
     const Layer* lastLayer = nullptr;
diff --git a/Tests/Utilities/GeometryID_tests.cpp b/Tests/Utilities/GeometryID_tests.cpp
index b6af87944..f34bdcafe 100644
--- a/Tests/Utilities/GeometryID_tests.cpp
+++ b/Tests/Utilities/GeometryID_tests.cpp
@@ -15,36 +15,77 @@
 namespace Acts {
 namespace Test {
 
-  /// prepare all the masks and shifts
-  std::vector<std::pair<geo_id_value,geo_id_value>> masks_range
-      = { {GeometryID::volume_mask,
-          64-ACTS_BIT_SHIFT(GeometryID::volume_mask)},
-          {GeometryID::boundary_mask,
-          ACTS_BIT_SHIFT(GeometryID::volume_mask-
-          ACTS_BIT_SHIFT(GeometryID::boundary_mask)},
-          {GeometryID::layer_mask,
-          ACTS_BIT_SHIFT(GeometryID::boundary_mask-
-          ACTS_BIT_SHIFT(GeometryID::layer_mask)},          
-          {GeometryID::approach_mask,
-          ACTS_BIT_SHIFT(GeometryID::layer_mask)-
-          ACTS_BIT_SHIFT(GeometryID::approach_mask)},    
-          {GeometryID::sensitive_mask,
-          ACTS_BIT_SHIFT(GeometryID::layer_mask)-
-          ACTS_BIT_SHIFT(GeometryID::sensitive_mask)},    
-         {GeometryID::channel_mask,
-          ACTS_BIT_SHIFT(GeometryID::sensitive_mask)-
-          ACTS_BIT_SHIFT(GeometryID::channel_mask)}};
+  geo_id_value volume_mask    = GeometryID::volume_mask;
+  geo_id_value boundary_mask  = GeometryID::boundary_mask;
+  geo_id_value layer_mask     = GeometryID::layer_mask;
+  geo_id_value approach_mask  = GeometryID::approach_mask;
+  geo_id_value sensitive_mask = GeometryID::sensitive_mask;
+  geo_id_value channel_mask   = GeometryID::channel_mask;
 
   /// test of the geometry ID creation and consistency of the ranges
   BOOST_AUTO_TEST_CASE(GeometryID_test)
   {
+    // create the volume shift and range
+    geo_id_value volume_shift = ACTS_BIT_SHIFT(volume_mask);
+    geo_id_value volume_range = 64 - ACTS_BIT_SHIFT(volume_mask);
+
+    geo_id_value boundary_shift = ACTS_BIT_SHIFT(boundary_mask);
+    geo_id_value boundary_range
+        = ACTS_BIT_SHIFT(volume_mask) - ACTS_BIT_SHIFT(boundary_mask);
+
+    geo_id_value layer_shift = ACTS_BIT_SHIFT(layer_mask);
+    geo_id_value layer_range
+        = ACTS_BIT_SHIFT(boundary_mask) - ACTS_BIT_SHIFT(layer_mask);
+
+    geo_id_value approach_shift = ACTS_BIT_SHIFT(approach_mask);
+    geo_id_value approach_range
+        = ACTS_BIT_SHIFT(layer_mask) - ACTS_BIT_SHIFT(approach_mask);
+
+    geo_id_value sensitive_shift = ACTS_BIT_SHIFT(sensitive_mask);
+    geo_id_value sensitive_range
+        = ACTS_BIT_SHIFT(approach_mask) - ACTS_BIT_SHIFT(sensitive_mask);
+
+    geo_id_value channel_shift = ACTS_BIT_SHIFT(channel_mask);
+    geo_id_value channel_range = ACTS_BIT_SHIFT(sensitive_mask)
+        - ACTS_BIT_SHIFT(GeometryID::channel_mask);
+
+    BOOST_CHECK_EQUAL(56lu, volume_shift);
+    BOOST_CHECK_EQUAL(8lu, volume_range);
+    BOOST_CHECK_EQUAL(48lu, boundary_shift);
+    BOOST_CHECK_EQUAL(8lu, boundary_range);
+    BOOST_CHECK_EQUAL(40lu, layer_shift);
+    BOOST_CHECK_EQUAL(8lu, layer_range);
+    BOOST_CHECK_EQUAL(32lu, approach_shift);
+    BOOST_CHECK_EQUAL(8lu, approach_range);
+    BOOST_CHECK_EQUAL(16lu, sensitive_shift);
+    BOOST_CHECK_EQUAL(16lu, sensitive_range);
+    BOOST_CHECK_EQUAL(0lu, channel_shift);
+    BOOST_CHECK_EQUAL(16lu, channel_range);
+
+    /// prepare all the masks and shifts
+    std::vector<std::pair<geo_id_value, geo_id_value>> masks_range
+        = {{volume_mask, volume_range},
+           {boundary_mask, boundary_range},
+           {layer_mask, layer_range},
+           {approach_mask, approach_range},
+           {sensitive_mask, sensitive_range},
+           {channel_mask, channel_range}};
+
+    size_t test = 0;
     for (auto msr : masks_range) {
+
       auto mask  = msr.first;
       auto range = msr.second;
+      ///
+
       /// test the full range of ids
       for (geo_id_value idv = 0; idv < pow(2, range); ++idv) {
-        /// create the geometry ID
+        // create the geometry ID
         GeometryID geoID(idv, mask);
+        // encode - decode test
+        BOOST_CHECK_EQUAL(idv,
+                          (ACTS_BIT_DECODE(ACTS_BIT_ENCODE(idv, mask), mask)));
+        // geo id decoding
         BOOST_CHECK_EQUAL(idv, geoID.value(mask));
       }
     }
@@ -60,14 +101,29 @@ namespace Test {
     GeometryID approachID(4, GeometryID::approach_mask);
     GeometryID sensitiveID(5, GeometryID::sensitive_mask);
     GeometryID channelID(6, GeometryID::channel_mask);
+
+    // now check the validity before adding
+    BOOST_CHECK_EQUAL(1lu, volumeID.value(GeometryID::volume_mask));
+    BOOST_CHECK_EQUAL(2lu, boundaryID.value(GeometryID::boundary_mask));
+    BOOST_CHECK_EQUAL(3lu, layerID.value(GeometryID::layer_mask));
+    BOOST_CHECK_EQUAL(4lu, approachID.value(GeometryID::approach_mask));
+    BOOST_CHECK_EQUAL(5lu, sensitiveID.value(GeometryID::sensitive_mask));
+    BOOST_CHECK_EQUAL(6lu, channelID.value(GeometryID::channel_mask));
+
     // now create a compound ones
     GeometryID compoundID_dconst;
     compoundID_dconst += volumeID;
     GeometryID compoundID_cconst(volumeID);
     GeometryID compoundID_assign = volumeID;
-    //
+
     std::vector<GeometryID> compoundIDs
         = {compoundID_dconst, compoundID_cconst, compoundID_assign};
+
+    /// check the validity after assigning/copying/constructing
+    BOOST_CHECK_EQUAL(1lu, compoundID_dconst.value(GeometryID::volume_mask));
+    BOOST_CHECK_EQUAL(1lu, compoundID_cconst.value(GeometryID::volume_mask));
+    BOOST_CHECK_EQUAL(1lu, compoundID_assign.value(GeometryID::volume_mask));
+
     for (auto& cid : compoundIDs) {
       // add the sub IDs
       cid += boundaryID;
@@ -76,22 +132,12 @@ namespace Test {
       cid += sensitiveID;
       cid += channelID;
       // now check the cid
-      BOOST_CHECK_EQUAL(
-          1lu, 
-          cid.value(GeometryID::volume_mask));
-      BOOST_CHECK_EQUAL(
-          2lu,
-          cid.value(GeometryID::boundary_mask));
-      BOOST_CHECK_EQUAL(
-          3lu, cid.value(GeometryID::layer_mask));
-      BOOST_CHECK_EQUAL(
-          4lu,
-          cid.value(GeometryID::approach_mask));
-      BOOST_CHECK_EQUAL(
-          5lu,
-          cid.value(GeometryID::sensitive_mask));
-      BOOST_CHECK_EQUAL(
-          6lu, cid.value(GeometryID::channel_mask));
+      BOOST_CHECK_EQUAL(1lu, cid.value(GeometryID::volume_mask));
+      BOOST_CHECK_EQUAL(2lu, cid.value(GeometryID::boundary_mask));
+      BOOST_CHECK_EQUAL(3lu, cid.value(GeometryID::layer_mask));
+      BOOST_CHECK_EQUAL(4lu, cid.value(GeometryID::approach_mask));
+      BOOST_CHECK_EQUAL(5lu, cid.value(GeometryID::sensitive_mask));
+      BOOST_CHECK_EQUAL(6lu, cid.value(GeometryID::channel_mask));
     }
   }
 
-- 
GitLab