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

Jump to bottom

Improved SimpleCarState: AgentState #517

Merged
merged 7 commits into from
Aug 7, 2018
Merged

Improved SimpleCarState: AgentState #517

merged 7 commits into from
Aug 7, 2018

Conversation

apojomovsky
Copy link

This PR fixes #505

  • Replaces the SimpleCarState and SimpleCarState_v by an enhanced version called AgentState and AgentState_v.
  • Includes position in 3 dimensions (x, y, z) and orientation in euler (roll, pitch, yaw).

hidmic
hidmic suggested changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@apojomovsky just a couple comments.

src/protobuf/agent_state.proto Outdated
optional Vector3d position = 4;

/// \brief Orientation of the agent (in euler).
optional Euler orientation = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: consider RPYAngles instead of Euler. There're many axis/angle conventions for the former and it may lead to confusion.

Copy link
Author

Choose a reason for hiding this comment

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

Hm, you're right. Just renamed it, thanks!

src/protobuf/agent_state.proto Outdated
optional Euler orientation = 5;

/// \brief Current speed module of the car in radians
optional double velocity = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky shouldn't these be two Vector3d fields, for linear_velocity and angular_velocity?

Copy link
Author

Choose a reason for hiding this comment

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

Just added them both, thanks!

@apojomovsky apojomovsky force-pushed the apojomovsky/rename_improve_simple_car_state branch from 9865c4a to 411d23e Compare August 1, 2018 14:47
@apojomovsky
Copy link
Author

apojomovsky commented Aug 1, 2018 •
edited
Loading

Thanks for the review @hidmic , I just finished addressing your comments.
I also removed the drake-simple-car-state-to-ign translator code/tests since, despite not being used/required anymore, it also demanded some changes after the improvements made over the SimpleCarState, which didn't make sense to address at this point.
Could you PTAL?

@apojomovsky apojomovsky force-pushed the apojomovsky/rename_improve_simple_car_state branch from 411d23e to c029bdf Compare August 2, 2018 17:05
@apojomovsky apojomovsky force-pushed the apojomovsky/rename_improve_simple_car_state branch from c029bdf to 821003c Compare August 2, 2018 17:07
hidmic
hidmic suggested changes Aug 2, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@apojomovsky I left some more comments. About deprecating the SimpleCarState translators, I'm not so sure we won't ever need them again. @basicNew @stonier what do you think?

src/protobuf/rpy_angles.proto Outdated

/// \ingroup ignition.msgs
/// \interface RPYAngles
/// \brief The representation of orientation in euler angles.
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: I think that being explicit about frame conventions is never a waste of space.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, done!

src/protobuf/agent_state.proto Outdated
/// \ingroup ignition.msgs
/// \interface AgentState
/// \brief The ignition equivalent of the LCM command used to show the
/// current status of a delphyne agent in a simulation.
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: it'd be nice to be specific about frame conventions for pose and velocity.

Copy link
Author

Choose a reason for hiding this comment

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

Done, thanks!

src/translations/pose_bundle_to_agent_state_v.cc Outdated
const drake::Isometry3<double>& pose = drake_message.get_pose(i);
// Translates pose from quaternion to euler.
const Eigen::Vector3d euler_rotation =
pose.rotation().eulerAngles(0, 1, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: a small explanation on the indexes chosen would be nice.

Copy link
Author

Choose a reason for hiding this comment

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

Done, thanks!

test/regression/cpp/automotive_simulator_test.cc Outdated
EXPECT_NEAR(state_message.heading(), kHeading, kAccuracy);
EXPECT_NEAR(state_message.velocity(), kVelocity, kAccuracy);
EXPECT_EQ(state_message.position().x(), kX);
EXPECT_NEAR(state_message.position().y(), kY, kAccuracy);
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky why the difference in the last two assertions?

Copy link
Author

Choose a reason for hiding this comment

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

Hm, my bad. Just fixed it. Thanks!

test/regression/cpp/automotive_simulator_test.cc Outdated
EXPECT_NEAR(state_message.velocity(), kVelocity, kAccuracy);
EXPECT_EQ(state_message.position().x(), kX);
EXPECT_NEAR(state_message.position().y(), kY, kAccuracy);
EXPECT_NEAR(state_message.orientation().yaw(), kHeading, kAccuracy);
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky I think it is valuable to check that the other components are 0 as well.

Copy link
Author

Choose a reason for hiding this comment

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

Done, thanks!

@apojomovsky
Copy link
Author

Thanks for the review @hidmic , just finished addressing your comments. This is ready for another pass.

hidmic
hidmic approved these changes Aug 7, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Couple nits. LGTM!

src/protobuf/agent_state.proto Outdated
/// - linear_velocity: A Vector3 with the components of velocity projected over
/// each of the x, y and z axes, respectively. Measured in m/s.
/// - angular_velocity: A Vector3 with the angular velocities around the axes
/// x, y and z, respectively. Measured in rad/s.
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: stating it's a right hand convention would be valuable.

Copy link
Author

Choose a reason for hiding this comment

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

done, thanks!

src/protobuf/rpy_angles.proto Outdated
/// \ingroup ignition.msgs
/// \interface RPYAngles
/// \brief The representation of orientation in euler angles following the
/// x-y-z convention (roll-pitch-yaw).
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: same as above.

Copy link
Author

Choose a reason for hiding this comment

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

done, thanks!

src/protobuf/agent_state.proto Outdated
/// - orientation: In euler angles following the x-y-z convention
/// (roll, pitch, yaw).
/// - linear_velocity: A Vector3 with the components of velocity projected over
/// each of the x, y and z axes, respectively. Measured in m/s.
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky nit: the fact that vector components are projections over a vector base is kind of implied, not sure if it adds any value.

Copy link
Author

Choose a reason for hiding this comment

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

done, thanks!

@apojomovsky apojomovsky merged commit a8a7789 into master Aug 7, 2018
@apojomovsky apojomovsky deleted the apojomovsky/rename_improve_simple_car_state branch August 7, 2018 13:25
@stonier stonier mentioned this pull request Sep 18, 2018
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

@clalancette clalancette Awaiting requested review from clalancette

@basicNew basicNew Awaiting requested review from basicNew

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename/Improve SimpleCarState message.
2 participants
@apojomovsky @hidmic