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

Make path times public. #197

Merged
merged 7 commits into from
Mar 15, 2022

Conversation

jess-moss
Copy link

@jess-moss jess-moss commented Mar 9, 2022

Fixes #

Changes in this PRs:

Checklists:

  • Update HISTORY.md with a single line describing this PR

This change is Reviewable

Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Per Issue #187

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jess-moss and @jmirabel)

Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jmirabel)


cpp/src/toppra/parametrizer/const_accel.hpp, line 19 at r1 (raw file):

Previously, jmirabel (Joseph Mirabel) wrote…
  const Vector& getTs() { return m_ts; }

Done.

@jmirabel
Copy link
Collaborator

Per Issue #187

Thanks for pointing the issue. It made me realize I overlooked the changes. The prototype should be const Vector getTimes() const so two additional changes:

  • const to help the compiler (although it should be able to understand it)
  • function name change to try to stick to the naming convention followed elsewhere in this lib (and Ts is a little obscure).

Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Thank you for reviewing this! I need this function to gain access to these times while using toppra, so this will be very helpful.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jmirabel)

Copy link
Collaborator

@jmirabel jmirabel left a comment

Choose a reason for hiding this comment

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

Thanks. I have a few more comments.

cpp/src/toppra/parametrizer.hpp Show resolved Hide resolved
cpp/src/toppra/parametrizer/const_accel.hpp Outdated Show resolved Hide resolved
cpp/src/toppra/parametrizer/const_accel.hpp Outdated Show resolved Hide resolved
Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jmirabel)


cpp/src/toppra/parametrizer.hpp, line 55 at r3 (raw file):

Previously, jmirabel (Joseph Mirabel) wrote…
    /** \brief Return the waypoints time.
   */
   virtual const Vector& getTimes() const = 0;

Done.


cpp/src/toppra/parametrizer/const_accel.hpp, line 19 at r3 (raw file):

Previously, jmirabel (Joseph Mirabel) wrote…
  const Vector& getTimes() const override { return m_ts; }

Why final ? I think override is sufficient here. It is fine if a child class overrides this.

I made it final because I could not foresee an instance when a child class would want to override this function, and didn't want a child class to do so accidentally. I can change it to overrideif you think that is fine!

Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Thank you!

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jmirabel)

Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jmirabel)


cpp/src/toppra/parametrizer.hpp, line 55 at r3 (raw file):

Previously, jess-moss wrote…

Done.

@jmirabel Would it be possible to resolve this comment?

@hungpham2511
Copy link
Owner

Hi @jess-moss, thanks for the PR! Could you also add a line to the changelog?

Thanks @jmirabel for the review. The PR looks good to me; I will be happy to merge once all comments are resolved.

Copy link
Author

@jess-moss jess-moss left a comment

Choose a reason for hiding this comment

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

Just updated the changelog! Thank you!

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @jmirabel)

@jmirabel jmirabel merged commit 9a7325f into hungpham2511:develop Mar 15, 2022
@hashb hashb mentioned this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants