chore: remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface #2233

takayuki5168 · 2022-11-07T14:13:37Z

Signed-off-by: Takayuki Murooka takayuki5168@gmail.com

Description

related to #2229
remove autoware_auto_common from simple_planning_simulator, osqp_interface

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

I've confirmed the contribution guidelines.
The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

takayuki5168 · 2022-11-07T14:14:11Z

...r/simple_planning_simulator/src/simple_planning_simulator/simple_planning_simulator_core.cpp

  velocity.lateral_velocity = 0.0F;
-  velocity.heading_rate = static_cast<float32_t>(vehicle_model_ptr->getWz());
+  velocity.heading_rate = static_cast<double>(vehicle_model_ptr->getWz());
  return velocity;


I'm not sure why sometimes float32_t instead of float64_t is used

It is because the module interface message was defined by float_32 (e.g. path velocity). This package implementation just followed the interface. (If I remember correctly)

Got it. Thanks.

Should I revert to float?
In other planning packages, we use path velocity as double. Therefore, double can be used here as well IMO.

I don't know much about the risk of using different types, but if it is working, I think it is fine. There is no need to revert your commit. I just wanted to make sure it wasn't an unintentional change.

codecov · 2022-11-07T15:21:08Z

Codecov Report

Base: 11.13% // Head: 11.37% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (3e98ca4) compared to base (536a958).
Patch coverage: 64.77% of modified lines in pull request are covered.

❗ Current head 3e98ca4 differs from pull request most recent head a86aa66. Consider uploading reports for the commit a86aa66 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2233      +/-   ##
==========================================
+ Coverage   11.13%   11.37%   +0.24%     
==========================================
  Files        1203     1202       -1     
  Lines       86264    82738    -3526     
  Branches    20827    20748      -79     
==========================================
- Hits         9605     9412     -193     
+ Misses      66501    63330    -3171     
+ Partials    10158     9996     -162

Flag	Coverage Δ		*Carryforward flag
differential	`13.32% <64.53%> (?)`
total	`11.23% <64.59%> (+0.12%)`	⬆️	Carriedforward from 0d9c92f

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
...nterface/include/osqp_interface/osqp_interface.hpp	`0.00% <ø> (ø)`
...ulator/vehicle_model/sim_model_delay_steer_acc.hpp	`100.00% <ø> (ø)`
...vehicle_model/sim_model_delay_steer_acc_geared.hpp	`100.00% <ø> (ø)`
...ulator/vehicle_model/sim_model_delay_steer_vel.hpp	`100.00% <ø> (ø)`
...ulator/vehicle_model/sim_model_ideal_steer_acc.hpp	`100.00% <ø> (ø)`
...vehicle_model/sim_model_ideal_steer_acc_geared.hpp	`100.00% <ø> (ø)`
...ulator/vehicle_model/sim_model_ideal_steer_vel.hpp	`100.00% <ø> (ø)`
...ng_simulator/vehicle_model/sim_model_interface.hpp	`60.00% <ø> (-6.67%)`	⬇️
common/osqp_interface/test/test_osqp_interface.cpp	`22.22% <11.76%> (+7.22%)`	⬆️
...nning_simulator/simple_planning_simulator_core.cpp	`46.15% <22.22%> (+7.97%)`	⬆️
... and 628 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

takayuki5168 · 2022-11-09T16:02:10Z

I confirmed that planning simulator works well with this PR locally.

takayuki5168 · 2022-11-09T16:08:11Z

@TakaHoribe @rej55 @maxime-clem @satoshi-ota Could you review? No behavior change with this PR.

TakaHoribe · 2022-11-09T16:13:22Z

...r/simple_planning_simulator/src/simple_planning_simulator/simple_planning_simulator_core.cpp

  yaw += static_cast<float>((*n.rpy_dist_)(*n.rand_engine_));
  odom.pose.pose.orientation = motion::motion_common::from_angle(yaw);

-  vel.longitudinal_velocity += static_cast<float32_t>(velocity_noise);
+  vel.longitudinal_velocity += static_cast<double>(velocity_noise);


vel.longitudinal_velocity is defined as a float.
https://github.com/tier4/autoware_auto_msgs/blob/tier4/main/autoware_auto_vehicle_msgs/msg/VelocityReport.idl#L14

Did you replace float32_t with double on purpose in this PR? Or just mistake?

As commented here, on purpose.
I followed the other packages' implementation (= path velocity is handled as double).
#2233 (comment)

TakaHoribe

Just change to use the premitive types and remove dependency.
(Note: it is decided not to use the autoware specific types in the autoware.universe. This kind of type definition is discussed in the autoware.core architecture design.)

rej55

LGTM

…, osqp_interface Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>

…mulator and osqp_interface (autowarefoundation#2233) remove autoware_auto_common dependency from simple_planning_simulator, osqp_interface Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com> Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com> Signed-off-by: Kotaro Yoshimoto <pythagora.yoshimoto@gmail.com>

…mulator and osqp_interface (autowarefoundation#2233) remove autoware_auto_common dependency from simple_planning_simulator, osqp_interface Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com> Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com> Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>

takayuki5168 commented Nov 7, 2022

View reviewed changes

takayuki5168 marked this pull request as ready for review November 9, 2022 16:01

takayuki5168 requested review from TakaHoribe, rej55, maxime-clem and satoshi-ota as code owners November 9, 2022 16:01

takayuki5168 changed the title ~~remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface~~ chore: remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface Nov 9, 2022

TakaHoribe reviewed Nov 9, 2022

View reviewed changes

TakaHoribe approved these changes Nov 9, 2022

View reviewed changes

rej55 approved these changes Nov 9, 2022

View reviewed changes

remove autoware_auto_common dependency from simple_planning_simulator…

a86aa66

…, osqp_interface Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>

takayuki5168 force-pushed the chore/remove-autoware-auto-common-dependency branch from 3e98ca4 to a86aa66 Compare November 10, 2022 01:25

takayuki5168 enabled auto-merge (squash) November 10, 2022 01:26

takayuki5168 merged commit f07801d into autowarefoundation:main Nov 10, 2022

takayuki5168 deleted the chore/remove-autoware-auto-common-dependency branch November 10, 2022 02:14

tier4-autoware-public-bot bot mentioned this pull request Nov 10, 2022

chore: sync upstream tier4/autoware.universe#173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface #2233

chore: remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface #2233

takayuki5168 commented Nov 7, 2022 •

edited

Loading

takayuki5168 Nov 7, 2022

TakaHoribe Nov 9, 2022

takayuki5168 Nov 9, 2022

TakaHoribe Nov 9, 2022

takayuki5168 Nov 9, 2022

codecov bot commented Nov 7, 2022 •

edited

Loading

takayuki5168 commented Nov 9, 2022

takayuki5168 commented Nov 9, 2022

TakaHoribe Nov 9, 2022 •

edited

Loading

takayuki5168 Nov 9, 2022

TakaHoribe left a comment

rej55 left a comment

chore: remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface #2233

chore: remove autoware_auto_common dependency from simple_planning_simulator and osqp_interface #2233

Conversation

takayuki5168 commented Nov 7, 2022 • edited Loading

Description

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

Post-review checklist for the PR author

takayuki5168 Nov 7, 2022

Choose a reason for hiding this comment

TakaHoribe Nov 9, 2022

Choose a reason for hiding this comment

takayuki5168 Nov 9, 2022

Choose a reason for hiding this comment

TakaHoribe Nov 9, 2022

Choose a reason for hiding this comment

takayuki5168 Nov 9, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 7, 2022 • edited Loading

Codecov Report

takayuki5168 commented Nov 9, 2022

takayuki5168 commented Nov 9, 2022

TakaHoribe Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

takayuki5168 Nov 9, 2022

Choose a reason for hiding this comment

TakaHoribe left a comment

Choose a reason for hiding this comment

rej55 left a comment

Choose a reason for hiding this comment

takayuki5168 commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

TakaHoribe Nov 9, 2022 •

edited

Loading