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

change parameter tag from holonomic to steering #46

Merged
merged 5 commits into from Sep 3, 2021

Conversation

ddengster
Copy link
Contributor

@ddengster ddengster commented Sep 2, 2021

Signed-off-by: ddengster ed.fan@osrfoundation.org

Bug fix

Fixed bug

changed <holonomic> tags to <steering> tag for greater clarity. Likewise for code references.

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
Signed-off-by: ddengster <ed.fan@osrfoundation.org>
@ddengster ddengster changed the title change holonomic to steering_type change tag from holonomic to steering_type Sep 2, 2021
@ddengster ddengster changed the title change tag from holonomic to steering_type change tag from holonomic to steering Sep 2, 2021
@ddengster ddengster changed the title change tag from holonomic to steering change parameter tag from holonomic to steering Sep 2, 2021
@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #46 (9e0001d) into main (3d211fe) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##            main     #46      +/-   ##
========================================
  Coverage   0.00%   0.00%              
========================================
  Files        336     138     -198     
  Lines      31946   12873   -19073     
========================================
+ Misses     31946   12873   -19073     
Flag Coverage Δ
tests 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
..._robot_sim_gazebo_plugins/src/TeleportIngestor.cpp
...uilding_sim_gazebo_plugins/src/toggle_charging.cpp
...on/include/rmf_robot_sim_common/slotcar_common.hpp
...tion/rmf_robot_sim_common/src/dispenser_common.cpp
...building_sim_common/src/crowd_simulator_common.cpp
...m_ignition_plugins/src/LightTuning/LightTuning.cpp
..._sim_common/include/rmf_robot_sim_common/utils.hpp
...bot_sim_ignition_plugins/src/TeleportDispenser.cpp
...ation/rmf_building_sim_gazebo_plugins/src/door.cpp
...ing_sim_gazebo_plugins/src/thumbnail_generator.cpp
... and 452 more

@ddengster ddengster marked this pull request as ready for review September 2, 2021 08:57
Signed-off-by: ddengster <ed.fan@osrfoundation.org>
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

The changes here are okay with me. We can merge after @luca-della-vedova takes a look.

Comment on lines 170 to 172
// steering type constants
static const std::string DIFF_DRIVE;
static const std::string ACKERMANN;
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 we can use a hash map that maps a string to an enum (maybe called VehicleDriveType or something along those lines), so it would be a bit clearer, i.e. go from SlotcarCommon::DIFF_DRIVE to VehicleDriveType::DIFF_DRIVE.
Something similar to what we do for checking the physics engine plugin here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch to enum 2e157ce, no need for the unordered map atm since it's only used in a single area

Copy link
Member

Choose a reason for hiding this comment

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

I did a quick grep in the rmf codebase and noticed that we don't do uppercase enum class, the convention seems to be on UpperCamelCase, also having to write SlotcarCommon::STEERING_TYPE::ACKERMANN is quite verbose, you can bring the definition of the enum class outside of the SlotcarCommon class to get rid of the first scope. Since it would be in the rmf_robot_sim_common namespace there is no risk of polluting other namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 9e0001d

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
Signed-off-by: ddengster <ed.fan@osrfoundation.org>
Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes!

@luca-della-vedova luca-della-vedova merged commit 0f44ea4 into main Sep 3, 2021
@luca-della-vedova luca-della-vedova deleted the fix/steering_param_rename branch September 3, 2021 10:36
luca-della-vedova pushed a commit that referenced this pull request Mar 8, 2022
* change holonomic to steering_type

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

* uncrustify

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

* update changelogs

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

* switch to enum

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

* enum changes

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
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.

None yet

3 participants