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.

Sign up for GitHub

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

Cleanup and simplification of new-style discrete trajectory classes #3152

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Cleanup and simplification of new-style discrete trajectory classes #3152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cbb4755
Remove a layer of indirection.
pleroy Oct 12, 2021
cfd03ba
Change the factories to return timelines.
pleroy Oct 13, 2021
a2bf91d
Fix a test.
pleroy Oct 13, 2021
ad5f0c2
All tests passing.
pleroy Oct 13, 2021
5ffe9c7
Dead code.
pleroy Oct 13, 2021
9903b39
Merge.
pleroy Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 52 additions & 100 deletions physics/discrete_trajectory_iterator_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,135 +12,87 @@
#include "physics/discrete_trajectory_segment.hpp"
#include "physics/discrete_trajectory_segment_iterator.hpp"
#include "physics/discrete_trajectory_types.hpp"
#include "physics/mock_discrete_trajectory_segment.hpp"
#include "quantities/quantities.hpp"
#include "quantities/si.hpp"

namespace principia {
namespace physics {

using base::check_not_null;
using base::make_not_null_unique;
using base::not_null;
using geometry::Frame;
using geometry::Instant;
using physics::DegreesOfFreedom;
using quantities::Time;
using quantities::si::Second;
using ::testing::Return;

namespace {

// A trajectory that holds a real timeline, where we mock multiple methods to
// return data from that timeline.
template<typename Frame>
class FakeDiscreteTrajectorySegment
: public MockDiscreteTrajectorySegment<Frame> {
public:
FakeDiscreteTrajectorySegment() = default;

internal_discrete_trajectory_types::Timeline<Frame> timeline;
};

} // namespace

class DiscreteTrajectoryIteratorTest : public ::testing::Test {
protected:
using World = Frame<enum class WorldTag>;

using Segments = internal_discrete_trajectory_types::Segments<World>;
using Timeline = internal_discrete_trajectory_types::Timeline<World>;

DiscreteTrajectoryIteratorTest() {
// Set up a fake trajectory with 3 segments. After construction, the mocks
// are owned by |segments_|.
auto owned_mock1 = std::make_unique<FakeDiscreteTrajectorySegment<World>>();
auto owned_mock2 = std::make_unique<FakeDiscreteTrajectorySegment<World>>();
auto owned_mock3 = std::make_unique<FakeDiscreteTrajectorySegment<World>>();
auto const& mock1 = *owned_mock1;
auto const& mock2 = *owned_mock2;
auto const& mock3 = *owned_mock3;

segments_.push_back(std::move(owned_mock1));
auto const it1 = --segments_.end();
segments_.push_back(std::move(owned_mock2));
auto const it2 = --segments_.end();
segments_.push_back(std::move(owned_mock3));
auto const it3 = --segments_.end();

FillSegment(it1,
Timeline{MakeTimelineValueType(2 * Second),
MakeTimelineValueType(3 * Second),
MakeTimelineValueType(5 * Second),
MakeTimelineValueType(7 * Second),
MakeTimelineValueType(11 * Second)});
FillSegment(it2, Timeline{MakeTimelineValueType(13 * Second)});
FillSegment(it3,
Timeline{MakeTimelineValueType(13 * Second), // Duplicated.
MakeTimelineValueType(17 * Second),
MakeTimelineValueType(19 * Second),
MakeTimelineValueType(23 * Second)});

// This must happen *after* the segments have been set up.
EXPECT_CALL(mock1, timeline_begin())
.WillRepeatedly(Return(timeline_begin(it1)));
EXPECT_CALL(mock1, timeline_end())
.WillRepeatedly(Return(timeline_end(it1)));
EXPECT_CALL(mock2, timeline_begin())
.WillRepeatedly(Return(timeline_begin(it2)));
EXPECT_CALL(mock2, timeline_end())
.WillRepeatedly(Return(timeline_end(it2)));
EXPECT_CALL(mock3, timeline_begin())
.WillRepeatedly(Return(timeline_begin(it3)));
EXPECT_CALL(mock3, timeline_end())
.WillRepeatedly(Return(timeline_end(it3)));
}

FakeDiscreteTrajectorySegment<World>* DownCast(
std::unique_ptr<DiscreteTrajectorySegment<World>> const& segment) {
return dynamic_cast<FakeDiscreteTrajectorySegment<World>*>(segment.get());
}

void FillSegment(Segments::iterator const it, Timeline const& timeline) {
auto* const segment = DownCast(*it);
segment->timeline = timeline;
DiscreteTrajectoryIteratorTest()
: segments_(MakeSegments(3)) {
auto it = segments_->begin();
{
auto& segment1 = *it;
segment1.Append(t0_ + 2 * Second, unmoving_origin_);
segment1.Append(t0_ + 3 * Second, unmoving_origin_);
segment1.Append(t0_ + 5 * Second, unmoving_origin_);
segment1.Append(t0_ + 7 * Second, unmoving_origin_);
segment1.Append(t0_ + 11 * Second, unmoving_origin_);
}

++it;
{
auto& segment2 = *it;
segment2.Append(t0_ + 13 * Second, unmoving_origin_);
}

++it;
{
auto& segment3 = *it;
segment3.Append(t0_ + 13 * Second, unmoving_origin_);
segment3.Append(t0_ + 17 * Second, unmoving_origin_);
segment3.Append(t0_ + 19 * Second, unmoving_origin_);
segment3.Append(t0_ + 23 * Second, unmoving_origin_);
}
}

DiscreteTrajectoryIterator<World> MakeBegin(
Segments::const_iterator const it) {
return DiscreteTrajectoryIterator<World>(
DiscreteTrajectorySegmentIterator<World>(
check_not_null(&segments_), it),
timeline_begin(it));
DiscreteTrajectorySegmentIterator<World>(segments_.get(), it),
it->timeline_begin());
}

DiscreteTrajectoryIterator<World> MakeEnd(Segments::const_iterator it) {
return DiscreteTrajectoryIterator<World>(
DiscreteTrajectorySegmentIterator<World>(check_not_null(&segments_),
++it),
DiscreteTrajectorySegmentIterator<World>(segments_.get(), ++it),
std::nullopt);
}

Timeline::value_type MakeTimelineValueType(Time const& t) {
static const DegreesOfFreedom<World> unmoving_origin(World::origin,
World::unmoving);
return {t0_ + t, unmoving_origin};
}

internal_discrete_trajectory_types::Timeline<World>::const_iterator
timeline_begin(Segments::const_iterator const it) {
return DownCast(*it)->timeline.begin();
}

internal_discrete_trajectory_types::Timeline<World>::const_iterator
timeline_end(Segments::const_iterator const it) {
return DownCast(*it)->timeline.end();
// Constructs a list of |n| segments which are properly initialized.
// TODO(phl): Move to a central place.
static not_null<std::unique_ptr<Segments>> MakeSegments(const int n) {
auto segments = make_not_null_unique<Segments>(n);
for (auto it = segments->begin(); it != segments->end(); ++it) {
*it = DiscreteTrajectorySegment<World>(
DiscreteTrajectorySegmentIterator<World>(segments.get(), it));
}
return segments;
}

Segments segments_;
not_null<std::unique_ptr<Segments>> segments_;
Instant const t0_;
DegreesOfFreedom<World> const unmoving_origin_{World::origin,
World::unmoving};
};

TEST_F(DiscreteTrajectoryIteratorTest, ForwardOneSegment) {
auto segment = segments_.begin();
auto segment = segments_->begin();
auto iterator = MakeBegin(segment);
EXPECT_EQ(t0_ + 2 * Second, iterator->first);
auto const current = ++iterator;
Expand All @@ -152,7 +104,7 @@ TEST_F(DiscreteTrajectoryIteratorTest, ForwardOneSegment) {
}

TEST_F(DiscreteTrajectoryIteratorTest, BackwardOneSegment) {
auto segment = --segments_.end();
auto segment = --segments_->end();
auto iterator = MakeEnd(segment);
--iterator;
EXPECT_EQ(t0_ + 23 * Second, (*iterator).first);
Expand All @@ -165,7 +117,7 @@ TEST_F(DiscreteTrajectoryIteratorTest, BackwardOneSegment) {
}

TEST_F(DiscreteTrajectoryIteratorTest, ForwardAcrossSegments) {
auto segment = segments_.begin();
auto segment = segments_->begin();
auto iterator = MakeBegin(segment);
for (int i = 0; i < 4; ++i) {
++iterator;
Expand All @@ -178,7 +130,7 @@ TEST_F(DiscreteTrajectoryIteratorTest, ForwardAcrossSegments) {
}

TEST_F(DiscreteTrajectoryIteratorTest, BackwardAcrossSegments) {
auto segment = --segments_.end();
auto segment = --segments_->end();
auto iterator = MakeEnd(segment);
for (int i = 0; i < 3; ++i) {
--iterator;
Expand All @@ -193,15 +145,15 @@ TEST_F(DiscreteTrajectoryIteratorTest, BackwardAcrossSegments) {
TEST_F(DiscreteTrajectoryIteratorTest, Equality) {
// Construct two iterators that denote the time 13 * Second but in different
// segments.
auto it1 = MakeEnd(--segments_.end());
auto it1 = MakeEnd(--segments_->end());
for (int i = 0; i < 3; ++i) {
--it1;
}
EXPECT_EQ(t0_ + 17 * Second, (*it1).first);
--it1;
EXPECT_EQ(t0_ + 13 * Second, (*it1).first);

auto it2 = MakeBegin(segments_.begin());
auto it2 = MakeBegin(segments_->begin());
for (int i = 0; i < 4; ++i) {
++it2;
}
Expand All @@ -210,9 +162,9 @@ TEST_F(DiscreteTrajectoryIteratorTest, Equality) {
EXPECT_EQ(t0_ + 13 * Second, it2->first);

EXPECT_EQ(it1, it2);
EXPECT_NE(it1, MakeBegin(segments_.begin()));
EXPECT_NE(it2, MakeEnd(--segments_.end()));
EXPECT_NE(MakeBegin(segments_.begin()), MakeEnd(--segments_.end()));
EXPECT_NE(it1, MakeBegin(segments_->begin()));
EXPECT_NE(it2, MakeEnd(--segments_->end()));
EXPECT_NE(MakeBegin(segments_->begin()), MakeEnd(--segments_->end()));
}

} // namespace physics
Expand Down
2 changes: 2 additions & 0 deletions physics/discrete_trajectory_segment.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ FORWARD_DECLARE_FROM(discrete_trajectory_factories,
namespace physics {

class DiscreteTrajectoryIteratorTest;
class DiscreteTrajectorySegmentIteratorTest;
class DiscreteTrajectorySegmentTest;

namespace internal_discrete_trajectory_segment {
Expand Down Expand Up @@ -137,6 +138,7 @@ class DiscreteTrajectorySegment : public Trajectory<Frame> {

// For testing.
friend class physics::DiscreteTrajectoryIteratorTest;
friend class physics::DiscreteTrajectorySegmentIteratorTest;
friend class physics::DiscreteTrajectorySegmentTest;
template<typename F>
friend class testing_utilities::DiscreteTrajectoryFactoriesFriend;
Expand Down
4 changes: 2 additions & 2 deletions physics/discrete_trajectory_segment_iterator_body.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ DiscreteTrajectorySegmentIterator<Frame>::operator--(int) { // NOLINT
template<typename Frame>
internal_discrete_trajectory_segment::DiscreteTrajectorySegment<Frame> const&
DiscreteTrajectorySegmentIterator<Frame>::operator*() const {
return **iterator_;
return *iterator_;
}

template<typename Frame>
internal_discrete_trajectory_segment::DiscreteTrajectorySegment<Frame> const*
DiscreteTrajectorySegmentIterator<Frame>::operator->() const {
return iterator_->get();
return &*iterator_;
}

template<typename Frame>
Expand Down
72 changes: 51 additions & 21 deletions physics/discrete_trajectory_segment_iterator_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,53 +5,83 @@

#include "base/not_null.hpp"
#include "geometry/frame.hpp"
#include "geometry/named_quantities.hpp"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "physics/discrete_trajectory_segment.hpp"
#include "physics/discrete_trajectory_types.hpp"
#include "physics/mock_discrete_trajectory_segment.hpp"
#include "quantities/si.hpp"

namespace principia {
namespace physics {

using base::check_not_null;
using base::make_not_null_unique;
using base::not_null;
using geometry::Frame;
using geometry::Instant;
using quantities::si::Second;
using ::testing::Return;

// We use a mock segment in this test to avoid having to go through a
// complicated setup just to test the iterator.
class DiscreteTrajectorySegmentIteratorTest : public ::testing::Test {
protected:
using World = Frame<enum class WorldTag>;

using Segments = internal_discrete_trajectory_types::Segments<World>;

DiscreteTrajectorySegmentIteratorTest()
: segments_(MakeSegments(3)) {
auto it = segments_->begin();
{
auto& segment1 = *it;
segment1.Append(t0_ + 2 * Second, unmoving_origin_);
segment1.Append(t0_ + 3 * Second, unmoving_origin_);
segment1.Append(t0_ + 5 * Second, unmoving_origin_);
segment1.Append(t0_ + 7 * Second, unmoving_origin_);
segment1.Append(t0_ + 11 * Second, unmoving_origin_);
}

++it;
{
auto& segment2 = *it;
segment2.Append(t0_ + 13 * Second, unmoving_origin_);
}

++it;
{
auto& segment3 = *it;
segment3.Append(t0_ + 13 * Second, unmoving_origin_);
segment3.Append(t0_ + 17 * Second, unmoving_origin_);
segment3.Append(t0_ + 19 * Second, unmoving_origin_);
}
}

DiscreteTrajectorySegmentIterator<World> MakeIterator(
not_null<Segments const*> const segments,
Segments::const_iterator const iterator) {
return DiscreteTrajectorySegmentIterator<World>(segments, iterator);
}
};

TEST_F(DiscreteTrajectorySegmentIteratorTest, Basic) {
auto owned_mock1 = std::make_unique<MockDiscreteTrajectorySegment<World>>();
auto owned_mock2 = std::make_unique<MockDiscreteTrajectorySegment<World>>();
auto owned_mock3 = std::make_unique<MockDiscreteTrajectorySegment<World>>();
auto const& mock1 = *owned_mock1;
auto const& mock2 = *owned_mock2;
auto const& mock3 = *owned_mock3;

Segments segments;
segments.push_back(std::move(owned_mock1));
segments.push_back(std::move(owned_mock2));
segments.push_back(std::move(owned_mock3));
// Constructs a list of |n| segments which are properly initialized.
// TODO(phl): Move to a central place.
static not_null<std::unique_ptr<Segments>> MakeSegments(const int n) {
auto segments = make_not_null_unique<Segments>(n);
for (auto it = segments->begin(); it != segments->end(); ++it) {
*it = DiscreteTrajectorySegment<World>(
DiscreteTrajectorySegmentIterator<World>(segments.get(), it));
}
return segments;
}

EXPECT_CALL(mock1, size()).WillRepeatedly(Return(5));
EXPECT_CALL(mock2, size()).WillRepeatedly(Return(1));
EXPECT_CALL(mock3, size()).WillRepeatedly(Return(3));
not_null<std::unique_ptr<Segments>> segments_;
Instant const t0_;
DegreesOfFreedom<World> const unmoving_origin_{World::origin,
World::unmoving};
};

TEST_F(DiscreteTrajectorySegmentIteratorTest, Basic) {
{
auto iterator = MakeIterator(check_not_null(&segments), segments.begin());
auto iterator = MakeIterator(segments_.get(), segments_->begin());
EXPECT_EQ(5, iterator->size());
auto const current = ++iterator;
EXPECT_EQ(1, iterator->size());
Expand All @@ -61,7 +91,7 @@ TEST_F(DiscreteTrajectorySegmentIteratorTest, Basic) {
EXPECT_EQ(1, previous->size());
}
{
auto iterator = MakeIterator(check_not_null(&segments), segments.end());
auto iterator = MakeIterator(segments_.get(), segments_->end());
--iterator;
EXPECT_EQ(3, (*iterator).size());
auto const current = --iterator;
Expand Down
Loading