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

fix(simple_planning_simulator): set ego pitch to 0 if road slope is not simulated #5501

Conversation

danielsanchezaran
Copy link
Contributor

@danielsanchezaran danielsanchezaran commented Nov 7, 2023

Description

🤖 Generated by Copilot at 684bffd

Refactor simple_planning_simulator_core.cpp to skip ego pitch calculation when road slope simulation is off and use a constant for gravity. This improves performance and accuracy of the simple planning simulator.

Tests performed

I found the bug when using the planning simulator on a specific lanelet map that happened to have inclination (I did not know about the inclination). The result was that the controller output would give negative acceleration and the vehicle would not move after planning a path even when road slope simulation is off . The video shows the bug and the image the control output:

cap-.2023-11-06-17-04-55.mp4

image

I saved the specific starting pose of the vehicle and replayed the planning simulator after applying the fix on this PR and the problem was solved: the vehicle could move after planning. The acceleration is no longer negative either.

cap-.2023-11-07-10-02-30.mp4

Effects on system behavior

When performing simulations, the simulation calculates the acceleration caused by the vehicle's pitch angle, even if the enable_road_slope_simulation_ parameter is set to false which is an unexpected behavior. With this fix, the acceleration caused by pitch will be zero when enable_road_slope_simulation_ is false. So, in those cases, the acceleration given by the controller will change.

Pre-review checklist for the PR author

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

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

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.

@danielsanchezaran danielsanchezaran self-assigned this Nov 7, 2023
@danielsanchezaran danielsanchezaran added tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) and removed component:simulator labels Nov 7, 2023
@danielsanchezaran danielsanchezaran added component:simulation Virtual environment setups and simulations. (auto-assigned) and removed component:simulator labels Nov 7, 2023
@danielsanchezaran danielsanchezaran marked this pull request as ready for review November 7, 2023 01:07
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a3e87cc) 15.37% compared to head (350a81b) 15.38%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5501      +/-   ##
==========================================
+ Coverage   15.37%   15.38%   +0.01%     
==========================================
  Files        1697     1697              
  Lines      117496   117437      -59     
  Branches    37763    37735      -28     
==========================================
+ Hits        18063    18066       +3     
+ Misses      78861    78810      -51     
+ Partials    20572    20561      -11     
Flag Coverage Δ *Carryforward flag
differential 57.18% <50.00%> (?)
total 15.38% <ø> (+<0.01%) ⬆️ Carriedforward from a3e87cc

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

Files Coverage Δ
...ils/include/motion_utils/trajectory/trajectory.hpp 65.65% <ø> (ø)
...alization/gyro_odometer/src/gyro_odometer_core.cpp 47.72% <ø> (+0.71%) ⬆️
...tag_based_localizer/src/ar_tag_based_localizer.cpp 5.14% <ø> (+1.52%) ⬆️
...nning_simulator/simple_planning_simulator_core.cpp 37.53% <50.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@TakaHoribe TakaHoribe left a comment

Choose a reason for hiding this comment

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

LGTM

@TakaHoribe TakaHoribe enabled auto-merge (squash) November 9, 2023 07:31
@danielsanchezaran danielsanchezaran force-pushed the fix/planning-simulator-pitch-angle-use branch from 684bffd to 06f360e Compare November 9, 2023 07:35
@danielsanchezaran danielsanchezaran enabled auto-merge (squash) November 9, 2023 07:36
Signed-off-by: Daniel Sanchez <danielsanchezaran@gmail.com>
@danielsanchezaran danielsanchezaran force-pushed the fix/planning-simulator-pitch-angle-use branch from 06f360e to 350a81b Compare November 9, 2023 07:49
@danielsanchezaran danielsanchezaran merged commit 20ad7c2 into autowarefoundation:main Nov 9, 2023
24 checks passed
@danielsanchezaran danielsanchezaran deleted the fix/planning-simulator-pitch-angle-use branch November 9, 2023 08:29
h-ohta pushed a commit to tier4/autoware.universe that referenced this pull request Nov 20, 2023
…ot simulated (autowarefoundation#5501)

set ego pitch to 0 if road slope is not simulated

Signed-off-by: Daniel Sanchez <danielsanchezaran@gmail.com>
h-ohta added a commit to tier4/autoware.universe that referenced this pull request Nov 21, 2023
…ot simulated (autowarefoundation#5501) (#1027)

set ego pitch to 0 if road slope is not simulated

Signed-off-by: Daniel Sanchez <danielsanchezaran@gmail.com>
Co-authored-by: danielsanchezaran <daniel.sanchez@tier4.jp>
takayuki5168 pushed a commit to tier4/autoware.universe that referenced this pull request Nov 22, 2023
…ot simulated (autowarefoundation#5501)

set ego pitch to 0 if road slope is not simulated

Signed-off-by: Daniel Sanchez <danielsanchezaran@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:simulation Virtual environment setups and simulations. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants