RJD-1057 (1/5): Remove non-API member functions: EntityManager’s TrafficLight related member functions

[TauTheLepton]

Description

Abstract

This pull request introduces the change of removing any traffic lights functionality from EntityManager class and encapsulating it inside the new TrafficLights class.
The responsibility of managing traffic lights have been moved to the API.

The change got rid of simulation_api_schema proto messages from traffic_simulator and transitioned to using ROS messages only. Proto messages are now used exclusively in simple_sensor_simulator.

The general goal of this PR is to divide scenario_simulator_v2 into smaller modules with some specific functionality and responsibilities. Apart from module dividing, some code has been refactored to make it cleaner, simpler and easier to read.

Background

This pull request is one of many that aim to modularize the scenario_simulator_v2.

Details

The traffic lights functionality has been aggregated in one master TrafficLights class. This way, all components that make up traffic lights are grouped together and managed by one resource. This master class - along with the responsibility - has been moved to the API. Although the EntityManager does hold a reference to TrafficLights, which is needed by NPCs.

Newly developed classTrafficLights and its components contain some additional functionality that has been spread throughout the whole scenario_simulator_v2 codebase, like the member functionisRequiredStopTrafficLightState or getConventionalTrafficLightsComposedState.

The new traffic lights classes encapsulated by TrafficLights are designed in such a way, that they do not share the underlying TrafficLight object, but act as a middleman modifying the TrafficLight object as needed:

scenario_simulator_v2/simulation/traffic_simulator/include/traffic_simulator/traffic_lights/traffic_lights_base.hpp

Lines 60 to 73 in a06f4b4

	auto isAnyTrafficLightChanged() const -> bool;
	auto isRequiredStopTrafficLightState(const lanelet::Id traffic_light_id) -> bool;
	auto compareTrafficLightsState(const lanelet::Id lanelet_id, const std::string & state) -> bool;
	auto setTrafficLightsColor(
	const lanelet::Id lanelet_id, const traffic_simulator::TrafficLight::Color & color) -> void;
	auto setTrafficLightsState(const lanelet::Id lanelet_id, const std::string & state) -> void;
	auto setTrafficLightsConfidence(const lanelet::Id lanelet_id, const double confidence) -> void;
	auto getTrafficLightsComposedState(const lanelet::Id lanelet_id) -> std::string;

Some additional traffic light message type conversions have been developed in order to get rid of the simulation_api_schema dependency in traffic_simulator and make message managing easier. Also, the traffic lights are now responsible for generating the messages:

scenario_simulator_v2/simulation/traffic_simulator/include/traffic_simulator/traffic_lights/traffic_lights_base.hpp

Lines 75 to 80 in a06f4b4

	auto generateAutowarePerceptionMsg() const -> autoware_perception_msgs::msg::TrafficSignalArray;
	auto generateAutowareAutoPerceptionMsg() const
	-> autoware_auto_perception_msgs::msg::TrafficSignalArray;
	auto generateTrafficSimulatorV1Msg() const -> traffic_simulator_msgs::msg::TrafficLightArrayV1;

Additionally, a dedicated message publisher has been developed for simple_sensor_simulator:

scenario_simulator_v2/simulation/simple_sensor_simulator/include/simple_sensor_simulator/sensor_simulation/traffic_lights/traffic_lights_publisher.hpp

Lines 27 to 53 in a06f4b4

	class TrafficLightsPublisherBase
	{
	public:
	virtual auto publish(
	const rclcpp::Time & current_ros_time,
	const simulation_api_schema::UpdateTrafficLightsRequest & request) const -> void = 0;
	};
	template <typename MessageType>
	class TrafficLightsPublisher : public TrafficLightsPublisherBase
	{
	public:
	template <typename NodeTypePointer>
	explicit TrafficLightsPublisher(const NodeTypePointer & node, const std::string & topic_name)
	: TrafficLightsPublisherBase(),
	traffic_light_state_array_publisher_(
	rclcpp::create_publisher<MessageType>(node, topic_name, rclcpp::QoS(10).transient_local()))
	{
	}
	auto publish(
	const rclcpp::Time & current_ros_time,
	const simulation_api_schema::UpdateTrafficLightsRequest & request) const -> void override;
	private:
	const typename rclcpp::Publisher<MessageType>::SharedPtr traffic_light_state_array_publisher_;
	};

Please note that this PR removes all tests for TrafficLightManager as these tests have no purpose with the new TrafficLights implementation.
New tests have been developed on this branch (here is a diff), but they have not been added to this PR, as this is a large PR by itself as is now.

We are planning to cast a new PR with just the tests. However, if the reviewer would like to have the tests included in this PR as well, we can do that, just please let us know.
Please just note, that then (with tests included) this PR will become very large with over 2000 additions (tests alone have 1000 additions).

Additional note: many of the changes are restructuring, so the code is changed only a little, if at all, but it has been moved to some other places like for example this has been moved here.
Likewise, these type conversions have been moved from publish member functions to appropriate traffic lights functions, which use cast conversions defined here.

References

INTERNAL LINK
PREVIOUS INTERNAL LINK

Regression tests

Regression report
Local tests with deeper regression investigation (no regression confirmed)

Destructive Changes

None

Known Limitations

None

[yamacir-kit]

@TauTheLepton CC: @HansRobo

1. Since the V2I traffic light design is not yet finalized, the publisher of V2I traffic light information is in traffic_simulator. Ideally, however, the traffic_simulator should only depend on messages of protobuf type, and dependence on Autoware messages should be limited to the simple_sensor_simulator and the concealer. In other words, in the future, we would like to move traffic light publishers to simple_sensor_simulator. However, this would require a lot of additional work.
2. In the ideal case above, there is no need to convert the traffic light class data in traffic_simulator directly to Autoware's message type. (The conversion is done to protobuf type to send to simple_sensor_simulator, and then converted from protobuf type to Autoware's message type in simple_sensor_simulator.) Therefore, this pull request introduces a direct conversion from the traffic light class data in traffic_simulator to Autoware's message type, but please change this back. The original conversion is redundant, but until measurements reveal that this is a performance bottleneck, we prefer that the code be compact.

2 is a change request to this pull request. But 1 is not. Do you think you should work on 1 in this pull request? We think it is ok to push it to the queue of items that need to be refactored if we need a lot of additional time on it.

[TauTheLepton]

@yamacir-kit cc @dmoszynski

1. Since the V2I traffic light design is not yet finalized, the publisher of V2I traffic light information is in traffic_simulator. Ideally, however, the traffic_simulator should only depend on messages of protobuf type, and dependence on Autoware messages should be limited to the simple_sensor_simulator and the concealer. In other words, in the future, we would like to move traffic light publishers to simple_sensor_simulator. However, this would require a lot of additional work.

2. In the ideal case above, there is no need to convert the traffic light class data in traffic_simulator directly to Autoware's message type. (The conversion is done to protobuf type to send to simple_sensor_simulator, and then converted from protobuf type to Autoware's message type in simple_sensor_simulator.) Therefore, this pull request introduces a direct conversion from the traffic light class data in traffic_simulator to Autoware's message type, but please change this back. The original conversion is redundant, but until measurements reveal that this is a performance bottleneck, we prefer that the code be compact.

2 is a change request to this pull request. But 1 is not. Do you think you should work on 1 in this pull request? We think it is ok to push it to the queue of items that need to be refactored if we need a lot of additional time on it.

In this PR

The current implementation from this PR is performing the conversions as following
Raw -> Autoware -> Proto
Where

• Raw - traffic light class data
• Autoware - one of Autoware message types
• Proto - Protobuf message type

Prior to this PR

Before the change, the conversions were as following
Raw -> Proto -> Autoware

Where the last step was performed in publishers present in traffic_simulator.

Proposal

If I understand correctly, ultimately the Autoware message publishing should not be performed by traffic_simulator, only the simple_sensor_simulator.

However, recently there was a new ROS message introduced together with ROS publisher in #1262.

Will this message type (and publishing) be ultimately dropped as well, or will it continue to be used?

If this message type will continue to be used:
Maybe instead of going from
Raw -> Autoware -> Proto
to
Raw -> Proto
We could go to
Raw -> Internal -> *
Where

• Internal - The recently introduced internal ROS message type
• * - any other type that could be needed.

This Internal message type could be used as the only internally used type (except from Raw traffic light class) that would be converted to what is needed.

For example, as currently there exists a need to publish some Autoware messages from within the traffic_simulator, there could be a conversion Internal -> Autoware alongside the conversion used to communicate with simple_sensor_simulator using Internal -> Proto.

Any of the conversions from Internal could be dropped at any time, and new ones could be created without any major changes.

Does this change meet your requirements? If not, please let me know your preferred way of making the requested change.

[HansRobo]

@TauTheLepton Thank you for suggestion.
Things are getting a bit complicated, so let me clarify things a bit.

list of components

autoware

Autoware, autonomous driving software stack with localization / sensing / perception / planning / control features.
Depending on the simulation configuration, features such as localization, sensing, and perception may not be used.

traffic_simulator

A component that manages entities etc. according to instructions from openscenario_interpreter.

simulator

A simulator that performs environmental simulation according to commands from the traffic_simulator. In most cases, this is the simple_sensor_simulator , but other simulators such as AWSIM can also be used.

external services

In this context, this refers to services other than those mentioned above that handle traffic light information, such as Autoware Evaluator.

Autoware messages

example	autoware_auto_perception_msgs::TrafficLight
purpose	to provide (V2I) traffic light information from simulator to Autoware
current communication	simulator -> autoware (V2I traffic light: traffic_simulator -> autoware)
ideal communication	simulator -> autoware

Note that AWSIM provides traffic light information through Autoware's image input, so traffic light messages may not necessarily be used.

Internal message

Question: Will this message type (and publishing) be ultimately dropped as well, or will it continue to be used?

Answer: No, internal message will be used continuously.

example	traffic_simulator_msgs::TrafficLightV1
purpose	to provide stable traffic light message for external services like Autoware Evaluator
current communication	traffic_simulator -> external services
ideal communication	traffic_simulator -> external services

Note that this message has no motivation to move to simple_sensor_simulator.

detailed background of the internal message

Each time Autoware's traffic light messages changed, peripheral tools that handle traffic light information, such as scenario_simulator_v2 and Autoware Evaluator (mainly for visualizing traffic lights), were forced to support new message.

While Autoware's traffic light messages are heavily influenced by changes to the internal specifications of the traffic light recognition function, the information required for traffic_simulator and Autoware Evaluator to process has remained almost unchanged long time.

So, the internal message (　traffic_simulator_msgs::TrafficLightV1 ) was provided to provide a traffic light topic with stable specifications from traffic_simulator for Autoware Evaluator, and this internal message will continue to be provided as long as Autoware Evaluator visualizes traffic lights.

proto

example	simulation_api_schema
purpose	to provide traffic light information without ROS, to simulate traffic lights in various simulators
current communication	traffic_simulator -> simulator
ideal communication	traffic_simulator -> simulator

About your proposal

First of all, than you for your suggestion.
And I apologize for not giving you enough information.

Now that we have all the information, it is easier to understand.
The only message that is always used in both traffic_simualtor and simulator( simple_sensor_simulator ) is proto, which shows that proto-centric conversion is simple and effective.

An internal-centric conversion is one option, but you shouldn't bring Internal into the simulator unless you need to.

So again, I want to go back to a proto-centric conversion architecture.

[dmoszynski]

@yamacir-kit @HansRobo
I totally understand what you mean.
Therefore, I've prepared a revision proposal.
The changes to the code were not time-consuming, so I decided to make them at once as part of the presentation of my proposal. It can be seen in this commit. (of course, they have not been fully tested, as this is only a proposal).
Please let me know what you think of this proposal 🙇

Briefly about the changes:

• First of all, I removed the dependence on autoware::msgs - the conversion proto -> autoware::msgs is performed just here.
• Due to the fact that, as I understand it, there are no plans to move publication of traffic_simulator_msgs::TrafficLightV1 to ScenarioSimulator, I left the possibility of casting TrafficLight to traffic_simulator_msgs::TrafficLightV1.
• I've prepared a diagram of the architecture that is currently developed in this merge (see below).
• Now, all communication in TrafficSimulator is based on proto (see green arrows), and autoware::msgs are used only there where publication takes place directly (see blue arrows).
  

Regarding to the move of V2I to ScenarioSimulator,
Could you tell me if in the future it should be implemented in a way similar to that shown in the diagram below? 🙇
If so, I believe that the architecture proposed above will make it quite easy to move V2I to ScenarioSimulator in the future.

[HansRobo]

@dmoszynski CC: @yamacir-kit
Thank you for your understanding and implementation.
Your revision proposal meets our goals, and the code is good for us.
The code has changed significantly since the last regression test, so please run the regression tests again.
Then we can proceed to the merge!

Note

Could you tell me if in the future it should be implemented in a way similar to that shown in the diagram below? 🙇

Yes. Your diagram aligns with our future architecture, and I appreciate your foresight.

[TauTheLepton]

@HansRobo cc @dmoszynski

Regression tests

The regression tests have been performed and they have proven no regressions.

Link to regression results

Note

The regression tests have actually been performed some time ago, more precisely on this commit: de0e44a

After this commit, only some minor changes have been introduced, and we believe these changes have no way of affecting regression tests results.

These changes include:

• Minor refactoring that consists of changing names of 2 variables and moving some using to parent scope: 44a600d
• Removing some unused message headers: b9bdd82
• Changing switch behavior to not use default so that when not all enum cases are considered, the compilation will fail: 91c6b12
  • This change moves the possible error detection from run-time to compilation-time, which is better, because it can be detected earlier (e.g. in GitHub CI)

[sonarcloud]

Quality Gate passed

Issues
22 New issues
0 Accepted issues

Measures
0 Security Hotspots
23.2% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarCloud