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

Make TopDownMultiChannel support ScenarioEnv #498

Merged
merged 8 commits into from
Oct 18, 2023

Conversation

pimpale
Copy link
Collaborator

@pimpale pimpale commented Sep 19, 2023

What changes do you make in this PR?

In this PR, I add support for using the TopDownMultiChannel observation together with ScenarioEnv.

Changes:

  1. Drawing lane boundaries: previously, TopDownMultiChannel only supported NodeRoadNetwork.
    However, ScenarioEnv uses EdgeRoadNetwork.
    My PR adds support for fetching the lane and road boundaries to draw from the map manager.

  2. Drawing vehicles: previously, TopDownMultiChannel selected the vehicles to draw from the traffic manager. ScenarioEnv uses scenario_traffic_manager, not traffic_manager. I add support for using scenario_traffic_manager if traffic_manager does not exist.

    • I also add a vehicles property to ScenarioTrafficManager

Checklist

  • I have merged the latest main branch into current branch.
  • I have run bash scripts/format.sh before merging.
  • Please use "squash and merge" mode.

checkpoints = self.target_vehicle.navigation.checkpoints
for i, c in enumerate(checkpoints[:-1]):
lanes = self.road_network.graph[c][checkpoints[i + 1]]
for lane in lanes:
LaneGraphics.draw_drivable_area(lane, canvas, color=color)

def draw_navigation_trajectory(self, canvas, color=(128, 128, 128)):
lane = PointLane(self.target_vehicle.navigation.checkpoints, DEFAULT_TRAJECTORY_LANE_WIDTH)
Copy link
Member

Choose a reason for hiding this comment

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

There is already an ego car trajectory in the simulator called engine.map_manager.current_sdc_route. But it is ok to use either of the two.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense!

@@ -162,6 +163,10 @@ def sdc_object_id(self):
def current_scenario_length(self):
return self.engine.data_manager.current_scenario_length

@property
def vehicles(self):
Copy link
Member

Choose a reason for hiding this comment

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

As there are more objects in a traffic scenario now, drawing only vehicles is not enough. It would be better to draw all objects including cones, barriers, pedestrians, and bikes. So I suggest drawing all objects like what we did in the Top-Down renderer. This can be done by accessing all objects via engine._spawned_objects or engine.get_objects() and draw them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have done so now!

if hasattr(self.engine, "traffic_manager"):
vehicles = self.engine.traffic_manager.vehicles
elif hasattr(self.engine, "scenario_traffic_manager"):
vehicles = self.engine.scenario_traffic_manager.vehicles
Copy link
Member

Choose a reason for hiding this comment

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

As I said before, let's draw all other things besides vehicles. A suggestion here is that we can actually cancel this if condition. For both PGEnvironment and ScenarioEnvironment, all objects are actually in the engine._spawned_objects. What a manager does is a set of rules to add, delete, or actuate objects. Thus we can access all objects through env.engine directly, regardless of what the traffic manager is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

elif hasattr(self.engine, "map_manager"):
for data in self.engine.map_manager.current_map.blocks[-1].map_data.values():
if ScenarioDescription.POLYLINE in data:
LaneGraphics.display_scenario(
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember what the figure drawn with this code looks like, so I am not sure if display_scenario achieves what you want. If not, just check this implementation in the if semantic_map:

It gives you filled polygons to show maps and lines to represent lines like this:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I use LaneGraphics.display_scenario_line to draw the lines, I get this image:

image

Ideally, it would have the same spacing between stripes as LaneGraphics.striped_line, so I'll work on ensuring that.

@QuanyiLi
Copy link
Member

Actually, this PR reminds me of a stale issue #455. We have almost the same set of functions/implementations for the top-down renderer and top-down observation. I think we should merge them someday.

@QuanyiLi
Copy link
Member

@pengzhenghao Any suggestions? I think the top-down renderer and top-down obs are maintained by you. Could you reveal how many differences exist between these two sets of implementations? If not too much, we should merge them, and acquire the top down observation from the top-down renderer.

@QuanyiLi
Copy link
Member

I changed the API display_scenario to display_scenario_line. Besides, the implementation and arguments are changed as well. Please update related code.

@QuanyiLi
Copy link
Member

Also, it is good to add a test for this function.

@pimpale
Copy link
Collaborator Author

pimpale commented Sep 22, 2023

Regarding #455, I'm working on a slightly larger change next to support traffic control elements (stop signs and traffic lights), so I might be able to do some unification when I send that PR.

@QuanyiLi
Copy link
Member

QuanyiLi commented Sep 22, 2023 via email

@QuanyiLi QuanyiLi merged commit 3a7c740 into main Oct 18, 2023
7 checks passed
@QuanyiLi QuanyiLi deleted the top-down-scenario-support branch October 18, 2023 15:59
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