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

Draft
wants to merge 7 commits into
base: RJD-736/autoware_msgs_support
Choose a base branch
from

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.

Copy link

sonarcloud bot commented Nov 8, 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