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

Refactor CARLA Operator #279

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

pschafhalter
Copy link
Member

@pschafhalter pschafhalter commented Aug 12, 2022

Refactors the CARLA operator by creating drivers for pose, ground truth information, and HD maps. The CarlaOperator will continue to write on vehicle_id_stream once the ego-vehicle is set up.

Changes

Driver Operators

Tested via the MatchesOperator on synchronous and pseudo-async mode.

  • CarlaPoseDriverOperator. Implements pose_stream and pose_stream_for_control.
  • CarlaTrafficLightsDriverOperator. Implements ground_traffic_lights_stream.
  • CarlaObstaclesDriverOperator. Implements ground_obstacles_stream.
  • CarlaSpeedLimitSignsDriverOperator. Implements ground_speed_limit_signs_stream.
  • CarlaStopSignsDriverOperator. Implements ground_stop_signs_stream.
  • CarlaOpenDriveDriverOperator. Implements open_drive_stream.

Other Changes

  • Implement __eq__ for Pose for testing.
  • Update CarlaOperator
  • Deprecate global_trajectory_stream from the CarlaOperator. Replace with an IngestStream as it only sends a top watermark.
  • Remove MatchesOperator once all drivers are tested.

@pschafhalter pschafhalter changed the title [WIP] Refactor CARLA Operator Refactor CARLA Operator Aug 30, 2022
Copy link
Collaborator

@sukritkalra sukritkalra left a 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.

Comment on lines +892 to +895
if isinstance(other, Pose):
return (self.transform == other.transform
and self.forward_speed == other.forward_speed
and self.velocity_vector == other.velocity_vector)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Comment on lines +1110 to +1115
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)))
Copy link
Collaborator

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?

Copy link
Member Author

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.

Comment on lines +12 to +14
class CarlaObstaclesDriverOperator(CarlaBaseGNSSDriverOperator):
"""Publishes the bounding boxes of all vehicles and people retrieved from
the simulator at the provided frequency.
Copy link
Collaborator

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.

Copy link
Member Author

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:

  1. 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.
  2. 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.
  3. 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 rename process_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.

Copy link
Collaborator

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?

Copy link
Member Author

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.

Comment on lines +28 to +31
gnss_setup = pylot.drivers.sensor_setup.GNSSSetup(
self.config.name, transform)
super().__init__(vehicle_id_stream, obstacles_stream, gnss_setup,
frequency, flags)
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

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.

2 participants