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

Headers for the new-style DiscreteTrajectory #3137

Merged
merged 4 commits into from
Oct 3, 2021

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Oct 3, 2021

First step of #3136.

template<typename Frame>
class DiscreteTrajectory2 : public Trajectory<Frame> {
public:
using Iterator = DiscreteTrajectoryIterator<Frame>;
Copy link
Member

Choose a reason for hiding this comment

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

let’s call the result of begin iterator; the API is meant to look like a Container here (or a std::span or somesuch).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Iterator end() const;

Iterator rbegin() const;
Iterator rend() const;
Copy link
Member

Choose a reason for hiding this comment

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

I should hope the reverse iterator is not equal to the iterator, otherwise the container cannot contain more than one element.
std::reverse_iterator<iterator>, which should probably be named using reverse_iterator = std::reverse_iterator<iterator>.

Sorry, something went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

SegmentIterator segments_end() const;

SegmentIterator segments_rbegin() const;
SegmentIterator segments_rend() const;
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for rbegin, std::reverse_iterator<SegmentIterator>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Iterator upper_bound(Instant const& t) const;

SegmentIterator segments_begin() const;
SegmentIterator segments_end() const;
Copy link
Member

Choose a reason for hiding this comment

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

If we want to iterate over the segments, a SegmentRange type accessible as trajectory.segments() would be useful, so that we can write

for (auto const& segment: trajectory.segments()) {

rather than

for (auto segments_it = trajectory.segments_begin();
     segments_it != trajectory.segments_end();
     ++segments_it) {
  auto const& segment = *segments_it;

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding segments() and rsegments() since it avoids all these segment_ functions.

SegmentIterator segments_rbegin() const;
SegmentIterator segments_rend() const;
SegmentRange segments() const;
ReverseSegmentRange rsegments() const;
Copy link
Member

Choose a reason for hiding this comment

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

I think the Right Thing to do is to have only segments and use an adapter to iterate backward (in C++20 that would be https://en.cppreference.com/w/cpp/ranges/reverse_view, maybe absl has a thing in the meantime, or we can write one in base, we should have make_reverse_iterator).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a TODO to fix this once we have C++20.

@eggrobin eggrobin added the LGTM label Oct 3, 2021
@pleroy pleroy merged commit 189e189 into mockingbirdnest:Ondelette Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants