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_simulation_msgs): add traffic light v1 messages #120

Conversation

HansRobo
Copy link
Member

@HansRobo HansRobo commented May 13, 2024

Signed-off-by: Kotaro Yoshimoto [email protected]

Related Links

Description

The messages for traffic light was changed not to represent physical traffic lights, in autoware_perception_msgs.
This change made difficult to visualize or do something about physical traffic lights based on Autoware's traffic light topic.

These messages are for treating physical traffic light instead of old autoware_auto_perception_msgs.

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Code is properly formatted
  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • Code is properly formatted
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets
  • Write release notes

CI Checks

  • Build and test for PR: Required to pass before the merge.
  • Check spelling: NOT required to pass before the merge. It is up to the reviewer(s). See here if you want to add some words to the spell check dictionary.

@HansRobo HansRobo marked this pull request as ready for review June 3, 2024 05:29
@yukkysaito
Copy link
Collaborator

Which specific types of autoware_auto_msgs are you referring to?
The migration list is written here:
https://github.com/orgs/autowarefoundation/discussions/3862#discussioncomment-9638820

@HansRobo
Copy link
Member Author

HansRobo commented Jun 3, 2024

@yukkysaito
Thank you for the comment!

Which specific types of autoware_auto_msgs are you referring to?

The correspondence with autoware_auto_msgs is as follows:

message in this PR message in autoware_auto_msgs
tier4_simulation_msgs/TrafficLightArrayV1 autoware_auto_perception_msgs//TrafficSignalArray
tier4_simulation_msgs/TrafficLightV1 autoware_auto_perception_msgs//TrafficSignal
tier4_simulation_msgs/TrafficLightBulbV1 autoware_auto_perception_msgs//TrafficLight

The migration list is written here:

I don't intend to do a migration.
One of the reasons for defining new custom messages is to avoid effects from message changes in Autoware.

@mitsudome-r
Copy link
Collaborator

mitsudome-r commented Jun 3, 2024

I think what @HansRobo is trying to represent with this new message is individual traffic light instead of TrafficLightGroup (which is a group of traffic lights with same behavior).
Actually, we have tier4_perception_msgs/TrafficLight to represent this.

@mitsudome-r
Copy link
Collaborator

mitsudome-r commented Jun 3, 2024

Could you also explain if this message will be used to communicate between Autoware and a simulator (e.g., scenario_simulator) or is this just for internal communication within simulator modules?

@HansRobo
Copy link
Member Author

HansRobo commented Jun 3, 2024

Could you also explain if this message will be used to communicate between Autoware and simulator (e.g., scenario_simulator) or is this just for internal communication within simulator modules?

This message is used for output from ScenarioSimulator to external.
However, it is not intended to be used by Autoware, and is intended specifically for external tools such as Autoware Evalautor.

@HansRobo
Copy link
Member Author

HansRobo commented Jun 3, 2024

I think what @HansRobo is trying to represent with this new message is individual traffic light instead of TrafficLightGroup (which is a group of traffic lights with same behavior).

exactly.
Furthermore, the added messages are designed so that external tools can properly detect large message changes as type changes, while small constant changes can be handled without requiring tooling changes.

@yukkysaito
Copy link
Collaborator

@HansRobo Thanks for your reply
What information is missing from the existing TrafficLight?

@HansRobo
Copy link
Member Author

HansRobo commented Jun 3, 2024

@yukkysaito

What information is missing from the existing TrafficLight?

The information is sufficient in the existing TrafficLight.
However, the purpose is to provide additional support like described below and to separate the messages.

Furthermore, the added messages are designed so that external tools can properly detect large message changes as type changes, while small constant changes can be handled without requiring tooling changes.

@HansRobo
Copy link
Member Author

@yukkysaito @mitsudome-r

How should we proceed with this PR?
I understand that there are similar messages, but separating the messages in the first place is meaningful in itself, so for now I'm not considering tier4_perception_msgs/TrafficLight as an option.

@HansRobo
Copy link
Member Author

Note: The visualization of traffic lights on Autoware Evaluator was implemented and released based on these messages.
CC: @vios-fish

@HansRobo
Copy link
Member Author

CC: @YoshinoriTsutake

@HansRobo
Copy link
Member Author

HansRobo commented Jul 4, 2024

The messages are moved into scenario_simulator_v2
tier4/scenario_simulator_v2#1262

@HansRobo HansRobo closed this Jul 4, 2024
@HansRobo HansRobo deleted the feat/add-simulation-traffic-light-v1 branch July 4, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants