refactor: Explicit and type safe navigation delegates #2214
Conversation
|
TODO
|
| /// @param nState [in,out] is the navigation state to be updated | ||
| /// | ||
| /// @todo for surfaces skip the non-reached ones, while keep for portals | ||
| inline static void updateCandidates( |
There was a problem hiding this comment.
Do these need to be inline?
There was a problem hiding this comment.
I guess not - I just saw that removing that would lead to linker errors in some cases but maybe that was also in combination with templates. the question is also if these kind of methods really have to have their impl in the header
There was a problem hiding this comment.
the question is also if these kind of methods really have to have their impl in the header
That's what I was trying to ask.
| const std::vector<std::shared_ptr<Acts::Surface>>& lSurfaces, | ||
| const std::vector<size_t>& assignToAll, | ||
| std::vector<std::shared_ptr<Acts::Surface>> lSurfaces, | ||
| std::vector<size_t> assignToAll, |
There was a problem hiding this comment.
Just need to make sure we don't copy these needlessly.
There was a problem hiding this comment.
agreed - I just think by value gives you more flexibility when you are sinking the param anyways
| // Return the transform, the volume bounds, and some default portal | ||
| // generators | ||
| return {Transform3(m_cfg.transform * eTransform), std::move(volumeBounds), | ||
| defaultPortalGenerator()}; | ||
| makePortalGenerator<const DefaultPortalGenerator>()}; |
There was a problem hiding this comment.
what do you mean by force? I wondered if I should add it to the helper directly. but at the same time I wondered why we require const because the method being called is const anyways 🤔
|
@andiwand how do we go about this? |
I can update this again - in principle it is ready to review just the CI is failing There is one detail about the grids I was unsure about. I had to touch it without really knowing what is going on behind the scenes |
- consistent pass by value - reformat documentation pull changes out of #2214
- consistent pass by value - reformat documentation pull changes out of acts-project#2214
|
this will be followed up in other, smaller PRs |
In this PR I try to make the navigation delegates more explicit and type safe by splitting the interface of volume finders and surface candidate lookups. That way the navigation state is just a view to the delegates and cannot be modified which also lowers bug risk IMO.
DetectorVolumeUpdatortoDetectorVolumeFinderSurfaceCandidatesUpdatortoSurfaceCandidatesDelegate