feat(map_based_prediction): consider crosswalks signals #6189

yuki-takagi-66 · 2024-01-26T11:39:59Z

Description

use crosswalk signals to predict the pedestrian paths

Screencast from 01-29-2024 01:10:43 PM.webm

Tests performed

psim tests and tier4 internal tests were perfomed.
https://evaluation.tier4.jp/evaluation/reports/2f207a70-d4d9-51d8-9e5f-37136b92e392?project_id=prd_jt

Notes for reviewers

Interface changes

map_based_prediction becomes to subscribe /perception/traffic_light_recognition/traffic_signals.

Effects on system behavior

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

I've confirmed the contribution guidelines.
The PR follows the pull request guidelines.

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.

codecov · 2024-01-27T03:53:44Z

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (132d7d8) 14.39% compared to head (40fe812) 14.39%.
Report is 18 commits behind head on main.

Files	Patch %	Lines
...based_prediction/src/map_based_prediction_node.cpp	0.00%	28 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6189      +/-   ##
==========================================
- Coverage   14.39%   14.39%   -0.01%     
==========================================
  Files        1906     1906              
  Lines      129860   129888      +28     
  Branches    37582    37582              
==========================================
  Hits        18699    18699              
- Misses      90167    90195      +28     
  Partials    20994    20994

Flag	Coverage Δ		*Carryforward flag
differential	`0.00% <0.00%> (?)`
total	`14.39% <ø> (ø)`		Carriedforward from 132d7d8

*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.

YoshiRi

Since this PR highly depends on map settings and signal detection stability/accuracy, I think it's better to prepare rosparam to switch crosswalk_traffic_light usage like use_crosswalk_signals for easy tuning.
@ktro2828 How do you think about this?

perception/map_based_prediction/include/map_based_prediction/map_based_prediction_node.hpp

YoshiRi · 2024-01-29T13:12:56Z

perception/map_based_prediction/src/map_based_prediction_node.cpp

@@ -872,6 +875,14 @@ void MapBasedPredictionNode::mapCallback(const HADMapBin::ConstSharedPtr msg)
  crosswalks_.insert(crosswalks_.end(), walkways.begin(), walkways.end());
 }

+void MapBasedPredictionNode::trafficSignalsCallback(const TrafficSignalArray::ConstSharedPtr msg)
+{
+  traffic_signal_id_map.clear();


[imho]
According to design description in TIER IV INNER LINK, traffic_signal may drop for various reasons.

I think state estimation or something will be needed for the future.

I think state estimation or something will be needed for the future.

I agree with this idea. Some estimation should be implemented somewhere.
As temporary, state estimation is implemented for crosswalk traffic lights.
Therefore, I think it would be fine to implement the point of concern when it becomes necessary.

perception/map_based_prediction/src/map_based_prediction_node.cpp

ktro2828 · 2024-01-29T15:11:13Z

@yuki-takagi-66 @YoshiRi

Since this PR highly depends on map settings and signal detection stability/accuracy, I think it's better to prepare rosparam to switch crosswalk_traffic_light usage like use_crosswalk_signals for easy tuning.

That is what I was thinking. As @.YoshiRi said, I think It is better to handle the traffic_signal as an optional or control with a parameter.

@yuki-takagi-66 Also I want to ask you about what the goal of this PR is. In this PR do you want to just apply new I/F or implement the algorithm using traffic signal results?

yuki-takagi-66 · 2024-01-30T06:09:42Z

@ktro2828

Also I want to ask you about what the goal of this PR is. In this PR do you want to just apply new I/F or implement the algorithm using traffic signal results?

The goal of this PR is just applying new I/F. To complete this purpose, I feel some of (but small enough) algorithm using traffic signals may be required.

After merging this PR, I intend to proceed in the following steps.

change map_based_prediction I/F (This PR)
re-implement prediction algorithms which were implemented in planning/crosswalk into perception/map_based_prediction
remove some of prediction algorithms implemented in crosswalk/planning

yuki-takagi-66 · 2024-01-30T06:42:36Z

@YoshiRi @ktro2828 About optional param
Thank you for the comments. I agree to prepare a (or some) rosparam to switch the features.
However, I need help determining which functions should be prepared to switch while this PR contains the following features.

Maintain traffic signal (not only crosswalk) information: mainly trafficSignalsCallback()
Check the map to determine if (crosswalk) traffic signals are in place: mainly getTrafficSignalId()
Predict pedestrians behavior using crosswalk traffic signal color: mainly if (crosswalk_signal_id_opt.has_value()) {something;}

How do you think?

ktro2828 · 2024-01-30T09:20:56Z

@ktro2828

Also I want to ask you about what the goal of this PR is. In this PR do you want to just apply new I/F or implement the algorithm using traffic signal results?

The goal of this PR is just applying new I/F. To complete this purpose, I feel some of (but small enough) algorithm using traffic signals may be required.

After merging this PR, I intend to proceed in the following steps.

change map_based_prediction I/F (This PR)

re-implement prediction algorithms which were implemented in planning/crosswalk into perception/map_based_prediction

remove some of prediction algorithms implemented in crosswalk/planning

@yuki-takagi-66 Thank you for explaining the steps you are planning to take for the work. I have no objection about your plan.

However, I need help determining which functions should be prepared to switch while this PR contains the following features.

Maintain traffic signal (not only crosswalk) information: mainly trafficSignalsCallback()

Check the map to determine if (crosswalk) traffic signals are in place: mainly getTrafficSignalId()

Predict pedestrians behavior using crosswalk traffic signal color: mainly if (crosswalk_signal_id_opt.has_value()) {something;}

How do you think?

I believe that No.3 is sufficient to determine whether the node should consider traffic signals. This is contingent upon the node having subscribed to the traffic signal topic for the corresponding crosswalk.
This allows us to achieve our goal without introducing any new parameters.

for (const auto & crosswalk : crosswalks_) {
  /* loads traffic signal id from the cache */
  if (const auto signal_id_opt = getTrafficSignalId(crosswalk); signal_id_opt.has_value()) {
    /* do something */
  }
}

As an optional, we can control the behavior of trafficSignalCallback with a new parameter(bool use_traffic_signal_) as follows.
This callback plays a role in updating the cache of the traffic signal id only when the topic is subscribed and use_traffic_signal_=true.

Note that, it should probably be checked the difference in timestamps between traffic signals and tracked objects when caching traffic signal ids. Then we might be need a parameter named like traffic_signal_time_difference_threshold.

void MapBasedPredictionNode::trafficSignalCallback(const TrafficSignalArray::ConstSharedPtr msg) {
    if (!use_traffic_signal_) { // maybe we don't need this parameter.
      /* caches nothing */
      return;
    } else {
      /* caches traffic signal id ...*/
    }
}

yuki-takagi-66 · 2024-01-31T07:44:46Z

@ktro2828

I believe that No.3 is sufficient to determine whether the node should consider traffic signals. This is contingent upon the node having subscribed to the traffic signal topic for the corresponding crosswalk.
This allows us to achieve our goal without introducing any new parameters.

I understood. @YoshiRi How do you think?

About the difference of timestamps
With the assumption of topic Hz is health enough and traffic_signal_id_map.clear(); is called each steps, I don't feel the need to check the timestamps.
What problems should be caught by this check? Time delay of the previous perception modules?

Signed-off-by: Yuki Takagi <[email protected]>

yuki-takagi-66 · 2024-01-31T12:40:52Z

@ktro2828 @YoshiRi
In response to the reviewers' comments, I modified the code.
Could you check the code aim to approve?

YoshiRi

LGTM

…dation#6189) * consider the crosswalks signals * update with the reviewers comments Signed-off-by: Yuki Takagi <[email protected]>

github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jan 26, 2024

yuki-takagi-66 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jan 26, 2024

yuki-takagi-66 changed the title ~~consider the crosswalks signals~~ consider crosswalks signals Jan 26, 2024

yuki-takagi-66 marked this pull request as ready for review January 29, 2024 04:18

yuki-takagi-66 requested review from ktro2828, kyoichi-sugahara, miursh, takayuki5168, tkimura4 and YoshiRi as code owners January 29, 2024 04:18

yuki-takagi-66 changed the title ~~consider crosswalks signals~~ feat(crosswalk): consider crosswalks signals Jan 29, 2024

YoshiRi reviewed Jan 29, 2024

View reviewed changes

yuki-takagi-66 removed the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jan 30, 2024

yuki-takagi-66 added tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) and removed tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) labels Jan 31, 2024

yuki-takagi-66 added 4 commits January 31, 2024 21:19

consider the crosswalks signals

9096bec

Signed-off-by: Yuki Takagi <[email protected]>

update with the reviewers comments

ea803bb

Signed-off-by: Yuki Takagi <[email protected]>

refactor

68c214b

Signed-off-by: Yuki Takagi <[email protected]>

refactor

40fe812

Signed-off-by: Yuki Takagi <[email protected]>

yuki-takagi-66 force-pushed the feat/map_based_prediction/consider-crosswalk-signal branch from 8049d2e to 40fe812 Compare January 31, 2024 12:34

yuki-takagi-66 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jan 31, 2024

YoshiRi changed the title ~~feat(crosswalk): consider crosswalks signals~~ feat(map_based_prediction): consider crosswalks signals Feb 2, 2024

YoshiRi approved these changes Feb 2, 2024

View reviewed changes

yuki-takagi-66 merged commit 3256acf into autowarefoundation:main Feb 2, 2024
30 of 36 checks passed

yuki-takagi-66 deleted the feat/map_based_prediction/consider-crosswalk-signal branch February 2, 2024 10:33

yuki-takagi-66 mentioned this pull request Feb 2, 2024

feat(map_based_prediction): consider crosswalks signals autowarefoundation/autoware_launch#849

Merged

4 tasks

tier4-autoware-public-bot bot mentioned this pull request Feb 7, 2024

chore: sync upstream tier4/autoware.universe#1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(map_based_prediction): consider crosswalks signals #6189

feat(map_based_prediction): consider crosswalks signals #6189

yuki-takagi-66 commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 27, 2024 •

edited

Loading

YoshiRi left a comment •

edited

Loading

YoshiRi Jan 29, 2024 •

edited

Loading

yuki-takagi-66 Jan 30, 2024

ktro2828 commented Jan 29, 2024 •

edited

Loading

yuki-takagi-66 commented Jan 30, 2024 •

edited

Loading

yuki-takagi-66 commented Jan 30, 2024

ktro2828 commented Jan 30, 2024 •

edited

Loading

yuki-takagi-66 commented Jan 31, 2024

yuki-takagi-66 commented Jan 31, 2024

YoshiRi left a comment

feat(map_based_prediction): consider crosswalks signals #6189

feat(map_based_prediction): consider crosswalks signals #6189

Conversation

yuki-takagi-66 commented Jan 26, 2024 • edited Loading

Description

Related links

Tests performed

Notes for reviewers

Interface changes

Effects on system behavior

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

Post-review checklist for the PR author

codecov bot commented Jan 27, 2024 • edited Loading

Codecov Report

YoshiRi left a comment • edited Loading

Choose a reason for hiding this comment

YoshiRi Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

yuki-takagi-66 Jan 30, 2024

Choose a reason for hiding this comment

ktro2828 commented Jan 29, 2024 • edited Loading

yuki-takagi-66 commented Jan 30, 2024 • edited Loading

yuki-takagi-66 commented Jan 30, 2024

ktro2828 commented Jan 30, 2024 • edited Loading

yuki-takagi-66 commented Jan 31, 2024

yuki-takagi-66 commented Jan 31, 2024

YoshiRi left a comment

Choose a reason for hiding this comment

yuki-takagi-66 commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 27, 2024 •

edited

Loading

YoshiRi left a comment •

edited

Loading

YoshiRi Jan 29, 2024 •

edited

Loading

ktro2828 commented Jan 29, 2024 •

edited

Loading

yuki-takagi-66 commented Jan 30, 2024 •

edited

Loading

ktro2828 commented Jan 30, 2024 •

edited

Loading