-
Notifications
You must be signed in to change notification settings - Fork 132
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
Refactor CARLA Operator #279
base: master
Are you sure you want to change the base?
Refactor CARLA Operator #279
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked over the entire PR yet, but it would be helpful for me to get the answers to the following questions to understand the migration architecture.
if isinstance(other, Pose): | ||
return (self.transform == other.transform | ||
and self.forward_speed == other.forward_speed | ||
and self.velocity_vector == other.velocity_vector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isinstance(other, Pose): | |
return (self.transform == other.transform | |
and self.forward_speed == other.forward_speed | |
and self.velocity_vector == other.velocity_vector) | |
if isinstance(other, Pose): | |
return (self.transform == other.transform | |
and self.forward_speed == other.forward_speed | |
and self.velocity_vector == other.velocity_vector) | |
return NotImplemented |
def matches_data(left_msgs, right_msgs): | ||
length_matches = (len(left_msgs) == len(right_msgs)) | ||
return length_matches and all( | ||
map( | ||
lambda x: x[0].timestamp == x[1].timestamp and x[0].data == x[1]. | ||
data, zip(left_msgs, right_msgs))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for shadow testing and will be removed once the PR is approved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is to check that the messages sent and received are the same.
class CarlaObstaclesDriverOperator(CarlaBaseGNSSDriverOperator): | ||
"""Publishes the bounding boxes of all vehicles and people retrieved from | ||
the simulator at the provided frequency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these operators subclassed from a BaseGNSSDriverOperator
? They don't seem to be using the gnss_msg
exposed in the process_gnss
method and retrieve different data from the simulator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought process is the following:
- The original CARLA operator performs complicated logic to send data from the simulator (e.g. pose, ground obstacles, etc) at multiple frequencies (localization and control). This logic is coupled with how the operator ticks the simulator.
- Sensors can define tick frequencies independently. Therefore, we can set up a lightweight sensor such as GNSS which calls the logic for sending arbitrary information at arbitrary frequencies.
- The logic for setting up the GNSS sensor is identical across several operators, so I moved the shared logic to
BaseGNSSDriverOperator
. I could also move it to a helper method or renameprocess_gnss
, if you think that's cleaner.
I checked if any other actors can invoke callbacks at configurable frequencies, but it appears that this is limited to sensors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the GNSS sensor is being used as a timestamping mechanism? Why not just have a base operator that sends its downstream operators a watermark message to retrieve and release data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works too, but it would still share logic with the GNSS sensor operator, and we'd still need to instantiate one for each downstream operator frequency. So the question is whether the data release mechanism should be a standalone operator or a coupled with the driver operators.
Personally, I think it makes sense to put the logic in the driver operator so that the API reflects the other driver operators and simplifies pipeline architecture.
gnss_setup = pylot.drivers.sensor_setup.GNSSSetup( | ||
self.config.name, transform) | ||
super().__init__(vehicle_id_stream, obstacles_stream, gnss_setup, | ||
frequency, flags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that a new GNSS
will be installed for every operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Refactors the CARLA operator by creating drivers for pose, ground truth information, and HD maps. The
CarlaOperator
will continue to write onvehicle_id_stream
once the ego-vehicle is set up.Changes
Driver Operators
Tested via the
MatchesOperator
on synchronous and pseudo-async mode.CarlaPoseDriverOperator
. Implementspose_stream
andpose_stream_for_control
.CarlaTrafficLightsDriverOperator
. Implementsground_traffic_lights_stream
.CarlaObstaclesDriverOperator
. Implementsground_obstacles_stream
.CarlaSpeedLimitSignsDriverOperator
. Implementsground_speed_limit_signs_stream
.CarlaStopSignsDriverOperator
. Implementsground_stop_signs_stream
.CarlaOpenDriveDriverOperator
. Implementsopen_drive_stream
.Other Changes
__eq__
forPose
for testing.CarlaOperator
global_trajectory_stream
from theCarlaOperator
. Replace with anIngestStream
as it only sends a top watermark.MatchesOperator
once all drivers are tested.