-
Notifications
You must be signed in to change notification settings - Fork 69
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
First step of simplifying and improving frames #2399
Conversation
geometry/frame.hpp
Outdated
// | ||
// or: | ||
// enum class MyFrameTag; | ||
// using MyFrame = Frame<MyFrameTag, MyFrameTag{}, Inertial>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frame<enum class MyFrameTag, MyFrameTag{}, Inertial>;
would work (do it that way in the other files);- We should put the
tag_
parameter last, so that it can be omitted even if inertia and handedness are provided: when we define a formal namespace-scope serializable frame, having to explicitly specify inertia and handedness is a feature, and for a local throwaway frame with a local enum class, thetag_
is the most useless parameter (and thus should be the most defaulted one).
Item 2. should probably be done in a separate pull request, it will be a little bit noisy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Done.
- Let's wait until the dust settles.
geometry/frame.hpp
Outdated
@@ -16,12 +16,41 @@ namespace internal_frame { | |||
using base::not_constructible; | |||
using base::not_null; | |||
|
|||
template<typename FrameTag, FrameTag frame_tag, bool frame_is_inertial> | |||
enum Inertia { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReferenceFrameInertia
; we don't see that name much because it's an enum
rather than an enum class
, so it doesn't need to be catchy, and using up Inertia
saddens me (see my comment about the possibility of renaming InertiaTensor
in a recent pull request).
Even better, ReferenceFrameMotion
: I was thinking of distinguishing rotating and non-rotating inertial frames at some point for KeplerOrbit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is called Frame
, not ReferenceFrame
, so the word "reference" is weird. Going for FrameMotion
.
geometry/frame_test.cpp
Outdated
@@ -28,7 +44,7 @@ TEST_F(FrameDeathTest, SerializationError) { | |||
serialization::Frame message; | |||
World1::WriteToMessage(&message); | |||
World2::ReadFromMessage(message); | |||
}, "tag =="); | |||
}, "\\(tag\\) =="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R"(\(tag\) ==)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but I'm not sure if the extra parentheses are better than the extra \
.
enum class
.is_inertial
by an enum.