Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated R3BDetector to use better position handling, use constructor … #147

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

janmayer
Copy link
Contributor

…delegation in some derived detectors, remove unnecessary construct geometry calls.

@janmayer
Copy link
Contributor Author

Note: Something similar is necessary in R3BModule, however I want to await response to this commit.

Also it some files are in dire need for some clang-format, or at least some white-space cleaning...

@kresan
Copy link
Contributor

kresan commented Jun 26, 2017

Nice update, simplifies a lot.
Please adopt the macros run_sim.C and run_aladin_sim.C (supposed to be a new standard) to work with your changes. Would be also interested to see how the usage of constructors looks like.
Best regards,
Dima

…delegation in some derived detectors, remove unnecessary construct geometry calls
@janmayer
Copy link
Contributor Author

janmayer commented Jul 5, 2017

Okay, this turned out larger than I expected. Because I ran clang-format on every file I touched, the number of lines changed is a bit inflated, but it had to be done. Most changes are straightforward and effectively the same in all derived classes.

The main changes are in the constructors of R3BDetector, R3BModule and everything derived from that. Best point to look at is, due to its small number of changes, the dTof detector (R3BdTof.h R3BdTof.cxx).

Ideas influencing the change:

  • There is no reason having to give the name of the detector in a specialized, derived detector class constructor. A default is provided. If this really needs to be changed, use SetName() before adding it as a module.
  • There is no reason having to specify the "active-ness" of the detector in the constructor. Default is now always true. If it really needs to be inactive, use SetActive(false) before adding it as a module.
  • The most important thing is the geometry file name. Instead of using SetGeometryFile, this is now recommended to do in the constructor.
  • Positioning of the detectors is done via TGeoCombiTrans internally anyway. By providing (constructor and setter) access to this, a lot of overly complicated code could be removed while adding a more flexible way to position detectors.
  • By using constructor delegation, a ton of duplicated code could be removed from the detector constructors.
  • Some code common to all detectors was pulled to the base class or removed.
  • R3BMagnet was changed to `R3BAladinMagnet´ for symmetry reasons.
  • Some macros and all tests were adapted to accommodate these changes

Known issues:

  • Will break everyone's macros (except those using r3ball.C)- the fixes are easy and mostly involve removing unnecessary stuff. I can make a forum post with explanations.
  • The constructor for TGeoRotation behaves slightly(TM) different than the list of rotations from before, see changes in macros for examples and the Root Documentation of the TGeo* classes.

I have tested the positing of detectors both in run_sim.C and run_aladin_sim.C, and it is the same as without those changes. However, I'm not sure if they are all correct - some walls seem to be rotated / not aligned with the floor.

@kresan
Copy link
Contributor

kresan commented Jul 6, 2017

All looks good.
Concerning the known issues:

  1. Yes, please make a post on the forum and send a short e-mail to r3b-ml-analysis@ and r3b-wg-sim@ with the link to your post.
  2. The rotation works properly. The walls which are not aligned with the floor - are the DCH modules, which are intentionally rotated along the z-axis.

@kresan kresan merged commit 0003c5a into R3BRootGroup:dev Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants