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(map_based_prediction): consider crosswalks signals #6189

Conversation

yuki-takagi-66
Copy link
Contributor

@yuki-takagi-66 yuki-takagi-66 commented Jan 26, 2024

Description

use crosswalk signals to predict the pedestrian paths

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

Related links

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.

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 the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jan 26, 2024
@yuki-takagi-66 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 yuki-takagi-66 changed the title consider the crosswalks signals consider crosswalks signals Jan 26, 2024
Copy link

codecov bot commented Jan 27, 2024

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.

@yuki-takagi-66 yuki-takagi-66 marked this pull request as ready for review January 29, 2024 04:18
@yuki-takagi-66 yuki-takagi-66 changed the title consider crosswalks signals feat(crosswalk): consider crosswalks signals Jan 29, 2024
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

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?

@@ -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();
Copy link
Contributor

@YoshiRi YoshiRi Jan 29, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@ktro2828
Copy link
Contributor

ktro2828 commented Jan 29, 2024

@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
Copy link
Contributor Author

yuki-takagi-66 commented Jan 30, 2024

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

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

@yuki-takagi-66
Copy link
Contributor Author

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

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

How do you think?

@ktro2828
Copy link
Contributor

ktro2828 commented Jan 30, 2024

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

  1. change map_based_prediction I/F (This PR)
  2. re-implement prediction algorithms which were implemented in planning/crosswalk into perception/map_based_prediction
  3. 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.

  1. Maintain traffic signal (not only crosswalk) information: mainly trafficSignalsCallback()
  2. Check the map to determine if (crosswalk) traffic signals are in place: mainly getTrafficSignalId()
  3. 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 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
Copy link
Contributor Author

@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?

@yuki-takagi-66 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
Signed-off-by: Yuki Takagi <[email protected]>
Signed-off-by: Yuki Takagi <[email protected]>
@yuki-takagi-66 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
Copy link
Contributor Author

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

@yuki-takagi-66 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 YoshiRi changed the title feat(crosswalk): consider crosswalks signals feat(map_based_prediction): consider crosswalks signals Feb 2, 2024
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

LGTM

@yuki-takagi-66 yuki-takagi-66 merged commit 3256acf into autowarefoundation:main Feb 2, 2024
30 of 36 checks passed
@yuki-takagi-66 yuki-takagi-66 deleted the feat/map_based_prediction/consider-crosswalk-signal branch February 2, 2024 10:33
anhnv3991 pushed a commit to anhnv3991/autoware.universe that referenced this pull request Feb 13, 2024
…dation#6189)

* consider the crosswalks signals
* update with the reviewers comments

Signed-off-by: Yuki Takagi <[email protected]>
yuki-takagi-66 added a commit to tier4/autoware.universe that referenced this pull request Feb 19, 2024
…dation#6189)

* consider the crosswalks signals
* update with the reviewers comments

Signed-off-by: Yuki Takagi <[email protected]>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
…dation#6189)

* consider the crosswalks signals
* update with the reviewers comments

Signed-off-by: Yuki Takagi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag: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