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(tier4_vehicle_rviz_plugin): add acceleration visualization plugin to RViz #4506

Merged

Conversation

Owen-Liuyuxuan
Copy link
Contributor

@Owen-Liuyuxuan Owen-Liuyuxuan commented Aug 3, 2023

Description

Add an acceleration meter plugin for debugging on RVIZ.

Tests performed

PSimulation and Rosbag simulation.

Interface changes

The new plugin relies on topics like /localization/acceleration, and expose plugin parameters as described in changes in README.

Effects on system behavior

A new panel recording acceleration is expected to opened on the right of speed panel.
Screenshot from 2023-08-04 13-48-00

The center text should turn red (by default) if acceleration is larger than a max-threshold (re-configurable on Rviz), or smaller than a min-threshold. (In the screenshot we manually reconfigured the thresholds to demonstrate the behaviour)
Screenshot from 2023-08-04 13-48-45

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.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

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.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) labels Aug 3, 2023
@Owen-Liuyuxuan Owen-Liuyuxuan force-pushed the feat/acceleartion_rviz branch from cdd039d to edd6673 Compare August 3, 2023 07:36
@github-actions github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Aug 3, 2023
@Owen-Liuyuxuan Owen-Liuyuxuan force-pushed the feat/acceleartion_rviz branch from edd6673 to e544238 Compare August 3, 2023 07:37
@github-actions github-actions bot removed the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Aug 3, 2023
@takayuki5168 takayuki5168 added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 3, 2023
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

Thank you for your PR 👍

My Feedback:
Is it possible to remove the limit part from this plugin as I would like to create it as a separate plugin?
https://github.com/autowarefoundation/autoware.universe/pull/4506/files#diff-b668fdfacd30844d8af0cdcaee2742a5c251f497dbf82ebbbd2793d88a9aceb1R182

Since the limiting acceleration is specified by a parameter, it is better to indicate it.

@Owen-Liuyuxuan
Copy link
Contributor Author

Thank you for your PR 👍

My Feedback: Is it possible to remove the limit part from this plugin as I would like to create it as a separate plugin? https://github.com/autowarefoundation/autoware.universe/pull/4506/files#diff-b668fdfacd30844d8af0cdcaee2742a5c251f497dbf82ebbbd2793d88a9aceb1R182

Since the limiting acceleration is specified by a parameter, it is better to indicate it.

Agree, it is not necessary to have a limit here. I will try update these parts.

@shmpwk shmpwk changed the title feat: add acceleration visualization plugin to RVIZ feat(tier4_vehicle_rviz_plugin): add acceleration visualization plugin to RVIZ Aug 3, 2023
@Owen-Liuyuxuan Owen-Liuyuxuan force-pushed the feat/acceleartion_rviz branch from 88bcdbb to edd6673 Compare August 3, 2023 08:00
@github-actions github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Aug 3, 2023
@shmpwk
Copy link
Contributor

shmpwk commented Aug 3, 2023

(It is ok not to force push the new commit but just add another commit because we will squash merge this PR.)

@Owen-Liuyuxuan
Copy link
Contributor Author

Change two places:

  1. Fix a bugged magic number (0.08) in line 178 with property_label_scale_;
  2. Remove the limit text in line 182.

@shmpwk
Copy link
Contributor

shmpwk commented Aug 3, 2023

@Owen-Liuyuxuan
Could you exclude the commit 4edf9f8 ?
Maybe you have to rebase main and force push.

@Owen-Liuyuxuan Owen-Liuyuxuan force-pushed the feat/acceleartion_rviz branch from f72d868 to 8076aab Compare August 3, 2023 08:16
@github-actions github-actions bot removed the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Aug 3, 2023
@shmpwk shmpwk changed the title feat(tier4_vehicle_rviz_plugin): add acceleration visualization plugin to RVIZ feat(tier4_vehicle_rviz_plugin): add acceleration visualization plugin to RViz Aug 3, 2023
@shmpwk
Copy link
Contributor

shmpwk commented Aug 3, 2023

@Owen-Liuyuxuan
Could you check failing spell check CI ?

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.03% ⚠️

Comparison is base (73726bd) 14.89% compared to head (8336231) 14.86%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4506      +/-   ##
==========================================
- Coverage   14.89%   14.86%   -0.03%     
==========================================
  Files        1519     1520       +1     
  Lines      104972   105123     +151     
  Branches    31986    31986              
==========================================
  Hits        15631    15631              
- Misses      72274    72425     +151     
  Partials    17067    17067              
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 14.89% <ø> (ø) Carriedforward from 02a3a8f

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

Files Changed Coverage Δ
...hicle_rviz_plugin/src/tools/acceleration_meter.cpp 0.00% <0.00%> (ø)

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

@shmpwk
Copy link
Contributor

shmpwk commented Aug 3, 2023

@Owen-Liuyuxuan
You can ignore codecov fail for now. It is not required to merge PR.

@shmpwk
Copy link
Contributor

shmpwk commented Aug 3, 2023

@Owen-Liuyuxuan

Looks like even if the acceleration is negative value, the meter hand is shown on the right side.
image

Owen-Liuyuxuan and others added 5 commits August 3, 2023 19:00
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
… property_label_scale_ not responding

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
…text of acceleration meter, and delte the parameter of property_label_value

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
@Owen-Liuyuxuan Owen-Liuyuxuan force-pushed the feat/acceleartion_rviz branch from 14a8de5 to 02a3a8f Compare August 3, 2023 11:00
@shmpwk
Copy link
Contributor

shmpwk commented Aug 3, 2023

@Owen-Liuyuxuan

(Please check tomorrow.)
Could you also create for RViz viewer ?
https://github.com/autowarefoundation/autoware_launch/blob/main/autoware_launch/rviz/autoware.rviz

  • The acceleration meter is disable by default
  • The acceleration meter is aligned with other meters in the RViz Displays.

Owen-Liuyuxuan and others added 2 commits August 4, 2023 13:20
…/min acceration; set threshold for reconfiguring emergency speed

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
@shmpwk
Copy link
Contributor

shmpwk commented Aug 4, 2023

@Owen-Liuyuxuan
Thank you for your fixing.
Could you update the screen shot of PR description ?

-> updated already

Copy link
Contributor

@shmpwk shmpwk left a comment

Choose a reason for hiding this comment

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

LGTM! I also tested with Psim.
Wating for @yukkysaito -san approval.

Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

@shmpwk shmpwk merged commit e39c928 into autowarefoundation:main Aug 4, 2023
shmpwk pushed a commit to autowarefoundation/autoware_launch that referenced this pull request Aug 4, 2023
… default (#499)

* feat: add acceleration meter for debugging, disabled by default; autowarefoundation/autoware.universe#4506

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* not directly setting pixel numbers in rviz for display on screen with various resolutions

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

---------

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Co-authored-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
@Owen-Liuyuxuan Owen-Liuyuxuan deleted the feat/acceleartion_rviz branch August 4, 2023 09:44
LeoDriveProject pushed a commit to leo-drive/autoware.universe.golf that referenced this pull request Aug 16, 2023
…n to RViz (autowarefoundation#4506)

* feat: add acceleration visualization plugin to RVIZ

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* feat: add RVIZ plugin for acceleration; remove limit text; debugging: property_label_scale_ not responding

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* style(pre-commit): autofix

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* fix typo in acceleration

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* fix a bug in keeping using abs(accel) to compute meter angle; delete text of acceleration meter, and delte the parameter of property_label_value

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* feat: separate the setting of max/min emergency threshold; update max/min acceration; set threshold for reconfiguring emergency speed

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Co-authored-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants