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

Enhancements to parking environment #464

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

Conversation

sowmen
Copy link

@sowmen sowmen commented May 1, 2023

Added the following enhancements to the parking environment:

  • Support for a variable number of rows
  • Displays unique ids for each parking spot for simulation
  • Added random start positions for the agent
  • Variable number of parked vehicles (0 -> n_spots)
  • Possible to add external obstacles on the road
  • Works with sb3 and new gymnasium
  • All changes are configurable with the config parameters

2 rows with walls

4 rows

6 rows

6 rows all cars

Additional Obstacles

Random agent location

Copy link
Collaborator

@eleurent eleurent left a comment

Choose a reason for hiding this comment

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

Hi, thanks a lot for the very nice features.
I am just slightly concerned that this PR adds quite a lot of boilerplate code which I am not sure is needed. I am also not sure why you felt the need of printing the lane identifiers, I don't really see the use for this and it also adds unnecessary complexity (per-lane identifier, font-size...).
Will approve subject to code quality improvements.

elif lane.line_types[side] == LineType.CONTINUOUS_LINE:
cls.continuous_line(lane, surface, stripes_count, s0, side)

if lane.display_font_size != None:
x = (last_pixel[0][0] + last_pixel[1][0]) // 2 - 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that passing around this additional return_last_pixel argument and the associated returned value is a bit awkward, it adds unnecessary complexity, requires changing the several functions signature (which are now inconsistent) etc.
Instead, you can just recompute the coordinate here: surface.vec2pix(lane.position(lane.length, 0))

net.add_lane("a", "b", StraightLane([x, y_offset], [x, y_offset+length], width=width, line_types=lt))
net.add_lane("b", "c", StraightLane([x, -y_offset], [x, -y_offset-length], width=width, line_types=lt))

row_ys = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be simplified, why is this list needed? is the y coordinate not just a linear function of the row index? maybe make this a lambda function.

allowed_y_space.append([row_ys[i][0]+8, row_ys[i-2][1]-8])

self.allowed_vehicle_space = {
'x' : (-30, 30),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this is hardcoded, it doesn't seem to adapt to other configs. Also, this is not used here, please move to create_vehicles.

'y' : allowed_y_space
}

# Walls
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why the code for walls is that much longer, it used to be 2 loops for horizontal and vertical walls. Can you not just change the endpoints positions to adapt to number of rows?

"""Display the road and vehicles on a pygame window."""
if not self.enabled:
return

self.sim_surface.move_display_window_to(self.window_position())
self.sim_surface.move_display_window_to(self.window_position() if center else [0.5, 0.5])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you should edit these common files in abstract and graphics, instead just subclass the window_position() method in the environment to customise it.
Also, I don't understand why you use [0.5, 0.5] as a centre position, this is in meters so it looks like a strange choice to me.

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