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): change orger of IDX in SimModelDelayS… #9128

Conversation

dmoszynski
Copy link
Contributor

Description

During work with vehicle models from simple_planning_simulator,
I noticed that practically all models use a standardized form of the IDX vector - see the table below.
Such approach (promise) certainly makes sense if we want to make sure, for example, that YAW is always at index 2 of the IDX vector.

This is especially useful for the case where in one common part of the code (method) we use different vehicle models and want to use IDX vector indexes (without using enums classes).

However, this approach (promise) is broken in the case of the recently added vehicle model SimModelDelaySteerAccGearedWoFallGuard - see table below.
In SimModelDelaySteerAccGearedWoFallGuard ACCX is not on index 5 as in any other model but on index 6.
On index 5 is the newly added PEDAL_ACCX.

This break in the IDX vector form standardization, makes the code prone to bugs, for this reason I propose to provide such standardization also for the SimModelDelaySteerAccGearedWoFallGuard vehicle model.

In this PR, I simply change the order of the IDX vector in the SimModelDelaySteerAccGearedWoFallGuard model.

SimModelActuationCmd SimModelDelaySteerAcc SimModelDelaySteerAccGeared SimModelDelaySteerAccGearedWoFallGuard SimModelDelaySteerMapAccGeared SimModelDelaySteerVel SimModelIdealSteerAcc SimModelIdealSteerAccGeared SimModelIdealSteerVel

Screenshot from 2024-10-21 11-02-44

sim_model_delay_steer_acc

sim_model_delay_steer_acc_geared

sim_model_delay_steer_acc_geared_wo_fall_guard

sim_model_delay_steer_map_acc_geared

sim_model_delay_steer_vel

Screenshot from 2024-10-21 11-03-47

Screenshot from 2024-10-21 11-03-47

Screenshot from 2024-10-21 11-04-56

Related links

None.

How was this PR tested?

The Autoware operation with modified vehicle model SimModelDelaySteerAccGearedWoFallGuard has been tested using scenario_simulator_v2, more precisely this scenario and this branch - video below. (tier4 access rights may be required).

after.mp4

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

The SimModelDelaySteerAccGearedWoFallGuard model vehicle has the order of the IDX vector changed - in order to try to maintain the promise.

…teerAccGearedWoFallGuard

Signed-off-by: Dawid Moszynski <dawid.moszynski@robotec.ai>
@github-actions github-actions bot added the component:simulation Virtual environment setups and simulations. (auto-assigned) label Oct 21, 2024
Copy link

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@mitsudome-r mitsudome-r added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Oct 29, 2024
@xmfcx xmfcx added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Oct 29, 2024
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.33%. Comparing base (b4d8e7c) to head (622232c).
Report is 62 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9128   +/-   ##
=======================================
  Coverage   28.33%   28.33%           
=======================================
  Files        1304     1306    +2     
  Lines      101077   101105   +28     
  Branches    39249    39250    +1     
=======================================
+ Hits        28641    28652   +11     
- Misses      69685    69703   +18     
+ Partials     2751     2750    -1     
Flag Coverage Δ *Carryforward flag
differential 24.11% <ø> (?)
total 28.33% <ø> (ø) Carriedforward from b4d8e7c

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

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

@dmoszynski dmoszynski removed their assignment Oct 30, 2024
@dmoszynski
Copy link
Contributor Author

@xmfcx
Thank you so much for taking care of this PR. 🙇
By mistake I've removed the assignment of me in this PR - and I can not bring it back. 😞

I would like to confirm that I've been performing tests and reviewing the code to make sure that a numeric value (int) is not being used directly instead of an enum. I did not find the use of the numeric value - the enum is used everywhere in autoware.universe.

@xmfcx
Copy link
Contributor

xmfcx commented Oct 30, 2024

Thank you very much @dmoszynski for the contribution 🙇

@tkimura4 @zulfaqar-azmi-t4 @soblin I will merge it since it is passing the CI and it's a clear mis-ordering of the enums.

Please check within internal TIER IV projects if it is used in any other way and feel free to revert if so.

@xmfcx xmfcx merged commit 0280e0b into autowarefoundation:main Oct 30, 2024
46 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:planning Route planning, decision-making, and navigation. (auto-assigned) component:simulation Virtual environment setups and simulations. (auto-assigned) 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.

3 participants