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

feat: add new vehicle model #1355

Closed

Conversation

yuki-takagi-66
Copy link

@yuki-takagi-66 yuki-takagi-66 commented Aug 26, 2024

Description

This PR is sync PR to the universe side PR: autowarefoundation/autoware.universe#7651

Abstract

The vehicle model has been updated due to the need to simulate hill-slip behavior.
It is necessary to pass acceleration and gradient separately to the vehicle model.

Background

see universe side PR: autowarefoundation/autoware.universe#7651

Details

References

Destructive Changes

Known Limitations

Copy link

This PR edits vehicle model that is copied from simple_planning_simulator. Please consider making changes to the original code to avoid confusion or consult developers (@hakuturu583, @yamacir-kit and @HansRobo ).

@yuki-takagi-66 yuki-takagi-66 changed the title add new file from psim feat: add new vehicle model Aug 26, 2024
@dmoszynski dmoszynski self-assigned this Oct 14, 2024
@dmoszynski
Copy link
Contributor

@yuki-takagi-66

Vector X order

I noticed that the reason of negative scenarios in these Evaluator results is in EgoEntitySimulation::overwrite() method.

  • The order of the set values was incorrect, in the new model index 5 is PEDAL_ACCX, and index 6 is ACCX.
  • Therefore, I developed a fix that is in this commit (included in this PR).
  • However, I'm wondering why ACCX is not on index 5 as in the other models (see table below), and why PEDAL_ACCX is not added as index 6? I suspect that the X vector for each model should be consistent. This consistency so far has provided the benefit of using [[fallthrough]]; in this code.
  • The change of order in the new model causes inconsistency - thus the mentioned fix is required.
  • Could you let me know if this change of order is intentional? 🙇
SimModelDelaySteerAcc SimModelDelaySteerAccGeared SimModelDelaySteerMapAccGeared SimModelDelaySteerAccGearedWoFallGuard SimModelDelaySteerVel

sim_model_delay_steer_acc

sim_model_delay_steer_acc_geared

sim_model_delay_steer_map_acc_geared

sim_model_delay_steer_acc_geared_wo_fall_guard

sim_model_delay_steer_vel

Source code difference

I believe that the code of vehicle models (simple_planning_simulator) added to scenario_simulator_v2 should be possibly identical to the one from autoware.universe.

image

  • I would also like to ask if this difference in the source code is intentional? 🙇

@yuki-takagi-66
Copy link
Author

@dmoszynski
Thank you

About Vector X order
For the conventional models, ACCX denotes both pedal_position and current acceleration.
However, to handle rolling back on slope, we have to separate these values.
Hence I add the element PEDAL_ACCX
In other words, element ACCX has different meaning for the each vehicle models

There is no reason about the order itself. I re-add ACCX after the first PR is merged.
autowarefoundation/autoware.universe#8607

I think we have a option to exchange ACCX and PEDAL_ACCX, but i feel this do not solve the issue.

About source code difference
Thank you so much.
It is not intentional difference. we should identify scenario_sim with simple_planning_simlator

@dmoszynski
Copy link
Contributor

@yuki-takagi-66 cc @yamacir-kit
The reported issue with Evaluator results has been fixed and I believe that you can resume working on this PR.

In details

In autoware.universe the PR that changes the order to the expected one has been merged, so:

  • I unified the code of the new SimModelDelaySteerAccGearedWoFallGuard between scenario_simulator_v2 and autoware.universe (commit) -

  • I’ve reversed my recent changes that solved the issue of the wrong order - because they are no longer needed (commit).

    • The common use of [[fallthrough]] is back, thanks to the fact that the order is correct now.
  • I’ve conducted local tests using 3 negative scenarios from these Evaluator results in which the issue occurred, all works well, and results Passed.

yuki-takagi-66 and others added 7 commits November 8, 2024 12:00
Signed-off-by: Yuki Takagi <yuki.takagi@tier4.jp>
Signed-off-by: Yuki Takagi <yuki.takagi@tier4.jp>
Signed-off-by: Yuki Takagi <yuki.takagi@tier4.jp>
…teerAccGearedWoFallGuard from simple_planning_simulator from autoware.universe
…ER_ACC_GEARED_WO_FALL_GUARD"

This reverts commit 6113254.
@yuki-takagi-66 yuki-takagi-66 force-pushed the feat/add_new_vehicle_model branch from 0b1e2f0 to 4957043 Compare November 8, 2024 03:00
Copy link

sonarqubecloud bot commented Nov 8, 2024

@github-actions github-actions bot deleted the branch RJD-736/autoware_msgs_support December 10, 2024 10:02
@github-actions github-actions bot closed this Dec 10, 2024
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.

2 participants