fix(ros2): move ROS2 unregistration before actor deregistration#9640
Draft
youtalk wants to merge 11 commits intocarla-simulator:ue5-devfrom
Draft
fix(ros2): move ROS2 unregistration before actor deregistration#9640youtalk wants to merge 11 commits intocarla-simulator:ue5-devfrom
youtalk wants to merge 11 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>
Move ROS2 UnregisterSensor/UnregisterVehicle calls inside the CarlaActor null check, before Registry.Deregister. This prevents a potential null pointer dereference in multi-GPU setups where CarlaActor may be null when OnActorDestroyed fires. Also add "actor" prefix to auto-generated ROS names for clarity. Port of ue4-dev commit 201d375. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
youtalk
force-pushed
the
fix/ros2-multigpu-actor-destroy
branch
from
3f62db3 to
0eb0e90
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Port multi-GPU actor destroy fix from ue4-dev
201d375d3to ue5-dev.Moves ROS2
UnregisterSensor/UnregisterVehiclecalls inside theCarlaActornull check inOnActorDestroyed, beforeRegistry.Deregister. This prevents a potential null pointer dereference in multi-GPU setups whereCarlaActormay be null whenOnActorDestroyedfires.Also adds "actor" prefix to auto-generated ROS names for clarity.
Depends on #9638
Diff from #9638:
3f62db3Where has this been tested?
Possible Drawbacks
<id>toactor<id>— downstream code parsing topic names may need updatingThis change is