Add flags to Saver classes to specify what to save #339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #339 +/- ##
========================================
- Coverage 81.02% 81% -0.03%
========================================
Files 205 205
Lines 5997 6006 +9
========================================
+ Hits 4859 4865 +6
- Misses 1138 1141 +3
|
jslee02
reviewed
Feb 18, 2018
| /// \param _world World to save state from and restore to. | ||
| explicit WorldStateSaver(World* const _world); | ||
| /// \param world World to save state from and restore to. | ||
| /// \param options Options to specify what should be saved |
| explicit WorldStateSaver(World* const _world); | ||
| /// \param world World to save state from and restore to. | ||
| /// \param options Options to specify what should be saved | ||
| explicit WorldStateSaver(World* const world, int options = CONFIGURATIONS); |
There was a problem hiding this comment.
Nit: Please change to const World* world. They are identical but different style. I believe our convention is const World* world.
There was a problem hiding this comment.
I think it was actually incorrect to be const since we restore the World state in the destructor.
| /// Construct a MetaSkeletonStateSaver and save the current state of the \c | ||
| /// MetaSkeleton. This state will be restored when MetaSkeletonStateSaver is | ||
| /// destructed. | ||
| /// | ||
| /// \param _metaskeleton MetaSkeleton to save/restore | ||
| /// \param metaskeleton MetaSkeleton to save/restore | ||
| /// \param options Options to specify what should be saved |
jslee02
approved these changes
Feb 19, 2018
| auto skeleton = Skeleton::create(); | ||
| skeleton->createJointAndBodyNodePair<RevoluteJoint>(); | ||
| public: | ||
| void SetUp() |
There was a problem hiding this comment.
What do you mean? Isn't the correct name SetUp?
There was a problem hiding this comment.
I believe function naming convention is to start with a lower case (note that SetUp begins with an upper latter "S").
There was a problem hiding this comment.
Oh, this is required by gtest. Sorry for the noise. 😞
gilwoolee
pushed a commit
that referenced
this pull request
Jan 21, 2019
* Add flags to select what should be saved by Saver classes. * Add more tests. * Restructure SaverOptions. * Address @jslee02's comments. * Remove incorrect const specifier. * Update CHANGELOG.md.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #276. Resolves #293.
This implementation might not be ideal because the "options" can be arbitrary ints, rather than combinations of the options that we actually enumerate. If that's a problem, maybe something like this would be better?
Before creating a pull request
make formatBefore merging a pull request
CHANGELOG.md