Problem 410

Summary: Wrong implementation of instance counting in G4AssemblyVolume
Product: Geant4 Reporter: novikova
Component: geometry/managementAssignee: Radovan.Chytracek
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 4.1   
Hardware: All   
OS: All   

Description novikova 2002-09-10 10:59:40 CEST
This is the same class where the bug #409 was reported (not all implementations
of MakeImprint() method were updating the imprints counter).  This time I found
the second bug:  the implementation of the counter of instances is incorrect.

Here is what's happening. The class G4AssemblyVolume has a private static
member "static unsigned int fsInstanceCounter."  Each time a new instance is
created, the variable fsInstanceCounter is incremented.  Thus, all instances of
the class have one and the same value of fsInstanceCounter (since it is
static). Specifically, when the "G4AssemblyVolume av1" is declared, it has the
fsInstanceCounter equal to 1, but the moment "G4AssemblyVolume av2" is created,
BOTH av1 and av2 have the SAME counter value equal to 2.  As a result, when two
(or more) assembly volumes are created and then the method MakeImprint() is
used to create imprints, the name of the physical volumes generated so all
start with the same "av_2."  The correct way would be for the imprints of the
first assembly volume to start with "av_1" and for the imprints of the second
volume to start with "av_2."

Here is the fastest (but not the most correct way) to fix the problem.

Introduce one more private variable to the class: put the following line into
the header of the class, into the private section:
  unsigned int fsInstanceIDofThisObject;
This variable is not static, it will hold the ID (i.e. the counter value) for
each instance of the class.  The old fsInstanceCounter variable will be keeping
track of which ID number was used last.

One more correction in the header is needed: a function that would report the
ID of an instance.  Hence, add the following line to the header's public
section:
  unsigned int GetInstanceID() const;

All other corrections have to be made in the source code, i.e.,
G4AssemblyVolume.cc file. Here are all those corrections listed:

1. Add the line
  fsInstanceIDofThisObject = fsInstanceCounter;
as the last line in the constructor.

2. In both implementations of MakeImprint() method, change the operator
    pvName << "av_"
           << GetInstanceCount()
           << "_impr_"
           << GetImprintsCount()
           << "_"
           << fTriplets[i].GetVolume()->GetName().c_str()
           << "_pv_"
           << i
           << G4std::ends;
to read as
    pvName << "av_"
           << GetInstanceID()
           << "_impr_"
           << GetImprintsCount()
           << "_"
           << fTriplets[i].GetVolume()->GetName().c_str()
           << "_pv_"
           << i
           << G4std::ends;
I.e., the private to each instance ID value should be used in forming the name
of the physical volume, and not the current number of assembly volumes in
existence (which is what you've get if GetInstanceCount() is used).

3. Add the implementation of the method GetInstanceID() to the source code (the
declaration of it was already added to the header):

unsigned int G4AssemblyVolume::GetInstanceID() const
{
  return fsInstanceIDofThisObject;
}

4. It probably makes sense to add the line
  fsInstanceIDofThisObject = fsInstanceCounter;
as the last line of SetInstanceCount() method. However, I personally do not
agree with the fact that fsInstanceCounter is forces to play a double role
here - not only does it have to count the number of instances, but it also has
to beware of the initial value to start with.  The SetInstanceCount(), and the
whole idea of using the same variable for two roles, creates potential for a
bug.

I implemented all the corrections described above in the version of code on my
hard drive, and tested it.  I recommend similar corrections to be performed in
the official version of Geant4.
Comment 1 Radovan.Chytracek 2002-09-10 12:11:59 CEST
Thanks for pointing that out. Fix on CVS HEAD.

Cheers
          Radovan

Apply cvs diff bellow if you don't have direct access to CVS:

Index: include/G4AssemblyVolume.hh
===================================================================
RCS
file: /afs/cern.ch/sw/geant4/cvs/geant4/source/geometry/management/include/G4Ass
emblyVolume.hh,v
retrieving revision 1.7
retrieving revision 1.8
diff -r1.7 -r1.8
24c24
< // $Id: G4AssemblyVolume.hh,v 1.7 2002/06/22 00:39:23 radoone Exp $
---
> // $Id: G4AssemblyVolume.hh,v 1.8 2002/09/10 16:59:44 radoone Exp $
175a176,178
>     // Return the number of existing instance of G4AssemblyVolume class
>   unsigned int GetAssemblyID()    const;
>     // Return instance number of this concrete object
179a183,184
>   void         SetAssemblyID( unsigned int value );
>
185a191,193
>   // Class instance counter
>          unsigned int fAssemblyID;
>   // Assembly object ID derived from instance counter at construction time
Index: include/G4AssemblyVolume.icc
===================================================================
RCS
file: /afs/cern.ch/sw/geant4/cvs/geant4/source/geometry/management/include/G4Ass
emblyVolume.icc,v
retrieving revision 1.3
retrieving revision 1.4
diff -r1.3 -r1.4
24c24
< // $Id: G4AssemblyVolume.icc,v 1.3 2001/07/11 09:59:16 gunter Exp $
---
> // $Id: G4AssemblyVolume.icc,v 1.4 2002/09/10 16:59:46 radoone Exp $
55a56,68
>
> inline
> unsigned int G4AssemblyVolume::GetAssemblyID() const
> {
>   return fAssemblyID;
> }
>
> inline
> void         G4AssemblyVolume::SetAssemblyID( unsigned int value )
> {
>   fAssemblyID = value;
> }
>
Index: src/G4AssemblyVolume.cc
===================================================================
RCS
file: /afs/cern.ch/sw/geant4/cvs/geant4/source/geometry/management/src/G4Assembl
yVolume.cc,v
retrieving revision 1.10
retrieving revision 1.13
diff -r1.10 -r1.13
24c24
< // $Id: G4AssemblyVolume.cc,v 1.10 2002/06/24 07:15:27 gcosmo Exp $
---
> // $Id: G4AssemblyVolume.cc,v 1.13 2002/09/10 17:07:15 radoone Exp $
44a45
> : fAssemblyID( 0 )
46a48
>   SetAssemblyID( GetInstanceCount() );
195c197
<            << GetInstanceCount()
---
>            << GetAssemblyID()
255a258,259
>   ImprintsCountPlus();
>
269c273
<            << GetInstanceCount()
---
>            << GetAssemblyID()