feat(ros2): ROS2 refactor with BasePublisher/PublisherImpl templates#9638
feat(ros2): ROS2 refactor with BasePublisher/PublisherImpl templates#9638youtalk wants to merge 16 commits intocarla-simulator:ue5-devfrom
Conversation
Add foundational CRTP template classes for the new ROS2 publisher and subscriber architecture, replacing the old per-sensor PIMPL pattern: - BasePublisher.h: DDS-independent abstract base (topic, frame_id, actor) - PublisherImpl.h: FastDDS CRTP template (DomainParticipant, Publisher, Topic, DataWriter, TypeSupport init/destroy/Publish) - BaseSubscriber.h: DDS-independent abstract base for subscribers - SubscriberImpl.h: FastDDS template (DataReader, on_data_available) These are added alongside the existing CarlaPublisher/CarlaSubscriber classes which will be removed in a later step. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Introduce CarlaCameraPublisher as an abstract base class that manages Image and CameraInfo via two PublisherImpl<T> instances. Migrate CarlaRGBCameraPublisher to a thin subclass providing channel count and encoding. Replace CarlaDepthCameraPublisher, CarlaNormalsCameraPublisher, CarlaOpticalFlowCameraPublisher, CarlaSSCameraPublisher, and CarlaISCameraPublisher with type aliases to CarlaRGBCameraPublisher, eliminating ~2800 lines of duplicated boilerplate. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Rewrite Clock, Collision, GNSS, IMU, Radar, Lidar, SemanticLidar, and Transform publishers to use BasePublisher + PublisherImpl<T> templates, replacing the old PIMPL + manual FastDDS boilerplate pattern. Add new shared base classes: - CarlaPointCloudPublisher: abstract base for point cloud publishers (LiDAR, SemanticLiDAR, DVS, Radar) - CarlaDVSPublisher: composite publisher (Image + PointCloud) replacing the old CarlaDVSCameraPublisher Delete unused publishers and old base classes: - CarlaLineInvasionPublisher (NoopSerializer, unused) - CarlaMapSensorPublisher (NoopSerializer, unused) - CarlaSpeedometerSensor (unused) - CarlaPublisher.h (replaced by BasePublisher.h) - CarlaSubscriber.h (replaced by BaseSubscriber.h) - CarlaDVSCameraPublisher (replaced by CarlaDVSPublisher) Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Delete CarlaListener and CarlaSubscriberListener as PublisherImpl and SubscriberImpl now directly inherit DataWriterListener/DataReaderListener. BasicListener is retained for the WITH_ROS2_DEMO path. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Rewrite CarlaEgoVehicleControlSubscriber to use SubscriberImpl template, add new AckermannControlSubscriber with AckermannDrive/AckermannDriveStamped FastDDS types, and extend ROS2CallbackData variant with AckermannControl. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Remove the 201d375 changes (actor prefix and unregister ordering) from the main refactor to keep them in a separate PR. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Update ROS2 example files from ue4-dev reference: add GNSS and IMU sensors to stack.json configuration, update rviz config with new sensor frames and correct lidar topic path, sync ros2_native.py with upstream, and update README with launch instructions. Build system (CMakeLists.txt) uses dynamic file globs so no changes needed for renamed/added/removed source files. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes. |
- Move ROS2.cpp compilation from LibCarla server to Ros2Native shared library, since publisher headers now include PublisherImpl.h which depends on FastDDS headers - Add rpclib include path to Ros2Native build for MsgPack.h dependency - Make BasicPublisher/BasicSubscriber standalone (no longer inherit from deleted CarlaPublisher/CarlaSubscriber base classes) - Replace CarlaListener usage in BasicPublisher with inline DataWriterListener Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
Although I would like to refine the null pointer dereference in all |
Added a note about the ROS2 refactor and fixed various issues.
…rom* methods GetOrCreateSensor() can return nullptr for unsupported sensor types, and dynamic_pointer_cast can also return nullptr on type mismatch. All 8 ProcessDataFrom* methods were dereferencing the result without null checks, which would cause segfaults at runtime. Add early return guards for: ProcessDataFromCamera, ProcessDataFromGNSS, ProcessDataFromIMU, ProcessDataFromDVS, ProcessDataFromLidar, ProcessDataFromSemanticLidar, ProcessDataFromRadar, ProcessDataFromCollisionSensor. Addresses review feedback from carla-simulator#9638. https://claude.ai/code/session_0113FGAM9Ci8oZTpbPAc3aqP
There was a problem hiding this comment.
Pull request overview
Ports the ROS2-native refactor from the UE4 branch to UE5, replacing the old per-sensor PIMPL-based publishers/subscribers with shared template infrastructure and shifting actor registration toward explicit actor/sensor/vehicle registration APIs.
Changes:
- Introduces
BasePublisher/PublisherImpl<T>andBaseSubscriber/SubscriberImpl<T>to centralize FastDDS lifecycle code and reduce duplication. - Updates UE sensor code paths to call the new
ROS2::ProcessDataFrom*APIs (removingstream_id) and switches actor registration toRegister*/Unregister*. - Adds Ackermann control support (new message types + subscriber + UE-side handler) and updates the ROS2 Python example/configs.
Reviewed changes
Copilot reviewed 93 out of 93 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Server/CarlaServer.cpp | Updates parent ROS registration call to new RegisterActorParent API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/RayCastSemanticLidar.cpp | Removes stream_id usage; forwards transform + data via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/RayCastLidar.cpp | Removes stream_id usage; forwards transform + data via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/Radar.cpp | Removes stream_id usage; forwards transform + data via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/PixelReader.h | Camera publishing now passes transform + buffer only; ROS2 side derives intrinsics from image header. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/ObstacleDetectionSensor.cpp | Removes stream_id usage; forwards transform + event data via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/InertialMeasurementUnit.cpp | Removes stream_id usage; forwards transform + IMU values via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/GnssSensor.cpp | Removes stream_id usage; forwards transform + GNSS data via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/DVSCamera.cpp | Removes stream_id usage; forwards transform + DVS buffer via new ROS2 API. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/CollisionSensor.cpp | Publishes ROS2 collision event using CARLA actor IDs instead of UE UniqueID. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Actor/ActorROS2Handler.h | Adds Ackermann control overload for ROS2 callbacks. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Actor/ActorROS2Handler.cpp | Implements Ackermann control application on ACarlaWheeledVehicle. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Actor/ActorDispatcher.cpp | Switches to RegisterSensor/RegisterVehicle and parent/TF attribute handling. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Actor/ActorDescription.h | Adds GetAttribute helper for variations lookup. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Actor/ActorBlueprintFunctionLibrary.cpp | Adds new ROS2-related actor variations (ros_frame_id, ros_publish_tf) and changes defaults. |
| Ros2Native/LibCarlaRos2Native/CMakeLists.txt | Extends Ros2Native build to compile core ROS2 sources and adds rpclib include path. |
| Ros2Native/CMakeLists.txt | Passes RPCLIB_INCLUDE_PATH into the external Ros2Native build. |
| PythonAPI/examples/ros2/stack.json | Updates example blueprint/sensor config (vehicle type, camera resolution, adds GNSS+IMU). |
| PythonAPI/examples/ros2/rviz/ros2_native.rviz | Updates frames and topic names (e.g., point cloud topic). |
| PythonAPI/examples/ros2/ros2_native.py | Adds egg path bootstrap; adjusts timeouts and minor formatting. |
| PythonAPI/examples/ros2/README.md | Updates run instructions for packaged/editor workflows. |
| LibCarla/source/carla/ros2/types/AckermannDriveStampedPubSubTypes.h | Adds FastDDS-generated PubSubType for AckermannDriveStamped. |
| LibCarla/source/carla/ros2/types/AckermannDriveStampedPubSubTypes.cpp | Adds FastDDS-generated PubSubType implementation for AckermannDriveStamped. |
| LibCarla/source/carla/ros2/types/AckermannDriveStamped.h | Adds FastDDS-generated AckermannDriveStamped message type. |
| LibCarla/source/carla/ros2/types/AckermannDriveStamped.cpp | Adds FastDDS-generated AckermannDriveStamped message implementation. |
| LibCarla/source/carla/ros2/types/AckermannDrivePubSubTypes.h | Adds FastDDS-generated PubSubType for AckermannDrive. |
| LibCarla/source/carla/ros2/types/AckermannDrivePubSubTypes.cpp | Adds FastDDS-generated PubSubType implementation for AckermannDrive. |
| LibCarla/source/carla/ros2/types/AckermannDrive.h | Adds FastDDS-generated AckermannDrive message type. |
| LibCarla/source/carla/ros2/types/AckermannDrive.cpp | Adds FastDDS-generated AckermannDrive message implementation. |
| LibCarla/source/carla/ros2/subscribers/SubscriberImpl.h | Adds template subscriber implementation shared across subscribers. |
| LibCarla/source/carla/ros2/subscribers/CarlaSubscriber.h | Removes legacy subscriber base class. |
| LibCarla/source/carla/ros2/subscribers/CarlaEgoVehicleControlSubscriber.h | Refactors ego vehicle control subscriber to BaseSubscriber + SubscriberImpl. |
| LibCarla/source/carla/ros2/subscribers/CarlaEgoVehicleControlSubscriber.cpp | Implements message conversion + callback dispatch using new subscriber infrastructure. |
| LibCarla/source/carla/ros2/subscribers/BasicSubscriber.h | Removes inheritance from deleted CarlaSubscriber; inlines minimal name/frame fields. |
| LibCarla/source/carla/ros2/subscribers/BaseSubscriber.h | Introduces shared subscriber base for actor pointer + base topic + frame id. |
| LibCarla/source/carla/ros2/subscribers/AckermannControlSubscriber.h | Adds subscriber for AckermannDriveStamped control topic. |
| LibCarla/source/carla/ros2/subscribers/AckermannControlSubscriber.cpp | Implements Ackermann message conversion + callback dispatch. |
| LibCarla/source/carla/ros2/ROS2CallbackData.h | Adds AckermannControl and expands callback variant. |
| LibCarla/source/carla/ros2/ROS2.h | Refactors ROS2 API for actor/sensor/vehicle registration and new ProcessData signatures. |
| LibCarla/source/carla/ros2/publishers/PublisherImpl.h | Adds template publisher implementation shared across publishers. |
| LibCarla/source/carla/ros2/publishers/CarlaTransformPublisher.h | Refactors TF publishing to BasePublisher + PublisherImpl. |
| LibCarla/source/carla/ros2/publishers/CarlaSSCameraPublisher.h | Aliases SS camera publisher to RGB camera publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaSpeedometerSensor.h | Removes unused speedometer publisher (header). |
| LibCarla/source/carla/ros2/publishers/CarlaSpeedometerSensor.cpp | Removes unused speedometer publisher (implementation). |
| LibCarla/source/carla/ros2/publishers/CarlaSemanticLidarPublisher.h | Refactors semantic lidar publisher onto shared point cloud publisher base. |
| LibCarla/source/carla/ros2/publishers/CarlaRGBCameraPublisher.h | Refactors RGB camera publisher onto shared camera publisher base. |
| LibCarla/source/carla/ros2/publishers/CarlaRadarPublisher.h | Refactors radar publisher onto shared point cloud publisher base. |
| LibCarla/source/carla/ros2/publishers/CarlaPublisher.h | Removes legacy publisher base class. |
| LibCarla/source/carla/ros2/publishers/CarlaPointCloudPublisher.h | Adds shared point cloud publisher base (PointCloud2). |
| LibCarla/source/carla/ros2/publishers/CarlaPointCloudPublisher.cpp | Implements point cloud message filling helper. |
| LibCarla/source/carla/ros2/publishers/CarlaOpticalFlowCameraPublisher.h | Aliases optical flow publisher to RGB camera publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaNormalsCameraPublisher.h | Aliases normals publisher to RGB camera publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaMapSensorPublisher.h | Removes unused map sensor publisher (header). |
| LibCarla/source/carla/ros2/publishers/CarlaMapSensorPublisher.cpp | Removes unused map sensor publisher (implementation). |
| LibCarla/source/carla/ros2/publishers/CarlaLineInvasionPublisher.h | Removes unused lane invasion publisher (header). |
| LibCarla/source/carla/ros2/publishers/CarlaLineInvasionPublisher.cpp | Removes unused lane invasion publisher (implementation). |
| LibCarla/source/carla/ros2/publishers/CarlaLidarPublisher.h | Refactors lidar publisher onto shared point cloud publisher base. |
| LibCarla/source/carla/ros2/publishers/CarlaLidarPublisher.cpp | Implements lidar-specific PointCloud2 field definitions + point conversion. |
| LibCarla/source/carla/ros2/publishers/CarlaISCameraPublisher.h | Aliases instance segmentation publisher to RGB camera publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaIMUPublisher.h | Refactors IMU publisher onto BasePublisher + PublisherImpl. |
| LibCarla/source/carla/ros2/publishers/CarlaIMUPublisher.cpp | Implements IMU message filling logic in new publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaGNSSPublisher.h | Refactors GNSS publisher onto BasePublisher + PublisherImpl. |
| LibCarla/source/carla/ros2/publishers/CarlaGNSSPublisher.cpp | Implements GNSS message filling logic in new publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaDVSPublisher.h | Adds combined DVS publisher (image + camera info + point cloud). |
| LibCarla/source/carla/ros2/publishers/CarlaDVSPublisher.cpp | Implements DVS point cloud fields + conversion. |
| LibCarla/source/carla/ros2/publishers/CarlaDVSCameraPublisher.h | Removes legacy DVS camera publisher (header). |
| LibCarla/source/carla/ros2/publishers/CarlaDepthCameraPublisher.h | Aliases depth publisher to RGB camera publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaCollisionPublisher.h | Refactors collision publisher onto BasePublisher + PublisherImpl. |
| LibCarla/source/carla/ros2/publishers/CarlaCollisionPublisher.cpp | Implements collision message filling logic in new publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaClockPublisher.h | Refactors clock publisher onto BasePublisher + PublisherImpl. |
| LibCarla/source/carla/ros2/publishers/CarlaClockPublisher.cpp | Implements clock message filling logic in new publisher. |
| LibCarla/source/carla/ros2/publishers/CarlaCameraPublisher.h | Adds shared camera publisher base (Image + CameraInfo). |
| LibCarla/source/carla/ros2/publishers/CarlaCameraPublisher.cpp | Implements camera info + image message filling logic. |
| LibCarla/source/carla/ros2/publishers/BasicPublisher.h | Removes dependency on deleted CarlaPublisher; inlines minimal name/frame fields. |
| LibCarla/source/carla/ros2/publishers/BasicPublisher.cpp | Replaces removed CarlaListener with a local listener implementation. |
| LibCarla/source/carla/ros2/publishers/BasePublisher.h | Introduces shared publisher base for actor pointer + base topic + frame id. |
| LibCarla/source/carla/ros2/listeners/CarlaSubscriberListener.h | Removes legacy subscriber listener wrapper. |
| LibCarla/source/carla/ros2/listeners/CarlaSubscriberListener.cpp | Removes legacy subscriber listener wrapper implementation. |
| LibCarla/source/carla/ros2/listeners/CarlaListener.h | Removes legacy publisher listener wrapper. |
| LibCarla/source/carla/ros2/listeners/CarlaListener.cpp | Removes legacy publisher listener wrapper implementation. |
| LibCarla/CMakeLists.txt | Stops building ROS2.cpp into LibCarla server; Ros2Native now owns it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const double cx = static_cast<double>(width) / 2.0; | ||
| const double cy = static_cast<double>(height) / 2.0; | ||
| const double fx = static_cast<double>(width) / (2.0 * std::tan(fov) * M_PI / 360.0); |
There was a problem hiding this comment.
The camera intrinsics computation uses std::tan(fov) * M_PI / 360.0, which applies the degrees→radians factor after tan. This yields incorrect fx/fy (and thus incorrect CameraInfo). Convert to radians inside the tan, e.g., tan(fov * M_PI / 360.0) (or use a helper like DegreesToRadians).
| const double fx = static_cast<double>(width) / (2.0 * std::tan(fov) * M_PI / 360.0); | |
| const double fx = static_cast<double>(width) / (2.0 * std::tan(fov * M_PI / 360.0)); |
- ActorDescription: use FindRef for const-correct GetAttribute - ActorDispatcher: use TEXT() macro for string literals - ActorDispatcher: use std::visit instead of boost::variant2::visit - CarlaDVSPublisher: fix descriptor3/descriptor4 copy-paste bug - CarlaDVSPublisher: fix t field datatype (INT64 instead of FLOAT64) - CarlaDVSPublisher: fix uint16_t y-flip underflow, avoid in-place mutation - CarlaCameraPublisher: fix FOV-to-radians conversion order in tan() - CollisionSensor: publish fallback ID for unregistered actors - README: fix executable name CarlaUE4sh -> CarlaUnreal.sh Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
- Change `t` field from non-existent PointField__INT64 to FLOAT64 (the only 8-byte type available in sensor_msgs/PointField) - Change `pol` field from INT8 to UINT8 to match bool semantics Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
@JesusAnaya Could you review this? It is the poring of #9097 and some Copilot review addressing. |
|
Hello @youtalk 👋, sure, I can take a look this afternoon. |
|
Hey @youtalk if you want the PR to be reviwed from CARLA Team you will need to split it as @JesusAnaya has done |
|
My initial motivation was the porting of each critical fix commits from However, if you still think it would be better to split it up, I can try doing that. |
Description
Port the ROS2 architecture overhaul from ue4-dev
cd3d640f3(ROS2 Refactor #9097) to ue5-dev.This replaces the old per-sensor PIMPL pattern with a template-based
BasePublisher/PublisherImpl<T>architecture, eliminating ~4,100 lines of duplicated FastDDS boilerplate.Key changes:
BasePublisher/PublisherImpl<T>for publishers,BaseSubscriber/SubscriberImpl<T>for subscribers — all FastDDS lifecycle management is now sharedCarlaCameraPublisherbase manages Image + CameraInfo; 6 camera publishers reduced to thin subclasses or type aliasesCarlaPointCloudPublisherbase for LiDAR, SemanticLiDAR, Radar, and DVS sensorsRegisterSensor/UnregisterSensor/RegisterVehicle/UnregisterVehiclereplaceAddActorRosName/RemoveActorRosName+stream_id-based managementAckermannControlSubscriber+AckermannDrive/AckermannDriveStampedmessage typesCarlaLineInvasionPublisher,CarlaMapSensorPublisher,CarlaSpeedometerSensorCarlaListener,CarlaSubscriberListenerRelated to #9097
Where has this been tested?
Possible Drawbacks
ProcessDataFrom*method signatures changed (removedstream_idparameter) — out-of-tree code calling these needs updatingCarlaPublisher/CarlaSubscriberbase classes removed — switch toBasePublisher/BaseSubscriberThis change is