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

Implement GPU-compatible transformations with ORANGE #872

Merged
merged 7 commits into from
Aug 11, 2023

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Aug 1, 2023

I'm having second thoughts about declaring separate Trans{former,lator}{Down,Up} classes. Maybe we should just have a single class with down(), up(), etc.; and include utility functions (such as "calc_inverse" for the transformation) as part of the class definition...

@sethrj sethrj added enhancement New feature or request orange Work on ORANGE geometry engine labels Aug 1, 2023
@sethrj sethrj requested a review from elliottbiondo August 1, 2023 19:02
@sethrj
Copy link
Member Author

sethrj commented Aug 8, 2023

@elliottbiondo I'd appreciate a quick look and feedback for this.

@elliottbiondo
Copy link
Contributor

Based on only what you have committed here, I don't fully understand what you are proposing, especially since you haven't implemented the translation + rotation case yet.

My best guess: we could easily have one class constructed like: Transformer(Real3 translation, Matrix rot) with up(Real3 point) and down(Real3 point) methods. In the translation-only case, the code has to check that there there is no rotation, or perform rotation as a no-op. But since this is done so frequently in the code, you want to use template metaprogramming to get around this?

@sethrj
Copy link
Member Author

sethrj commented Aug 8, 2023

@elliottbiondo Yes, not only for the extra checks but also for the extra storage. Also having them as two separate types makes it much easier and more elegant to apply transformations to surfaces and shapes.

@sethrj
Copy link
Member Author

sethrj commented Aug 8, 2023

@elliottbiondo I pushed the changes I'd neglected before. Most important properties we need:

  • Apply one transform to another (super nice if we have separate classes with and without rotation)
  • Apply a transform up and down to a position
  • Apply a transform up and down to a direction (rotate only, nullop for translation)
  • Apply a transform to a surface (if we have translation and trans+rot as a separate class, the surface class types can be determined at compile time)

@elliottbiondo
Copy link
Contributor

@sethrj this all looks fine to me in terms of a draft. My only comment: is it necessary to have separate TransfromTrans{former, later} classes, or could these just be methods on Transformation and Translation classes? In the very least, these should be renamed TransfromTrans{formation, lation} to match the names.

@sethrj
Copy link
Member Author

sethrj commented Aug 9, 2023

I think you'll see what I'm getting at when I submit the next chained pull requests: I'm defining function-like classes that when applied to a surface, return the transformed/translated surface type (type dependent input, type dependent output); and same thing for both translations and transformations. So the TT classes are translator/transformer rather than translation/transformation.

@sethrj sethrj force-pushed the orange-transformer branch from 0bd7ba9 to 9cb5602 Compare August 10, 2023 19:17
Copy link
Member Author

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elliottbiondo I gave this a quick look over and it's ready for review! (I'm deferring the transform transformers to a later PR.)

@sethrj sethrj marked this pull request as ready for review August 11, 2023 11:11
Copy link
Contributor

@elliottbiondo elliottbiondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @sethrj, just a few minor comments

src/orange/MatrixUtils.hh Show resolved Hide resolved
src/orange/OrangeTrackView.hh Show resolved Hide resolved
src/orange/transform/Transformation.cc Show resolved Hide resolved
src/orange/transform/Translation.hh Outdated Show resolved Hide resolved
src/orange/transform/Translation.hh Outdated Show resolved Hide resolved
@sethrj sethrj merged commit a0934f1 into celeritas-project:develop Aug 11, 2023
@sethrj sethrj deleted the orange-transformer branch August 11, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request orange Work on ORANGE geometry engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants