Fix write and read of GeoFullPhysVol as root volume

This MR fixes #84 (closed).

While trying to read back a .db file in which an instance of GeoFullPhysVol was used as the Root volume, @todorova got a crash with this error message:

ERROR! A pointer to a GeoGraphNode instance is NULL {root getRootVolume [GeoPhysVol* GeoModelIO::ReadGeoModel::getRootVolume()]}. Exiting...ERROR! A pointer to a GeoGraphNode instance is NULL {root getRootVolume [GeoPhysVol* GeoModelIO::ReadGeoModel::getRootVolume()]}. Exiting...

The error came from GeoModelIO/GeoModelRead.

I investigated and found that ReadGeoModel crashed when trying to perform a dynamic cast to GeoPhysVol when retrieving the Root volume with the getRootVolume method.

All methods handling the Root volume, in fact, only handled instances of GeoPhysVol, and that was for historical reasons only: when I started to develop GeoModelIO, I initially had Root volumes as GeoPhysVol and never modified them.

But, of course, a GeoFullPhysVol can very correctly be used as the Root/World volume, so I modified the methods related to the Root to handle pointers to GeoVPhysVol: in that way, instances of both GeoPhysVol and GeoFullPhysVol can now be safely used as Root volumes.

There was also another problem in the writing. In GeoModelIO/GeoModelWrite, the method that stores an instance of GeoFullPhysVol as the Root volume in the .db file was wrongly storing the node as a GeoPhysVol: it was clearly a typo left by poor copy-paste from my side, when copying from the method handling the GeoPhysVol. This MR fixes that as well.

Also, the HelloGeoWrite and HelloGeoRead examples have been updated as well, in this MR, to reflect the above changes better.

---

Cc: @todorova @tsulaia @boudreau

added GeoModelExamples GeoModelIO labels

mentioned in issue #84 (closed)

Hi @rbianchi, thanks for the fix! I'm about to start the procedure of switching Athena to GeoModel 4.5.0. This means that the changes from this MR will not be visible to Athena at this point, but I guess that's fine.

Another minor comment: could you perhaps change the color of the GeoModelExamples label? White text is not really readable on the shiny yellow background. Thanks!

enabled an automatic merge when the pipeline for 5c71ced1 succeeds

resolved all threads

marked this merge request as draft

Marked as Draft, since the change of interface to GeoVPhysVol is needed in many packages and many changes are needed in Tools, Visualization, FullSimLight, GMX, Plugins, and so on.

This update is very similar to another that I have still sitting in a branch:

67a61662

The issue is very similar, but the impact is quite different. The detector plugin and detector factories are written to the GeoFullPhyVol interface, rather than the GeoVPhysVol interface and that limits their usefulness; there is no reason why these objects should create only ordinary phys vols.

The impact of changes in my branch is on all detector factories both on the Athena side and on the GeoModelATLAS side. That's why I did not request a merge.

What do we think about merging both of these updates at the same time? We could immediately adapt the GeoModelATLAS code. We could then adapt the Athena side code when we switch athena to a new release.

Probably we should issue email warnings if we follow this suggestion.

aborted the automatic merge because source branch was updated

added 4 commits

• ba0dcebe - code indenting
• c922ec5b - code indenting
• ca11a96b - code indenting
• 9d35f036 - Fix use of FullPhysVol as Root/World volume.

Compare with previous version

added FullSimLight GDMLtoGM GeoModelCore GeoModelTools GeoModelVisualization labels

added 7 commits

• 9f3eb746 - code indenting
• 0e92aaf9 - Update World volume interface to GeoVPhysVol
• 392297f0 - code indenting
• e3bca61a - Update World volume interface to GeoVPhysVol
• a9f0393e - code indenting
• 46ec5619 - Update World volume interface to GeoVPhysVol
• c9c4abe3 - Update World volume interface to GeoVPhysVol

Compare with previous version

resolved all threads

The CI is now full green.

I have updated all packages built when enabling:

-DGEOMODEL_BUILD_ALL=1 -DGEOMODEL_BUILD_EXAMPLES=1

So, everything looks to be working now, at least at compilation time.

For better testing and merging, I think we could try to modify the GeoModelATLAS CI pipeline to add an option to run, on request, the full pipeline on a custom-defined branch, which could be, let's say, a testing branch in GeoModelDev. In that way, every time we have major changes that are likely to disrupt GeoModelATLAS, we could first test the pipeline over the test branch, before actually merging the changes into the GeoModel main. We can discuss

We could discuss this in our next meeting, what do you think @tsulaia and @boudreau ?

resolved all threads

UPDATE: I have updated the plugins in GeoModelATLAS and created a MR, which should be merged only after this MR is merged:

https://gitlab.cern.ch/atlas/geomodelatlas/GeoModelATLAS/-/merge_requests/95

@tsulaia, if you think we're fine with these changes, feel free to merge this and the other MR.

Hi @rbianchi, I'm fine with this MR in general, although here I have a similar comment as the one I posted to GeoModelATLAS!95. It would be good to not mix the formatting code changes with "real" code changes.

Once you address my comments on GeoModelATLAS!95, could you please un-draft it and then merge both MR-s so that the main branches of GeoModel and GeoModelATLAS repositories remain compatible?

Thanks.

@tsulaia , I'll decouple the changes and update the MR. Then, I'll undraft and merge both of them, as you suggested. Thanks, --Ric.

marked this merge request as ready

changed target branch from main to master