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(parameters_logging_visualization): log and visualize test iteration parameters #1360

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ahmeddesokyebrahim
Copy link

Description

fixes #1272

Abstract

This PR is to add the ability for scenario simulator to report the parameters used in each iteration as described in this feature request.
The PR is adding this parameters reporting using both command line print and rviz plugin visualization that can enabled when needed

Background

Please refer to the feature request.

Details

Using this PR, the parameters are printed to command line and visualized in RVIZ as follows :

  • RVIZ :
    image
  • Terminal :
    image

References

N.A

Destructive Changes

N.A

Known Limitations

@hakuturu583 hakuturu583 added the bump patch If this pull request merged, bump patch version of the scenario_simulator_v2 label Sep 2, 2024
@yamacir-kit
Copy link
Collaborator

@ahmeddesokyebrahim
We are not willing to review code that has not passed CI. Please fix it.

@HansRobo
Copy link
Member

HansRobo commented Sep 3, 2024

Note

You should fix the "Build and run" CI only.
We are currently working on an issue where the Release CI doesn't work properly with PRs from external contributors.
#1248

@ahmeddesokyebrahim
Copy link
Author

As I am not able to re-trigger the Build and run CI again, would it be possible from any of the maintainers or reviewers to run it again to see how it is succeeding after the latest fixes ?

I appreciate your support !!

cc : @mitsudome-r -san - @hakuturu583 -san - @yamacir-kit -san - @HansRobo -san.

@mitsudome-r
Copy link
Contributor

@ahmeddesokyebrahim I have triggered the CI

@ahmeddesokyebrahim ahmeddesokyebrahim force-pushed the 1272-feat-params-vis-log branch 3 times, most recently from 50ad4b3 to dcc8052 Compare September 19, 2024 04:59
@ahmeddesokyebrahim
Copy link
Author

I would highly appreciate if anyone can trigger the build & run CI. Much appreciated !!
@mitsudome-r -san - @hakuturu583 -san - @yamacir-kit -san - @HansRobo -san.

Copy link

sonarcloud bot commented Sep 20, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

@ahmeddesokyebrahim
Copy link
Author

The build and run CI passed but "check branch is up to date" did not as the master branch was updated after my changes.
I have updated the branch right now and would be very grateful for anyone who can run the checks again.

cc : @mitsudome-r -san - @hakuturu583 -san - @yamacir-kit -san - @HansRobo -san.

@HansRobo
Copy link
Member

@ahmeddesokyebrahim
OK, I approved to run CIs.

@ahmeddesokyebrahim
Copy link
Author

All the CI checks are passing (except the the release one as clarified here).

I would appreciate your time looking into the PR giving your comments and thoughts.

cc : @mitsudome-r -san - @hakuturu583 -san - @yamacir-kit -san - @HansRobo -san.

Copy link
Collaborator

@yamacir-kit yamacir-kit left a comment

Choose a reason for hiding this comment

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

I understand the need that more information about the scenario should be displayed in RViz. And I also understand that adding more information, even if it is less complete, will help the user. However, we do not have the capacity to improve such features that are not necessarily necessary for the scenario to work. Therefore, once this feature is accepted, it is likely to be left with only minimal maintenance to prevent compilation errors.

If you have a serious need for this feature, please follow the policy below to brush up on this feature.

  • It only addresses the visualization of the top-level parameters of OpenSCENARIO XML. Perhaps you do not want to visualize OpenSCENARIO XML ParameterDeclarations, but T4v2 ScenarioModifiers of the TIER IV extension. T4v2 ScenarioModifiers is a feature handled by scenario_test_runner, not openscenario_interpreter.

  • We do not want to add anything to the interpreter other than the code that interprets the scenario as much as possible. If you want to visualize scenario information, it should be based on the information in the topic /simulation/context. However, since this topic is also linked to Scenario Play on Autoware Evaluator, changing the content of this topic would require more than just adding a single feature.

  • Please reconsider whether this functionality needs to be implemented independently of the existing visualization functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch If this pull request merged, bump patch version of the scenario_simulator_v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancing scenario_simulator_v2 to print or visualize parameters/modifiers used for test iterations
5 participants