Consider Dropping `TNamed` inheritance from `TVirtualMCSensitiveDetector`

[ChristianTackeGSI]

We are currently investigating some (internal) redesign. For this we consider multiple inheritance. Simplified:

• Class M1 inherits from nothing
• Class M2 (basically FairModule) inherits from TNamed
• Class D1 inherits from M1 and TVirtualMCSensitiveDetector
• Class D2 (basically FairDetector) inherits from M2 and TVirtualMCSensitiveDetector

classDiagram
  M1 <|-- D1
  TVirtualMCSensitiveDetector <|-- D1
  TNamed <|-- TVirtualMCSensitiveDetector

  TNamed_ <|-- M2
  M2 <|-- D2
  TVirtualMCSensitiveDetector <|-- D2

Case D1 is simple and will just work.

But case D2 has a problem. There are two ways to reach TNamed. I think this is called diamond inheritance. Currently D2 will have two copies of TNamed (TNamed and TNamed_ in the diagram). Not really, what we want.

1. The C++ answer to this problem is called virtual base classes. These come with some performance overhead when accessing TNamed member variables (might or might not be relevant. We haven't checked).

   classDiagram
    M1 <|-- D1
    TVirtualMCSensitiveDetector <|-- D1
    TNamed <|-- TVirtualMCSensitiveDetector
   
    TNamed <|-- M2
    M2 <|-- D2
    TVirtualMCSensitiveDetector <|-- D2

2. Another question is, whether vmc, geamt4_vmc and geant3 actually depend on TVirtualMCSensitiveDetector deriving from TNamed? If none of them depend on this base class, it maybe could be dropped from TVirtualMCSensitiveDetector? This would solve D2 and even improve D1, because the whole TNamed thing would be gone.

   classDiagram
    M1 <|-- D1
    TVirtualMCSensitiveDetector <|-- D1
   
    TNamed <|-- M2
    M2 <|-- D2
    TVirtualMCSensitiveDetector <|-- D2

What do you think?

cc: @dennisklein

[ChristianTackeGSI]

Note: We would like to avoid M1, and/or M2 inheriting from TVirtualMCSensitiveDetector.

But even then, removing TNamed from TVirtualMCSensitiveDetector would simplify D1.

classDiagram
  TVirtualMCSensitiveDetector <|-- M1
  M1 <|-- D1

  TNamed <|-- M2
  TVirtualMCSensitiveDetector <|-- M2
  M2 <|-- D2

[ihrivnac]

Hi @ChristianTackeGSI ,

The inheritance from TObject is needed in the geant3 implementation, where we use it in associating the user SD with TGeoVolume via TGeoRCExtension; see:

https://github.com/vmc-project/geant3/blob/master/TGeant3/TGeant3TGeo.cxx#L2189

Reducing TNamed to TObject would not help you in your problem.

And virtual inheritance cannot be used as it would prevent from restoring TVirtualMCSensitiveDetector from TObject:

[  0%] Building CXX object CMakeFiles/geant321.dir/TGeant3/TGeant3TGeo.cxx.o
/Users/ivana/work/projects/vmc/dev/geant3/TGeant3/TGeant3TGeo.cxx:2232:11: error: cannot cast 'TObject *' to 'TVirtualMCSensitiveDetector *' via virtual base 'TNamed'
   return static_cast<TVirtualMCSensitiveDetector *>(static_cast<TGeoRCExtension *>(extension)->GetUserObject());
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I think that the best approach in your case could be to use composition instead of multiple inheritance and have M1 : TNamed containing D1 : TVirtualMCSensitiveDetector etc. (unless you come with other working solution).

Best regards,

[dennisklein]

--- a/TGeant3/TGeant3TGeo.cxx
+++ b/TGeant3/TGeant3TGeo.cxx
@@ -2229,7 +2229,7 @@ TVirtualMCSensitiveDetector *TGeant3TGeo::GetCurrentSensitiveDetector() const
       return 0;

    // Using static cast require exclusive use of TGeoVolume extension for VMC senistive detectors
-   return static_cast<TVirtualMCSensitiveDetector *>(static_cast<TGeoRCExtension *>(extension)->GetUserObject());
+   return dynamic_cast<TVirtualMCSensitiveDetector *>(static_cast<TGeoRCExtension *>(extension)->GetUserObject());
 }
 #else

should make it compile. But I guess the takeaway is, that adding a virtual base class later on, is just a breaking change in general...

[ChristianTackeGSI]

Hi @ChristianTackeGSI ,

The inheritance from TObject is needed in the geant3 implementation, where we use it in associating the user SD with TGeoVolume via TGeoRCExtension; see:

https://github.com/vmc-project/geant3/blob/master/TGeant3/TGeant3TGeo.cxx#L2189

Hmmm, looking at this, I wonder, whether one could rewrite that part to not use TGeoRCExtension, but some TGeant3GeoExtension that is a very simplified version of TGeoRCExtension without the Reference Counting and a TVirtualMCSensitiveDetector pointer instead?

[…]

I think that the best approach in your case could be to use composition instead of multiple inheritance […]

Yeah, using composition in some way was on our alternative list as well.

[ihrivnac]

dynamic_cast should make it compile. But I guess the takeaway is, that adding a virtual base class later on, is just a breaking change in general...

Yes, it does, but it does not solve the problem either.

Hmmm, looking at this, I wonder, whether one could rewrite that part to not use TGeoRCExtension, but some TGeant3GeoExtension that is a very simplified version of TGeoRCExtension without the Reference Counting and a TVirtualMCSensitiveDetector pointer instead?

We need to have the extension attached directly to TGeoVolume to profit from the direct access. For this we need to use what the TGeoVolume class provides. We have already an alternative implementation in TGeant3 using a map, but this is less efficient as we have to retrieve the SD from the map.