Fix mapping of reflected volumes to VecGeom logical volume instances

[sethrj]

This adds the reflected (rather than solely the constituent) volumes to the VecGeom -> Geant4 map. It still leaves the mapping in place that points to the unreflected ("constituent") volumes from the reflected VecGeom LVs.

@JuanGonzalezCaminero
Fixes #16 .

[JuanGonzalezCaminero]

I have tested with cms-me11.gdml but I seem to still be getting the same mappings as before with map_reflected=true. I will try to reproduce the specific issue I'm seeing with the new test cases

[drbenmorgan]

Happy to check basics and do the eventual merge, but @JuanGonzalezCaminero, @agheata should give final approval to merge as they're best placed to confirm this fixes the Issue!

[sethrj]

@JuanGonzalezCaminero I'm continuing to dig into this and I think the problem may be that the physical rather than logical volumes aren't being correctly mapped from VecGeom to Geant4...

[JuanGonzalezCaminero]

Thanks Seth, I think I have an idea of what's happening now. During conversion of course the deepest volumes are placed first, and they are mapped just after they are placed:
https://github.com/celeritas-project/g4vg/blob/main/src/g4vg_impl/Converter.cc#L144

However, if their mother is reflected, the vecgeom reflection factory will go over the daughters, apply the transformation, and actually replace them with a new volume, which will have a different ID:
https://gitlab.cern.ch/VecGeom/VecGeom/-/blob/master/source/ReflFactory.cpp#L160
https://gitlab.cern.ch/VecGeom/VecGeom/-/blob/master/source/ReflFactory.cpp#L209

So I believe this may be the reason why the original mappings don't work

[sethrj]

So maybe instead of

(*placed_pv_)[id] = g4pv

we also need to look up the g4pv in the reflection factory and use the equivalent there?

EDIT: actually you can't look up the physical volume. Maybe @agheata can help us out? He certainly understands the reflection process better than I do.

[JuanGonzalezCaminero]

I think mapping the volumes that way is fine since it works for all volumes that don't have a reflection in their path. If a volume is reflected we could propagate that information and skip mapping any placements below them.

Then, we probably need to visit and map everything below when we place a reflected volume, only in the first reflection in the path to avoid visiting the same subtree multiple times.

[sethrj]

@JuanGonzalezCaminero I think I've got it working with your and @agheata 's suggestion. The only weirdness is that capacities for the reflected volumes show as negative, but I think this is just a bug in VecGeom. (Previously we were using the underlying volume so we wouldn't have seen the scaled volume's representation, and the capacity implementation multiplies by the scaling factor products which includes the -1 factor.)

[JuanGonzalezCaminero]

I can confirm that this fixes the issue!

While testing it I noticed that the g4vg tests fail to link when using -DBUILD_SHARED_LIBS=ON (Since we build it as shared for linking with AdePT). It doesn't seem like a big issue since one can build as static for testing, just mentioning it here.

[sethrj]

@JuanGonzalezCaminero That's great! Can you please take a moment to review the code so I can merge? Also, can you describe your link error? I seem to have no problem building with shared libs (though I generally develop without CUDA...)

[sethrj]

And the CI also builds with shared libs plus CUDA and has no issues...

[JuanGonzalezCaminero]

The errors don't give much information unfortunately, it fails to find calls from vecgeomcuda_static. It may depend on the OS as well, I'm on debian right now.

[sethrj]

OK that sounds like some RDC stuff @pcanal ...