Project

General

Profile

Bug #14384

Copy of the Geometry object does not behave as the original one

Added by Gianluca Petrillo almost 3 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Category:
Geometry
Target version:
-
Start date:
11/03/2016
Due date:
% Done:

100%

Estimated time:
Spent time:
Occurs In:
Experiment:
-
Co-Assignees:
Duration:

Description

From David Adams, on issue #14362#note-3:

While modifying DuneApaChannelMapAlg, I discovered that I get different results (when calling Geometry::WireCoordinate) if I make a copy of the wire geometry. To reproduce this, enable the #if statement at line 89. I don't know where the change is but if copying the geometry changes the original, we should probably delete the copy operator.


Related issues

Copied from dunetpc - Feature #14362: Add ROP support to 10 kt geometryClosed2016-11-02

Associated revisions

Revision 0a103daf (diff)
Added by Gianluca Petrillo almost 2 years ago

Replaced internal storage of wires in geo::PlaneGeo.

This is in the context of issue #14384.

Revision 44880e2f (diff)
Added by Gianluca Petrillo almost 2 years ago

Changed the internal storage of planes in geo::TPCGeo.

Related to issue #14384.

Revision 7b76ebbc (diff)
Added by Gianluca Petrillo almost 2 years ago

Changed the internal storage of TPCs in geo::CryostatGeo.

Related to issue #14384.

Revision 95851dff (diff)
Added by Gianluca Petrillo almost 2 years ago

Changed the internal storage of optical detectors in geo::CryostatGeo.

Related to issue #14384.

Revision cb195cc6 (diff)
Added by Gianluca Petrillo almost 2 years ago

Changed the internal storage of cryostats in geo::GeometryCore.

Related to issue #14384.

Revision ba57c4a8 (diff)
Added by Gianluca Petrillo almost 2 years ago

Removed broken geo::GeometryCore::AuxDetGeoVec()

Related to issue #14384.

Revision 265db500 (diff)
Added by Gianluca Petrillo almost 2 years ago

Updated the comment on GeometryCore deleted constructors.

This concludes the resolution of issue #14384.

History

#1 Updated by Gianluca Petrillo almost 3 years ago

#2 Updated by Gianluca Petrillo almost 3 years ago

  • Description updated (diff)
  • Status changed from New to Assigned
  • Assignee set to Gianluca Petrillo

Disabling the copy is definitely an option, but the behaviour you describe is unexpected to me.
I want to investigate it, in case it hides some deeper problem.

#3 Updated by Gianluca Petrillo almost 3 years ago

  • Description updated (diff)

#4 Updated by Gianluca Petrillo almost 2 years ago

  • Status changed from Assigned to Resolved
  • % Done changed from 0 to 100
Back from ice lands, I have taken a deep look at the issue.
  • geo::GeometryCore is not copiable -- copy has been disabled ages ago with larcorealg:dfa78e6
  • the geometry objects it hosts allow copy, but their default copy/move constructors fail because the objects store bare pointers to the sub-components they manage:
    • geo::PlaneGeo contains a vector of bare pointers to geo::WireGeo
    • geo::TPCGeo contains a vector of bare pointers to geo::PlaneGeo
    • geo::CryostatGeo contains vectors of bare pointers to geo::TPCGeo and geo::OpDetGeo
    • geo::GeometryCore contains vectors of bare pointers to geo::CryostatGeo and geo::AuxDetGeo
  • some of the objects also contain the coordinate transformation matrix as a bare pointer

Bare pointers in this context are a design flaw. Since the contained objects are not polymorphic, direct containment is enough. If they had been polymorphic, ownership should have been explicitly stated by the use of std::unique_ptr.
I have replaced the bare pointers to geo::WireGeo, geo::PlaneGeo, geo::TPCGeo and geo::OpDetGeo. These changes are transparent to the users because the internal storage structure is never exposed (which is good!).
I have also replaced the bare pointers to geo::CryostatGeo. Unfortunately, the storage of cryostats from geo::GeometryCore is exposed so that geo::ChannelMapAlg::initialize() can receive it. This is a consequence of a shortcoming of the design of the geometry initialization, that has some circular dependences. My change affects geo::ChannelMapAlg interface and all the classes derived from it: it is therefore a breaking change. The resolution, documented in the Breaking changes page, is quite straightforward (where there used to be pointers, now there are constant references).

I have not replaced the bare pointers to geo::AuxDetGeo objects. The design of this feature is flawed enough that changing it will require more extensive changes.
Part of the flaw is exposed by geo::GeometryCore::AuxDetGeoVec() method, which even being constant provides non-constant pointers to AuxDetGeo objects of the geometry. Since this method can be replaced by others in order to access to the auxiliary detectors, it has been removed: this is another breaking change.

Transformation matrices have been turned into direct data members rather than dynamically allocated as they were.

With the breaking changes, geo::WireGeo, geo::PlaneGeo, geo::TPCGeo, geo:OpDetGeo and geo::CryostatGeo are expected to be copy- and move-safe, although copying those objects is still not recommended. Copy and move of geo::GeometryCore are still disabled, and the object is not copy-safe until the issue with the auxiliary detectors is addressed.

The changes have been provided in branch feature/gp_Issue14384 of larcorealg repository.
Fixes for the breaking changes are provided in branch feature/gp_Issue14384 of experiment repositories dunetpc, lariatcode, sbndcode and icaruscode.
All of them are updated to the current develop branch, and all of them except the latter are updated to LArSoft v06_51_00.

#5 Updated by Gianluca Petrillo almost 2 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF